|author||Leonard Chan <email@example.com>||Wed Dec 09 11:40:05 2020 -0800|
|committer||Leonard Chan <firstname.lastname@example.org>||Thu Dec 10 21:13:47 2020 +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
Downloaded and updated manually from the QuickJS homepage.