Skip to content

Port bellard/quickjs@fcbf5ea to fix BJSON array serialization - #1748

Open
gengjiawen wants to merge 1 commit into
quickjs-ng:masterfrom
gengjiawen:port-bjson-array-fcbf5ea
Open

gengjiawen wants to merge 1 commit into
quickjs-ng:masterfrom
gengjiawen:port-bjson-array-fcbf5ea

Conversation

@gengjiawen

Copy link
Copy Markdown
Contributor

JS_WriteArray() reads the elements with JS_GetPropertyUint32(), which runs
arbitrary JS when an element is an accessor. The array is borrowed from the
enclosing object -- JS_WriteObjectTag() passes p->prop[i].u.value without
taking a reference -- so a getter that drops the last reference to the array
frees it in the middle of its own serialization:

import * as bjson from "qjs:bjson";
const holder = { a: [1, 2, 3] };
Object.defineProperty(holder.a, 0, {
    get() { delete holder.a; return 5; },
    enumerable: true, configurable: true
});
bjson.write(holder);   // segfault
ERROR: AddressSanitizer: heap-use-after-free READ of size 2
    #0 js_get_fast_array_element
    #1 JS_GetPropertyUint32
    #2 JS_WriteArray
    #3 JS_WriteObjectRec
    #4 JS_WriteObjectTag
    #5 js_bjson_write
freed by: free_object <- free_zero_refcount

The loop keeps reading the freed JSObject, and JS_WriteObjectRec() clears
->tmp_mark on it afterwards.

Upstream fixed this in bellard/quickjs@fcbf5ea ("fixed BJSON array
serialization (#457)"): write the own enumerable value properties directly, the
way JS_WriteObjectTag() already does for plain objects, and throw a TypeError
for accessors. No JS runs during the serialization, so nothing can free the
object under the writer.

os.Worker's postMessage() uses the same serializer (JS_WriteObject2() with
JS_WRITE_OBJ_REFERENCE), so it was affected as well; there the freed pointer
also ends up in the object reference list.

Behaviour changes

case before after
hole, with Array.prototype[1] = "x" "x" undefined
accessor element the getter is called TypeError
non-enumerable element the value is written undefined

The last two are what already happens for the properties of a plain object.

Difference from upstream

The fast-array loop stops at min_uint32(len, p->u.array.count), so the number
of elements written can never exceed the length written to the stream. I could
not construct a case where count > len; it is cheap insurance for the reader.

Tests

  • upstream's two hole tests;
  • the getter above (segfaults on master, TypeError with this change);
  • a slow array with a hole in the middle, and a non-enumerable element;
  • a template object round-tripped through WRITE_OBJ_BYTECODE -- the
    is_template branch and its non-enumerable raw property had no coverage,
    and this change rewrites both.

tests.conf is 116/116, api-test passes, and tests/test_bjson.js plus
tests/test_worker.js are clean under ASAN.

JS_WriteArray() read the elements with JS_GetPropertyUint32(), which runs
arbitrary JS when an element is an accessor. The array is borrowed from the
enclosing object, so a getter that drops the last reference to it frees the
array in the middle of its own serialization; the loop then keeps reading the
freed JSObject, and JS_WriteObjectRec() clears ->tmp_mark on it afterwards:

    import * as bjson from "qjs:bjson";
    const holder = { a: [1, 2, 3] };
    Object.defineProperty(holder.a, 0, {
        get() { delete holder.a; return 5; },
        enumerable: true, configurable: true
    });
    bjson.write(holder);

    ERROR: AddressSanitizer: heap-use-after-free READ of size 2
        #0 js_get_fast_array_element
        #1 JS_GetPropertyUint32
        quickjs-ng#2 JS_WriteArray
        quickjs-ng#3 JS_WriteObjectRec
        quickjs-ng#4 JS_WriteObjectTag
        quickjs-ng#5 js_bjson_write
    freed by: free_object <- free_zero_refcount

Write the own enumerable value properties directly instead, the way
JS_WriteObjectTag() already does for plain objects, and throw a TypeError for
accessors. No JS runs during the serialization, so nothing can free the object.

Elements that are missing or non-enumerable are now written as undefined, which
means holes no longer resolve through the prototype chain.

os.Worker's postMessage() uses the same serializer and was affected as well.

Co-authored-by: Fabrice Bellard <fabrice@bellard.org>

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't an easier fix doing js_dup(obj) before the JS_GetPropertyUint32 calls, then JS_FreeValue(ctx, obj) afterwards?

Overriding array elements with getters isn't common but given the choice between properties getting lost in transit or not, I prefer 'not'. I suppose the counterargument is that getters turn into regular properties during round-tripping now, but that still seems better.

It's nice this PR writes out fast arrays more efficiently but that can be a separate PR (and it's already plenty fast, it's not a bottleneck.)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants