tree 322d85f295c5c2280a39e6e4d9b98d046169b2b2
parent 69b041bc41fc8155eba9e6c9e86315b9c06e54c3
author Leonard Chan <leonardchan@google.com> 1607542805 -0800
committer Leonard Chan <leonardchan@google.com> 1607634827 +0000

[quickjs] Fix UBSan bugs

There were 3 UB issues (and one unique issue) when attempting to build
quickjs with UBSan:

1. A left shift into the sign bit of an int.
2. Passing a null pointer into the second argument of memcpy(), which is
   marked with the nonnull attribute.
3. Signed integer overflow when converting a double to an int32_t.
4. (Unique) qjs would throw a stack overflow exception if JS_CallInternal
   was instrumented with UBSan (adding
   __attribute__((no_sanitize("undefined"))) would silence it).

Solutions:

1. This only happens in a macro that l-shifts an enum value 24 places.
   Casting this to an unsigned value then performing the bitwise OR will
   circumvent the UB. We can then do a two's complement operation (via a
   macro I added) to effectively cast back to an int type.

   The macro itself just does some arithmetic to get the two's complement.
   We can't use type punning (via unions) to cast the unsigned to a
   signed since that makes assumptions about the underlying
   representation of a signed int. (To make it simpler, we can
   technically assume we're using a two's complement representation
   since Clang uses that representation for signed ints). I am able to
   assert that using this macro still results in the same codegen (with
   optimizations) as the previous code, but without UB.

2. (May not the best solution, but the code still seems to work.) Just
   check if the second operand is a nullptr before calling memcpy().

3. Instead implicitly convert the double to a uint32_t (which can hold
   the range of values this double represents), then use the two's
   complement macro in (1) to get a int32_t. I'm also able to assert the
   codegen is the same with optimizations.

4. I was able to trace down what triggers this exception to the dispatch
   table in JS_CallInternal. I don't exactly understand how, but somehow,
   instrumenting with UBSan on this particular table leads to either the
   JS runtime stack_top pointer or js_get_stack_pointer() to yield the
   wrong values, hitting the check that triggers the exception. As a
   workaround, we can instead just use a normal switch by setting
   DIRECT_DISPATCH to 0. This just instead opts for using switch-case
   over the table, which the compiler can just choose to replace with a
   table if it's more fitting. This doesn't change any functionality
   since it effectively does the same task.

This will require updating the flower afterwards.

Bug: 47041
Change-Id: Ifc9fc921de5542bec6516e14f3d6038409d2745f