1435170 - Split out --enable-ubsan-uint-overflow from --enable-ubsan-int-overflow

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Signed integer overflow is undefined behavior.  It is unequivocally bad.

Unsigned integer overflow is fully defined behavior.  In many cases it is exactly what the doctor ordered.

The code in the tree to enable integer overflow checking, does so for *both* flavors of overflow.  This results in many, many spurious warnings for code that deliberately depends on overflow behavior.  For example, if I comment out the exclusion of js/src/* from overflow consideration and do a build, this macro

#define JS7_ISDEC(c)    ((((unsigned)(c)) - '0') <= 9)

triggers nearly a dozen different overflow warnings -- despite that detecting digits 0-9 only, and rejecting all others, is exactly what that implementation does as it depends upon unsigned integer overflow.

At least in clang -- and that appears to be the only compiler supported for this right now -- these are split into two sanitize= names.  No reason we can't have two flags for this.  And we probably should have two blacklists files, too, but this at least lets me build with integer sanitizing without seeing a whole ton of spew that often was intended.

Unfortunately (?), I don't know of any signed integer overflows in SpiderMonkey right now, so I had to test this by reintroducing bug 1432646 temporarily locally.  That done, this does seem to work as intended.

(I should also note, I'm not sure why we were using -fsanitize=integer and not -fsanitize=undefined here.  The latter is UB, *only* UB, and will grow as the sanitizers get smarter.  I don't know why we wouldn't use that and ignore the integer-specific modes entirely.)

Comment 2

[Jeff Walden [:Waldo]]

Worth noting -- I tested locally, and multiple -fsanitize-blacklist= arguments get handled precisely as one would expect them to, which is to say the combination of all of them is what you get.

Comment 3

[Christian Holler (:decoder)]

(In reply to Jeff Walden [:Waldo] from comment #1)
> Created attachment 8947720 [details] [diff] [review]
> Patch
> 
> Signed integer overflow is undefined behavior.  It is unequivocally bad.
> 
> Unsigned integer overflow is fully defined behavior.  In many cases it is
> exactly what the doctor ordered.

I disagree. From a security standpoint, integer overflow is often unwanted and even for benign overflows, I've asked some developers around Mozilla with examples I had from this analysis and they weren't aware of the possibility of overflows in those locations. This only shows how dangerous and underestimated this is.

This analysis was about finding security problems (unwanted overflows), not about undefined behavior per se. And it was not used standalone but I tried to combine it with static analysis to cross-reference overflows that happen at runtime with overflows that would be bad according to static analysis. Unfortunately there is a lot of noise (not only from unsigned but mainly even from signed overflow) so it is a lot of work to blacklist noisy parts.


> 
> At least in clang -- and that appears to be the only compiler supported for
> this right now -- these are split into two sanitize= names.  No reason we
> can't have two flags for this.  And we probably should have two blacklists
> files, too, but this at least lets me build with integer sanitizing without
> seeing a whole ton of spew that often was intended.

I agree, having two flags is a good idea. There is an existing blacklist in build/sanitizers/ubsan_blacklist_int.txt that would have to be split and that is quite some work. 


> 
> Unfortunately (?), I don't know of any signed integer overflows in
> SpiderMonkey right now, so I had to test this by reintroducing bug 1432646
> temporarily locally.  That done, this does seem to work as intended.

If this works for SM, that's great news. The truth for the browser is: We have so many signed integer overflows that will explicitly not be fixed by developers that it does not make sense to differentiate there.

> 
> (I should also note, I'm not sure why we were using -fsanitize=integer and
> not -fsanitize=undefined here.  The latter is UB, *only* UB, and will grow
> as the sanitizers get smarter.

As I explained above already a bit, the analysis was about security issues, not UB. Unfortunately, developers do not really care about most UB types. If you can enforce this more strictly in SM, that's great and I'm fully supporting that. But for the browser, we tried this before and failed. Right now, we are working on enabling some UB modes in fuzzing and get rid of these (Tyson is working on that) which is a start.

Comment 4

[Christian Holler (:decoder)]

Comment on attachment 8947720 [details] [diff] [review]
Patch


>     if test -z "$CLANG_CL"; then
>         LDFLAGS="-fsanitize=integer $LDFLAGS"
>     fi

I guess this also needs splitting into signed/unsigned?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8947720 [details] [diff] [review]
Patch

Review of attachment 8947720 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.I wonder if we should rename the --enable-ubsan-int-overflow to --enable-ubsan-sint-overflow or something to make it slightly more obvious that we're only enabling signed integer overflow checks.  Obviously the help is clear on this point, but people actually read the help?  Pshaw.

OTOH, maybe people will already understand that 'int' is obviously signed, and go from there.  I can be convinced either way.

::: build/autoconf/sanitize.m4
@@ +94,5 @@
>      MOZ_UBSAN=1
> +    # The blacklist really should be split into separate signed/unsigned
> +    # blacklists, but we leave that task for another day.
> +    CFLAGS="-fsanitize-blacklist=$_topsrcdir/build/sanitizers/ubsan_blacklist_int.txt $CFLAGS"
> +    CXXFLAGS="-fsanitize-blacklist=$_topsrcdir/build/sanitizers/ubsan_blacklist_int.txt $CXXFLAGS"

Optional: pull the blacklist out into a separate variable and use that variable when defining CFLAGS and CXXFLAGS?

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Christian Holler (:decoder) from comment #3)
> From a security standpoint, integer overflow is often unwanted

Well, it really depends.

> and even for benign overflows, I've asked some developers around Mozilla
> with examples I had from this analysis and they weren't aware of the
> possibility of overflows in those locations. This only shows how dangerous
> and underestimated this is.

That it is sometimes underestimated, doesn't mean it always is underestimated.

> > Unfortunately (?), I don't know of any signed integer overflows in
> > SpiderMonkey right now, so I had to test this by reintroducing bug 1432646
> > temporarily locally.  That done, this does seem to work as intended.
> 
> If this works for SM, that's great news.

Well, I don't know how well it necessarily works -- but we can't know until we try.  I suspect generally we're in decent shape on the front.

> The truth for the browser is: We
> have so many signed integer overflows that will explicitly not be fixed by
> developers that it does not make sense to differentiate there.

Not initially, at least.  #NeverGiveUp  #NeverSurrender

I'll file a followup bug to split the blacklist into two and to rename the int-overflow option (which I left so named only because it kept the existing name doing something, rather than silently turning into a nothingburger).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70cfa86b3b9c
Split out --enable-ubsan-uint-overflow from --enable-ubsan-int-overflow.  r=decoder, r=froydnj

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/70cfa86b3b9c