1404251 - Fix more UBSan issues in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[André Bargull [:anba]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5014dec2f36c49fc079d6b1bf2714d8d1aae6cb2

Also known as: Replace undefined behaviour with implementation-defined behaviour (signed integer overflow vs. cast from unsigned to signed when the unsigned value is larger than INT32_MAX)... :-/

Comment 1

[André Bargull [:anba]]

Attachment: bug1404251.patch

The majority of these changes are merely exchanging undefined behaviour (signed integer overflow, left-shifting a negative signed int, ...) with implementation-defined behaviour (casting an unsigned integer greater than INT32_MAX to a signed integer). That's not really great, but at least it fixes the UBSan issues. Waldo suggested [1] to create a MFBT function to avoid the implementation-defined behaviour, but I'd like to create a follow-up bug for that work, because it will probably take longer to bike-shed the MFBT function and then to fix all places in SpiderMonkey where we already cast values to unsigned to avoid UB (bug 1401209, bug 934520, ...).

The issues fixed in this patch were found by running the jstests and jit-tests in an UBSan-enabled SpiderMonkey build.

[1] https://mozilla.logbot.info/jsapi/20171031#c13788269

jit/IonAnalysis.cpp:
vm/Interpreter.cpp:
- Prevent signed integer overflow in additions/subtractions.

jit/JitcodeMap.cpp:
wasm/WasmValidate.h:
- Prevent shifting a negative signed integer, because that's UB.

jit/shared/CodeGenerator-shared-inl.h:
jit/x86-shared/Disassembler-x86-shared.cpp:
- Prevent shifting a negative signed integer, because that's UB.
- The result must be signed, so additionally cast the unsigned shift result back to signed.

jit/MCallOptimize.cpp:
- Ensure |sharedness| is always initialized, because it's UB to read an uninitialized local variable.

jit/RegisterSets.h:
- Use an explicit state to encode an uninitialized |ABIArg::Kind|, because it's UB to cast an out-of-range value to an enum value.
- Then assert |ABIArg::kind_| is never uninitialized in |ABIArg::kind()| for extra safety.
- And fix a few switch-statements to handle the new |ABIArg::Kind|.

vm/UbiNodeCensus.cpp:
wasm/WasmModule.cpp:
wasm/WasmSerialize.h:
- Avoid calling qsort(...) or memcpy(...) with a nullptr value, because that's also UB even when the passed array length is zero.

builtin/SIMD.cpp:
- Adds |MaybeMakeUnsigned| to convert signed integers to unsigned integers. For any other types it's the identity function.
- |-1 * negative_signed_int| is UB, so cast the value to an unsigned integer before multiplying it with |-1|.
- Prevent signed integer overflow in Add, Sub, and Mul.
- And add some static_asserts, so it's easier to see why Div, RecApprox, RecSqrtApprox, and Sqrt don't need to use MaybeMakeUnsigned.

Comment 2

[André Bargull [:anba]]

(In reply to André Bargull [:anba] from comment #1)
> The issues fixed in this patch were found by running the jstests and
> jit-tests in an UBSan-enabled SpiderMonkey build.

But the patch does not fix all UBSan issues when running these two test suites! There are still a few warnings about misaligned  loads and stores of variables and member accesses within misaligned addresses (jit/JitFrames.cpp and x86-shared/Assembler-x86-shared.h), signed overflows and left-shifting of negative values (fdlibm), and an integer overflow in GCRuntime::runDebugGC().

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8928499 [details] [diff] [review]
bug1404251.patch

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

LGTM, thanks for the patch!

::: js/src/builtin/SIMD.cpp
@@ +755,5 @@
>  };
>  template<typename T>
>  struct Neg {
> +    using MaybeUnsignedT = typename detail::MaybeMakeUnsigned<T>::Type;
> +    static T apply(T x) { return -1 * MaybeUnsignedT(x); }

Will -1 be coerced to an unsigned value in this case, or will the RHS be converted to a signed (thus cancelling the fix)? Could we explicitly coerce the LHS to unsigned, to remove any doubt?

::: js/src/vm/UbiNodeCensus.cpp
@@ +357,5 @@
>  
>      for (auto r = map.all(); !r.empty(); r.popFront())
>          entries.infallibleAppend(&r.front());
>  
> +    if (entries.length() > 0) {

nit: if (entries.length())

@@ +576,5 @@
>      if (!entries.reserve(count.table.count()))
>          return false;
>      for (Table::Range r = count.table.all(); !r.empty(); r.popFront())
>          entries.infallibleAppend(&r.front());
> +    if (entries.length() > 0)

ditto

@@ +743,5 @@
>      if (!entries.reserve(count.table.count()))
>          return false;
>      for (Table::Range r = count.table.all(); !r.empty(); r.popFront())
>          entries.infallibleAppend(&r.front());
> +    if (entries.length() > 0)

ditto

::: js/src/wasm/WasmIonCompile.cpp
@@ +993,5 @@
>              curBlock_->add(mir);
>              return call->stackArgs_.append(mir);
>            }
> +          case ABIArg::Uninitialized:
> +            MOZ_ASSERT_UNREACHABLE("Uninitialized ABIArg kind");

Weren't we told to use MOZ_MAKE_COMPILER_ASSUME_UNREACHABLE in this kind of situation? (I think it's ok since in the worst case this falls through to the MOZ_CRASH anyways)

::: js/src/wasm/WasmModule.cpp
@@ +435,5 @@
>      MutableBytes bytecode = js_new<ShareableBytes>();
>      if (!bytecode || !bytecode->bytes.initLengthUninitialized(bytecodeSize))
>          return nullptr;
>  
> +    if (bytecodeSize > 0)

I think that in this case the bytecode is always non zero, can you MOZ_ASSERT(bytecodeSize) instead? (and make sure this passes testing)

::: js/src/wasm/WasmSerialize.h
@@ +30,5 @@
>  static inline uint8_t*
>  WriteBytes(uint8_t* dst, const void* src, size_t nbytes)
>  {
> +    if (nbytes > 0)
> +        memcpy(dst, src, nbytes);

For this one too, I'd think nbytes is always non-zero, can you check by MOZ_ASSERTing? (otherwise fine to have the check)

@@ +38,5 @@
>  static inline const uint8_t*
>  ReadBytes(const uint8_t* src, void* dst, size_t nbytes)
>  {
> +    if (nbytes > 0)
> +        memcpy(dst, src, nbytes);

ditto

Comment 4

[André Bargull [:anba]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> ::: js/src/builtin/SIMD.cpp
> @@ +755,5 @@
> >  };
> >  template<typename T>
> >  struct Neg {
> > +    using MaybeUnsignedT = typename detail::MaybeMakeUnsigned<T>::Type;
> > +    static T apply(T x) { return -1 * MaybeUnsignedT(x); }
> 
> Will -1 be coerced to an unsigned value in this case, or will the RHS be
> converted to a signed (thus cancelling the fix)? Could we explicitly coerce
> the LHS to unsigned, to remove any doubt?
> 

It'll be coerced to unsigned.


> ::: js/src/wasm/WasmIonCompile.cpp
> @@ +993,5 @@
> >              curBlock_->add(mir);
> >              return call->stackArgs_.append(mir);
> >            }
> > +          case ABIArg::Uninitialized:
> > +            MOZ_ASSERT_UNREACHABLE("Uninitialized ABIArg kind");
> 
> Weren't we told to use MOZ_MAKE_COMPILER_ASSUME_UNREACHABLE in this kind of
> situation? (I think it's ok since in the worst case this falls through to
> the MOZ_CRASH anyways)

I just didn't know there's also MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE. Do you think it makes sense to replace the added MOZ_CRASH for uninitialized ABIArg kinds from this patch to MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE?


> 
> ::: js/src/wasm/WasmModule.cpp
> @@ +435,5 @@
> >      MutableBytes bytecode = js_new<ShareableBytes>();
> >      if (!bytecode || !bytecode->bytes.initLengthUninitialized(bytecodeSize))
> >          return nullptr;
> >  
> > +    if (bytecodeSize > 0)
> 
> I think that in this case the bytecode is always non zero, can you
> MOZ_ASSERT(bytecodeSize) instead? (and make sure this passes testing)

UBSan only prints warnings for definitely executed code, so at least in September this code was reachable with both arguments for |memcpy| being |nullptr|. From the UBSan output:

/home/andre/hg/mozilla-inbound/js/src/wasm/WasmModule.cpp:457:65: runtime error: null pointer passed as argument 1, which is declared to never be null
/home/andre/hg/mozilla-inbound/js/src/wasm/WasmModule.cpp:457:65: runtime error: null pointer passed as argument 2, which is declared to never be null

But I can re-test this case and the other two.

Comment 5

[André Bargull [:anba]]

(In reply to André Bargull [:anba] from comment #4)
> But I can re-test this case and the other two.

Using assertions for any of the three checks causes failures for these test cases:

    js/src/jit-test/tests/asm.js/testCaching.js
    js/src/jit-test/tests/asm.js/testCloning.js
    js/src/jit-test/tests/asm.js/testHeapAccess.js
    js/src/jit-test/tests/asm.js/testBullet.js
    js/src/jit-test/tests/asm.js/testSource.js
    js/src/jit-test/tests/asm.js/testStackWalking.js
    js/src/jit-test/tests/latin1/asm.js

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to André Bargull [:anba] from comment #5)
> (In reply to André Bargull [:anba] from comment #4)
> > But I can re-test this case and the other two.
> 
> Using assertions for any of the three checks causes failures for these test
> cases:
> 
>     js/src/jit-test/tests/asm.js/testCaching.js
>     js/src/jit-test/tests/asm.js/testCloning.js
>     js/src/jit-test/tests/asm.js/testHeapAccess.js
>     js/src/jit-test/tests/asm.js/testBullet.js
>     js/src/jit-test/tests/asm.js/testSource.js
>     js/src/jit-test/tests/asm.js/testStackWalking.js
>     js/src/jit-test/tests/latin1/asm.js

Alright, thanks for testing!

> I just didn't know there's also MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE. Do you think it makes sense to replace the added MOZ_CRASH for uninitialized ABIArg kinds from this patch to MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE?

I think it's better to have MOZ_CRASH, because as far as I remember, the other one can still silently be reached in optimized builds. Thanks!

Comment 7

[André Bargull [:anba]]

Attachment: bug1404251.patch

Updated patch to address review comments. Carrying r+ from :bbouvier.

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d904fa1e7ba6229d505057cde27a21d1a5d2bfad

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d6b80ba61d0
Fix various UBSan issues in SpiderMonkey. r=bbouvier

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4d6b80ba61d0