Icinga / icingadb

Icinga configuration and state database supporting multiple environments
https://icinga.com
GNU General Public License v2.0
59 stars 20 forks source link

Retention `Count` config `uint64` overflow #755

Open yhabteab opened 5 months ago

yhabteab commented 5 months ago

Describe the bug

The config.Retention.Count is user configurable via the YAML file and thus should not be of type uint64, otherwise it can easily be overflowed.

can't parse YAML file config.example.yml: cannot unmarshal -10 into Go value of type uint64 ( overflow )
exit status 1
julianbrost commented 5 months ago

Note that it's not that uint64 in inherently wrong here, it's just that the YAML parser we use seems to return a more helpful error message with mismatching types (parsing string into int) than with integer values out out range:

$ go run ./cmd/icingadb --config <(echo 'retention: {count: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint64 ( overflow )
exit status 1
$ go run ./cmd/icingadb --config <(echo 'retention: {count: "str"}')
can't parse YAML file /proc/self/fd/11: [1:20] cannot unmarshal string into Go struct field Config.Retention of type uint64
>  1 | retention: {count: "str"}
                          ^

exit status 1

Using a signed int could be a workaround to provide a more helpful error message, at least in some more common error cases (something like trying -1 to disable something).

oxzi commented 3 months ago

This issue is not restricted to the Count struct field, but to every uint64 configuration field.

$ ./icingadb --config <(echo 'retention: {history-days: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint64 ( overflow )
$ ./icingadb --config <(echo 'retention: {sla-days: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint64 ( overflow )

A quick fix could be to change the type to int64 and add a greater-zero check in Validate(), but this feels a bit like throwing the type system under the bus. Thus, as @julianbrost wrote, I would say that this is something to be fixed in the YAML library.

oxzi commented 3 months ago

This would be a quick fix addressing this https://github.com/goccy/go-yaml/pull/470.

oxzi commented 1 week ago

The mentioned PR got merged and a new go-yaml release was drafted, #832. In the meantime, the retention size was also modified in #824. @yhabteab, would you please take another look at this issue?