Fix crash on failure to read bytecode (arguments, local variables, etc.)

[penneryu]

We have encountered crashes in production related to bytecode reading. When reading specific parts of the bytecode (such as arguments, local variables, and other related data) fails, the subsequent release of the corresponding object may trigger a null pointer crash.

static JSValue JS_ReadFunctionTag(BCReaderState *s)
{    
    bc_read_trace(s, "args=%d vars=%d defargs=%d closures=%d cpool=%d\n",
                  b->arg_count, b->var_count, b->defined_arg_count,
                  b->closure_var_count, b->cpool_count);
    bc_read_trace(s, "stack=%d bclen=%d locals=%d\n",
                  b->stack_size, b->byte_code_len, local_count);

    if (local_count != 0) {
        bc_read_trace(s, "vars {\n");
        bc_read_trace(s, "off flags scope name\n");
        for(i = 0; i < local_count; i++) {
            JSVarDef *vd = &b->vardefs[i];
            if (bc_get_atom(s, &vd->var_name))
                // If this jump to fail, the bytecode object obj.byte_code_len will already have a value, but obj.byte_code_buf will still be null. Jumping to JS_FreeValue will crash at free_function_bytecode.
                goto fail; 
            if (bc_get_leb128_int(s, &vd->scope_level))
                goto fail;
            if (bc_get_leb128_int(s, &vd->scope_next))
                goto fail;
            vd->scope_next--;
            if (bc_get_u8(s, &v8))

[saghul]

Any chance of adding a test for it?

[penneryu]

Any chance of adding a test for it?

It’s a bit tricky to add an automated test for this, since reproducing the corrupted bytecode scenario is not straightforward. From what we observed, our crashes seem to be caused by incomplete bytecode files that were downloaded or decompressed on the mobile device.

[bnoordhuis]

The change itself is fine but apropos this:

our crashes seem to be caused by incomplete bytecode files that were downloaded or decompressed on the mobile device

The on-disk bytecode format is not designed to withstand corruption or malice. If you're downloading stuff over the network, you need to put additional integrity checks in place (like download over https only, or checking the hash after downloading, etc.)

In short, you need to make sure you're actually executing what you think you're executing.

[aabbdev]

@penneryu It looks like you may have a durability issue. Consider adding a checksum to verify data integrity, or rely on a storage engine with WAL support (e.g. SQLite, RocksDB, LMDB).

[penneryu]

Thank you very much for all of your suggestions. I will add safety checks for the downloaded files. 🙏