search

bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
685 stars 84 forks source link

Integer overflow due to u64->u32 when passing the "limits" around (most likely happen to `max_bytes`) #336

Open Alkaid-Benetnash opened 10 months ago

Alkaid-Benetnash commented 10 months ago

Hi,

I believe my local limits config caused integer overflow, which in turn caused dbus-broker launch failures.

In particular, there is a u64->u32 downcast from broker_new to bus_init, which then propagate to struct UserRegistry and struct User.

For the specific limit max_bytes, which is the multiple of max_connections_per_user and max_outgoing_bytes, can easily overflow a u32 (i.e., 4GiB).

Current default config (64 8iMB = 512MiB) will not overflow. But for example the reference limits from dbus-daemon, as shown in the end-of-line comments (64 127MiB), will overflow.

In my case, my customized config resulted in max_connections_per_user * max_outgoing_bytes == 2^32, which wrapped to 0 in u32. And max_bytes == 0 caused the launch failure.

I wonder whether those u32 should be replaced with u64?

Thanks!

dvdhrm commented 10 months ago

Urgs, nice catch. This definitely needs to use saturating arithmetic, rather than overflow.

I am unsure about 64bit. It would significantly slow down calculations on 32bit machines, but maybe we dont care. It is definitely reasonable for a system message broker to be using >2^32 as memory limit. Mhhh.

Alkaid-Benetnash commented 10 months ago

Then maybe change those limits type to size_t, which is u32 on 32bit machine and u64 on 64bit machine.

Also check for u64 overflowing size_t in launcher.

nolange commented 10 months ago

i think i ran into this aswell.

I am unsure about 64bit. It would significantly slow down calculations on 32bit machines, but maybe we dont care.

what are you calculating? addition/substraction/comparison shouldn't hurt too much. multiplication might, but i dont think that's in a "hot path".

nolange commented 1 month ago

This is still a problem. It should at the very least log a warning on overflow or even stop the daemon from starting. That is if there are good reasons for not just using 64bit.

nolange commented 1 month ago

I took the only function where the limits are apparently calculated, and switched the relevant parts to 64 bit.

The impact on Arm32 seems ok, aswell as x86 with clang. but gcc on x86 has a wild result: https://godbolt.org/z/M6do9bxPn