UBSan: signed integer overflow in [@ ToIntWidth]

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: tsmith, Assigned: Waldo)

Tracking

({csectype-undefined})

60 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments)

Found with changeset: 400421:c5461973d6ee
Built with -fsanitize=signed-integer-overflow

/include/js/Conversions.h:394:22: runtime error: signed integer overflow: -2147483648 + -2147483648 cannot be represented in type 'int'
    #0 0x7fbdeb6088d4 in int JS::detail::ToIntWidth<int>(double) /objdir-ff-ubsan/dist/include/js/Conversions.h:394:22
    #1 0x7fbdf44f3533 in js::ToInt32Slow(JSContext*, JS::Handle<JS::Value>, int*) /js/src/jsnum.cpp:1724:12
    #2 0x7fbdf40ba67e in ToInt32 /objdir-ff-ubsan/dist/include/js/Conversions.h:167:12
    #3 0x7fbdf40ba67e in BitAnd /js/src/vm/Interpreter-inl.h:769
    #4 0x7fbdf40ba67e in js::jit::DoBinaryArithFallback(JSContext*, void*, js::jit::ICBinaryArith_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /js/src/jit/SharedIC.cpp:752
    #5 0x11633b6d158a  (<unknown module>)
By inspection, JS::ToInt32(0xFFFFFFFF) triggers this.  My clang is not in a state to compile a JS shell with -fsanitize=undefined (not sure why), so I'm updating my clang build to see about going from there.  But with a little futzing, I cobbled together a toy program to verify the hypothesis.  It confirms the hypothesis (obviously you'd modify the various paths if you wanted to run it on your own system):

[jwalden@find-waldo-now tmp]$ LLVMCONFIG="/home/jwalden/Programs/old-clang/prefixdir/bin/llvm-config" \
>   ~/Programs/old-clang/prefixdir/bin/clang++ \
>     -DDEBUG -fsanitize=undefined -std=c++11 \
>     -I ~/moz/after/js/src/dbg/dist/include/ \
>     -g -o overflow \
>     overflow.cpp 
[jwalden@find-waldo-now tmp]$ ./overflow
overflow.cpp:123:22: runtime error: signed integer overflow: -2147483648 + -2147483648 cannot be represented in type 'int'

I'll take a look here.  I wrote this code and/or genericized it to arbitrary integer widths back in the day, and I tried to be pretty careful, so it's moderately surprising something slipped through.  Given prior attempt at rigor, hopefully this won't be too hard to fix.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Posted patch PatchSplinter Review
The code to invoke this bug is trivial, but because it's one of those not-entirely-visible sorts of things, I'm not sure there's a way to test it.
Attachment #8945207 - Flags: review?(nfroyd)
Actually, let's pull this algorithm into a more general location.
Attachment #8945262 - Flags: review?(nfroyd)
Comment on attachment 8945207 [details] [diff] [review]
Patch

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

Thank you for the extensive comments.

I guess for testing it, we could just use tests that converted some known values, particularly the testcase mentioned in the commit message?  Or a short program to test all the doubles for int32_t/uint32_t conversion correctness; such a program wouldn't take very long to run on modern hardware, but maybe wouldn't be appropriate for the testsuite...  Don't know where those would go, though.
Attachment #8945207 - Flags: review?(nfroyd) → review+
Comment on attachment 8945262 [details] [diff] [review]
Implement mozilla::WrapToSigned

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

I don't have any problem with moving this, I guess, but see below.

::: mfbt/MathAlgorithms.h
@@ +597,5 @@
> + * Convert an unsigned value to signed, if necessary wrapping around.
> + *
> + * This is the behavior normal C++ casting will perform in most implementations
> + * these days -- but this function makes explicit that such conversion is
> + * happening.

Alternatively, why isn't:

union {
  UnsignedType u;
  SignedType s;
} u;
u.u = aValue;
return u.s;

an equally valid implementation?  Then it's very clear that a bitwise conversion is happening, with no implementation-defined behavior, and the compiler ought to be able to optimize it just as well... (gcc and clang do, at least)?   We have BitwiseCast to do this for us already, even.
Attachment #8945262 - Flags: review?(nfroyd) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/816c9ead4b0c
Don't overflow performing signed integer arithmetic when performing |JS::ToInt32(0xFFFFFFFF)|.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/e266383ee349
Implement mozilla::WrapToSigned.  r=froydnj
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d789409cb08
Followup bustage fix for compilers that warn (error with -Werror-alike) when negating an unsigned expression.  r=bustage in a CLOSED TREE
Depends on: 1514794
You need to log in before you can comment on or make changes to this bug.