reposting 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.
-
@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 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.
@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.
-
@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 -
@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.
@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 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@uecker @gsuberland
shouldn't things that are disallowed be errors, while things that are allowed but probably a bad idea warnings? -
@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
-
@uecker @gsuberland
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.
-
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.
@gsuberland Pretty sure this would have passed the unit tests that anyone would have been likely to write anyway.
-
@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!
-
@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...
-
@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 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 @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 Fair. You should add clang as well... and please add that you need to use -Wconversion
@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 @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
-
@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
-
@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?
-
@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.
-
@WAHa_06x36 quite a few people! there are even coverage tools specifically for doing this.
@gsuberland Hmm, interesting, haven't seen those!
-
reposting 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.
@gsuberland that seems not good
-
R relay@relay.infosec.exchange shared this topic
