Peter Hosey
Graham Sutherland / Polynomialreposting for the day crowd: I ran into a memcmp implementation that only compared 25% of the bytes, and the issue wasn't caught in the build because the vendor toolchain failed to emit a warning.
to be fair it should also have been unit tested but I'm gonna cut the devs some slack here because the toolchain vendor rugpulling a whole warning category is a significantly worse offense.
Dag Ågren ↙︎↙︎↙︎@gsuberland Pretty sure this would have passed the unit tests that anyone would have been likely to write anyway.
@WAHa_06x36 this is why fuzz testing is a thing!
@gsuberland Hmm, would even fuzz testing find it? That seems tricky to set up in a way that a) would actually find the bug and b) would occur to you before seeing the bug.
I guess for very short inputs you might find it more easily by chance...
@WAHa_06x36 of course. fuzz testing would quickly find memcmp("aaaa", "Aaaa") == 0 or memcmp("aaaa", "aaaA") == 0 as a violation of the contract (depending on endianness)
@gsuberland I mean, if you set up a special test harness against a known-good implementation and used something like afl that actually instruments the code itself, maybe, but, who would ever do that?
@WAHa_06x36 quite a few people! there are even coverage tools specifically for doing this.
@gsuberland Hmm, interesting, haven't seen those!
halcy 
@WAHa_06x36 @gsuberland i think „only one byte differs“ kind of tests would probably find it, right? And these seem like something you’d write to test that
@gsuberland oh yeah i saw this earlier and thought surely the compiler would yell at you for that comparison
oopsie
Martin Uecker@gsuberland Do you get a warning with GCC? If I am not mistaken, this is not catched by GCC nor clang with the warning flags shown. You need -Wconversion.
@gsuberland But the other huge problem are the strict aliasing violations.
@gsuberland You should certainly also mention the strict aliasing issue in your blog as clang / GCC *will* miscompile this code with optimization.
@uecker is the strict aliasing violation somewhere in that code, or in the assumption that a caller will cast an incompatible type to uint32_t*? (I'm not actually super familiar with the technicalities of strict aliasing rules beyond the typical struct cast example)
@gsuberland It is the assumption of the cast. It is implied by the description of this being a memcpy which would mean it should work also for other types than uint32_t. But if you access some other type using uint32_t then this would be a strict aliasing violation.
@uecker presumably it should be const void*?
@gsuberland The argument should be void* and const void* but the type of the pointer argument does not matter. Making the access with uint32_t is undefined if the type of the object is something else. There is an exception for character types so this works. otherwise one has to use a compiler extension (.e.g. may_alias), but then why not directly use the compiler built-ins for memcpy?
@gsuberland or one compiles with -fno-strict-aliasing.
@uecker I might've bungled the flags in the post, 'cos I was tired, but the actual flags they were using in the build did generate the warning in gcc.
I would expect -Wnarrowing to catch implicit narrowing conversions, though.
@gsuberland It is a narrowing conversion, but it seems C++ only disallows this in initializer lists and this is when compiler warn:
https://eel.is/c++draft/dcl.init.list#def:conversion,narrowing
https://eel.is/c++draft/dcl.init.list#def:conversion,narrowing
Daniel Gibson@uecker @gsuberland
shouldn't things that are disallowed be errors, while things that are allowed but probably a bad idea warnings?
shouldn't things that are disallowed be errors, while things that are allowed but probably a bad idea warnings?
@Doomed_Daniel @gsuberland Obviously. The problem is there are too many people with broken code that do not want to fix it. For example, implicit int in C was disallowed in C99, GCC made it a hard error in 2024 (GCC 14) - 25 years later.
@uecker if -Wnarrowing doesn't catch narrowing conversions then I will edit the post to say "also gcc is terrible at naming things and encourages bugs as a result"
@gsuberland Fair. You should add clang as well... and please add that you need to use -Wconversion
John McCall@gsuberland @uecker I won’t defend Clang’s naming choices in every case, but I believe this specific one is all GCC; Clang originally called this -Wc++0x-narrowing (eventually -Wc++11-narrowing) and only added the -Wnarrowing alias for GCC compatibility. In any case, the documentation should really suggest -Wconversion, and on that front I can definitely accept blame for Clang, because our warning group documentation is awful
@gsuberland @rjmccall It seems it is under the language dialects options and explanation is not really clear. https://gcc.gnu.org/onlinedocs/gcc-15.2.0/gcc/C_002b_002b-Dialect-Options.html
Ryan Castellucci (they/them) 
@gsuberland that seems not good
@ryanc funnily enough that's exactly what they said on the readout
artemist@gsuberland I've found pointer overflows before this way, looking at code that was only ever compiled under gcc 4 with a modern compiler.
The Witch of Crow Briar@gsuberland memcmp: “meh, we made it this far, the rest is probably just like this, right? Time for a quick smoke”
Nikita Lisitsa@gsuberland Oh my, that's horrible!
Btw what do you mean by "constant-time"? It seems that the function would take time linear in "words" parameter.
@lisyarus it's a security thing. it takes a time that is constant independent of the contents of the data. it avoids timing side channel attacks that might leak the contents. the length of the buffer is not considered secret so it doesn't need to be masked.
@lisyarus for example if I did:
if (strcmp(password, "hunter2") == 0) { ... }
strcmp has an early-out optimisation where the comparison loop exits at the first mismatched character. by iterating each character sequentially while timing how long the comparison takes I can discover which character causes the comparison time to increase slightly, which lets me discover the password letter by letter.
@gsuberland Oh, so it means "constant if the length is considered fixed, regardless of the buffer contents" ?
@gsuberland I see, thank you!