IcingaDB::PrepareObject(): round Notification#interval and limit it to >=0

[Al2Klimov]

otherwise, e.g. with -42.5 (my successful before/after test case btw.), the Go daemon crashes. It expects uints there.

[yhabteab]

The changes look fine, but since you're already touching Notification-related stuff, I'd also check for any other attributes of that type that would cause Icinga DB to crash. First example times.begin:

2023-06-20T07:27:41.430Z        FATAL   icingadb        json: cannot unmarshal number 30.3 into Go value of type int64
can't unmarshal JSON into *int64
github.com/icinga/icingadb/internal.UnmarshalJSON
        /go/src/github.com/Icinga/icingadb/internal/internal.go:47
github.com/icinga/icingadb/pkg/types.(*Int).UnmarshalJSON
        /go/src/github.com/Icinga/icingadb/pkg/types/int.go:52
...

[Al2Klimov]

Actually I've done that, but completely overseen that particular attribute attribute. Thanks. I'll make a separate PR.

[yhabteab]

I know this isn't an Icinga 2 problem, but rather an Icinga DB one, nonetheless setting the notification.interval to Math.pow(2,32) still crashes Icinga DB.

2023-06-20T10:19:55.792Z        FATAL   icingadb        json: cannot unmarshal number 4294967296 into Go struct field Notification.notification_interval of type uint32
can't unmarshal JSON into *v1.Notification
github.com/icinga/icingadb/internal.UnmarshalJSON
        /go/src/github.com/Icinga/icingadb/internal/internal.go:47
github.com/icinga/icingadb/pkg/icingaredis.CreateEntities.func1.1

[Al2Klimov]

And, let me guess, if I double both the Go struct data type bits and the second argument to Math.pow, it still crashes?

[yhabteab]

And, let me guess, if I double both the Go struct data type bits and the second argument to Math.pow, it still crashes?

Yep! But I don't think we have valid use cases for using > 64 bit integer. You can of course set a value on the Icinga2 side that is beyond 64 bit signs but that would not only crash the Icinga DB daemon.

[Al2Klimov]

I doubt the latter.

<1> => Math.pow(2,128)
340282366920938463463374607431768211456.000000
<2> => Math.pow(2,128) < get_time()
false
<3> => Math.pow(2,128) > get_time()
true
<4> =>

But even Math.pow(2,32) in prod is a provocation, not an actual usage IMAO.

[julianbrost]

But even Math.pow(2,32) in prod is a provocation, not an actual usage IMAO.

If it's accepted and it crashes, it's a bug. Imagine if some (restricted) user is allowed to edit some objects and this allows them to crash the process, that's not good.

[Al2Klimov]

If this is your benchmark, you have just opened the pandora can. A lot of stuff is affected by this sabotage possibility. But instead of replicating signed/unsigned and bits amount info from Go to C++, I'd close/stall all such PRs of mine and instead make a templated type on Go side which handles all this gracefully. What do you think?

type MakeItEinfachGeil[T NumericConstraint] T

[julianbrost]

As discussed in our team meeting earlier today: most of the bad stuff should be handled by Icinga/icingadb#607, i.e. some funky object shouldn't bring down everything in Icinga DB. But the overall goal should still be that Icinga 2 writes consistent information into Redis, so the PRs here shouldn't be abandoned.

These concerns will be addressed separately: #9794 Icinga/icingadb#607