Re: Fix UBSAN issue

[Tim Rühsen]

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  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 ni...@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

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

signature.asc
Description: This is a digitally signed message part.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs

Re: Fix UBSAN issue

[Tim Rühsen]

On Mittwoch, 8. November 2017 12:44:28 CET Niels Möller wrote:
> Tim Rühsen  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 ni...@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

signature.asc
Description: This is a digitally signed message part.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs

Re: Fix UBSAN issue

[Niels Möller]

Tim Rühsen  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 ni...@google.com.

Regards,
/Niels

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs

Re: Fix UBSAN issue

[Tim Rühsen]

On Sonntag, 5. November 2017 23:20:27 CET Niels Möller wrote:
> Tim Rühsen  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


signature.asc
Description: This is a digitally signed message part.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs

Re: Fix UBSAN issue

[Niels Möller]

Tim Rühsen  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

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
___
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs