Nothing with a real impact, just to silence sanitizers.
From OSS-Fuzz:
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /src/libfuzzer/FuzzerSHA1.cpp:132:14 in sec-tabselect.c:58:24: runtime error: negation of 1 cannot be represented in type 'mp_limb_t' (aka 'unsigned long')
Patch attached.
With Best Regards, Tim
Tim Rühsen tim.ruehsen@gmx.de writes:
Nothing with a real impact, just to silence sanitizers.
Can you explain precisely what's undefined behavior in this code ?
diff --git a/sec-tabselect.c b/sec-tabselect.c index e6bf2282..942c4247 100644 --- a/sec-tabselect.c +++ b/sec-tabselect.c @@ -55,7 +55,7 @@ sec_tabselect (mp_limb_t *rp, mp_size_t rn, mpn_zero (rp, rn); for (p = table; p < end; p += rn, k--) {
mp_limb_t mask = - (mp_limb_t) (k == 0);
As far as I understand, this should be perfectly portable C.
(k == 0) evaluates to zero or one, with int type.
This always fits in an mp_limb_t, hence
(mp_limb_t) (k == 0) evaluates to zero or one, with mp_limb_t type.
And since mp_limb_t is an *unsigned* type, arithmetic is always well defined as being performed modulo (ULONG_MAX + 1), including unary negation. So
-(mp_limb_t) (k == 0) evaluates to zero or ULONG_MAX.
(Assuming mp_limb_t is unsigned long, which it is an almost anything except 64-bit windows, where it's instead unsigned long long).
But I may be missing something? These corners of the C language are a bit subtle.
mp_limb_t mask = (mp_limb_t) -(k == 0);
If the other way isn't broken, I'd prefer to change it like this. Because then one also has to think about why it produces the intended sign extension (which it does; it's not the same as (mp_limb_t) (unsigned) -(k == 0)).
In general, both nettle and GMP depend on well-defined modulo arithmetic on unsigned types in *lots* of places. Any sanitizer which complains about that is pretty useless for this code. If your sanitizer complains by default, please use some option to disable that. And if there's no such option, please bug report the sanitizer tool.
Best regards, /Niels
On Sonntag, 5. November 2017 23:20:27 CET Niels Möller wrote:
Tim Rühsen tim.ruehsen@gmx.de writes:
Nothing with a real impact, just to silence sanitizers.
Can you explain precisely what's undefined behavior in this code ?
diff --git a/sec-tabselect.c b/sec-tabselect.c index e6bf2282..942c4247 100644 --- a/sec-tabselect.c +++ b/sec-tabselect.c @@ -55,7 +55,7 @@ sec_tabselect (mp_limb_t *rp, mp_size_t rn,
mpn_zero (rp, rn); for (p = table; p < end; p += rn, k--)
{
mp_limb_t mask = - (mp_limb_t) (k == 0);As far as I understand, this should be perfectly portable C.
(k == 0) evaluates to zero or one, with int type.
This always fits in an mp_limb_t, hence
(mp_limb_t) (k == 0) evaluates to zero or one, with mp_limb_t type.
And since mp_limb_t is an *unsigned* type, arithmetic is always well defined as being performed modulo (ULONG_MAX + 1), including unary negation. So
-(mp_limb_t) (k == 0) evaluates to zero or ULONG_MAX.
(Assuming mp_limb_t is unsigned long, which it is an almost anything except 64-bit windows, where it's instead unsigned long long).
But I may be missing something? These corners of the C language are a bit subtle.
mp_limb_t mask = (mp_limb_t) -(k == 0);If the other way isn't broken, I'd prefer to change it like this. Because then one also has to think about why it produces the intended sign extension (which it does; it's not the same as (mp_limb_t) (unsigned) -(k == 0)).
In general, both nettle and GMP depend on well-defined modulo arithmetic on unsigned types in *lots* of places. Any sanitizer which complains about that is pretty useless for this code. If your sanitizer complains by default, please use some option to disable that. And if there's no such option, please bug report the sanitizer tool.
Thanks for taking such a detailed look !
Well, this is not really "my" sanitizer, it's "the" sanitizer (llvm/clang) used by Google's OSS-Fuzz. (gcc eventually implements clang's sanitizer features but is very behind).
And of course you might be right and this is a bug in the sanitizer. I leave that discussion to the experts and open a bug at oss-fuzz as soon as I find the time.
With Best Regards, Tim
Tim Rühsen tim.ruehsen@gmx.de writes:
And of course you might be right and this is a bug in the sanitizer. I leave that discussion to the experts and open a bug at oss-fuzz as soon as I find the time.
Ok. Please add me to cc me on any related bugs, my personal address if that works, otherwise my work address nisse@google.com.
Regards, /Niels
On Mittwoch, 8. November 2017 12:44:28 CET Niels Möller wrote:
Tim Rühsen tim.ruehsen@gmx.de writes:
And of course you might be right and this is a bug in the sanitizer. I leave that discussion to the experts and open a bug at oss-fuzz as soon as I find the time.
Ok. Please add me to cc me on any related bugs, my personal address if that works, otherwise my work address nisse@google.com.
Just opened an issue on Github. So i put a link here, if someone else is interested.
https://github.com/google/oss-fuzz/issues/971
With Best Regards, Tim
On Mittwoch, 8. November 2017 21:29:57 CET Tim Rühsen wrote:
On Mittwoch, 8. November 2017 12:44:28 CET Niels Möller wrote:
Tim Rühsen tim.ruehsen@gmx.de writes:
And of course you might be right and this is a bug in the sanitizer. I leave that discussion to the experts and open a bug at oss-fuzz as soon as I find the time.
Ok. Please add me to cc me on any related bugs, my personal address if that works, otherwise my work address nisse@google.com.
Just opened an issue on Github. So i put a link here, if someone else is interested.
I just closed that bug since they agree with you that negating an unsigned type is defined. And the funny part is, those bug/issue reports (there were more on the oss- fuzz report) are gone now.
Thanks for your investigation !
Regards, Tim
nettle-bugs@lists.lysator.liu.se
-
nisse＠lysator.liu.se -
Tim Rühsen