thom311 / libnl

Netlink Library Suite
GNU Lesser General Public License v2.1
419 stars 311 forks source link

lib: Avoid overflow in computation of s_seq_next. #395

Closed cferris1000 closed 1 month ago

cferris1000 commented 1 month ago
On some systems, the clock is reset, or is lost, so the value returned
by the time function can be a very small value. In that case, the
_badrandom_from_time function returns a large value close to the
maximum unsigned int value for s_seq_next. This can lead to the value
wrapping around fairly quickly.

When compiling the library with the unsigned-integer-overflow sanitizer
enabled, this causes an abort.

Detect this potential wrap around condition and avoid it.
thom311 commented 1 month ago

yes, good.

But in the commit message, could you better explain why this is done? It's not at all obvious. Yes, we know that overflows can happen, and that they may happen easier if _badrandom_from_time() returns a large value. But what is the problem with the overflow?

You say you are building with a certain compiler option(?) that flags unsigned wrap around as bugs. Which option is that? Why are you even doing that? etc.

Please elaborate in the commit message.

thom311 commented 1 month ago

Also, make it clear that the "overflow" in question is about an unsigned integer. Which according to C is well defined. So it's relevant to understand what the problem with this is, otherwise, the next time somebody reviews this patch, they will just revert it.

Maybe a clearer term for "overflow of an unsigned integer" is "wrap around".

cferris1000 commented 1 month ago

I updated the comment, see if this is more informative than before.

thom311 commented 1 month ago

merged to main. Thank you!!