Suppressing sanitizer in sha256.c

[Tim Rühsen]

When running with clang's sanitizing on, there is a UB runtime error
triggered in sha256.c.

This is expected behavior but still it rings the 'alarm bell'.


This patch suppresses it:


diff --git a/lib/sha256.c b/lib/sha256.c
index 85405b20f..cf161c65c 100644
--- a/lib/sha256.c
+++ b/lib/sha256.c
@@ -448,6 +448,9 @@ static const uint32_t sha256_round_constants[64] = {
    It is assumed that LEN % 64 == 0.
    Most of this code comes from GnuPG's cipher/sha1.c.  */
 
+#ifdef __clang__
+__attribute__((no_sanitize("unsigned-integer-overflow")))
+#endif
 void
 sha256_process_block (const void *buffer, size_t len, struct sha256_ctx
*ctx)
 {


Regards, Tim

[Paul Eggert]

Post by Tim Rühsen
This is expected behavior but still it rings the 'alarm bell'.

My kneejerk reaction is that the code has well-defined behavior and I'd
rather that developers didn't use -fsanitize=unsigned-integer-overflow.
For Gnulib, that flag is more trouble than it's worth.

[Eric Blake]

Post by Paul Eggert

Post by Tim Rühsen
This is expected behavior but still it rings the 'alarm bell'.

My kneejerk reaction is that the code has well-defined behavior and I'd
rather that developers didn't use -fsanitize=unsigned-integer-overflow.
For Gnulib, that flag is more trouble than it's worth.

I can see the validity of claiming that signed integer overflow is
undefined behavior, but I thought the C standard was pretty clear that
unsigned integer overflow is well-defined and performs modulo
arithmetic. What are the clang developers using as their justification
for this warning? Paul is probably correct that the warning is a bug in
clang, if they can't back up their warning with an actual quote from the
C standard.

[Bruno Haible]

Post by Eric Blake
What are the clang developers using as their justification
for this warning?

Quoting the clang documentation [1]:

"-fsanitize=unsigned-integer-overflow:
Unsigned integer overflows. Note that unlike signed integer overflow,
unsigned integer is not undefined behavior. However, while it has well-
defined semantics, it is often unintentional, so UBSan offers to catch it."

Bruno

[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

[Tim Rühsen]

Post by Bruno Haible

Post by Eric Blake
What are the clang developers using as their justification
for this warning?

Unsigned integer overflows. Note that unlike signed integer overflow,
unsigned integer is not undefined behavior. However, while it has well-
defined semantics, it is often unintentional, so UBSan offers to catch it."
Bruno
[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

Thanks for looking into it.

To squeeze out as many potential bugs from my applications, I turn all
sanitizer options on. Then I reduce false positives by tagging the code
parts / functions. A bit like deduction.

The bad thing with clang is that I can't tag the calling function (in my
application) but have to set the attribute for the function that
triggers (in gnulib).

Gnulib is made to serve app/lib developers. And reducing false positives
would be of great help to reduce time spent into securing code that
uses gnulib. But I understand if you deny to that on the gnulib's side -
you would possibly open a can of worms.

Regards, Tim

[Paul Eggert]

reducing false positives > would be of great help to reduce time spent into securing code that
uses gnulib.

Yes, and Gnulib tries to strike a balance here. For Gnulib headers, we
try harder to pacify compilers even if we think their warnings are
misguided. This particular bug report was about a .c file, though, and
for that it's reasonable to suggest that you not use the compile-time
option that enables this flag when compiling Gnulib code, as it is more
trouble than it's worth in Gnulib code.

[Tim Rühsen]

Post by Paul Eggert

reducing false positives  > would be of great help to reduce time
spent into securing code that
uses gnulib.

Yes, and Gnulib tries to strike a balance here. For Gnulib headers, we
try harder to pacify compilers even if we think their warnings are
misguided. This particular bug report was about a .c file, though, and
for that it's reasonable to suggest that you not use the compile-time
option that enables this flag when compiling Gnulib code, as it is
more trouble than it's worth in Gnulib code.

Thanks, good to know.

Regards, Tim