Assertion Fail DUK_HOBJECT_E_GET_KEY(thr->heap, obj, i) != key in duk_hobject_props.c

[WilliamParks]

I found this using Fuzzilli, a Javascript engine fuzzer that I patched duktape to run on. I can share the setup if you're interested (requires code cleanup on my end, and a patch to Fuzzilli).

Compilation Target: dukd in v2-maintenance branch, commit 468c7b4

Source:

function main() {
    var v1 = [13.37,13.37,13.37];
    var v2 = [13.37];
    var v3 = {a:v2,length:v2};
    var v4 = v3;
    v4.__proto__ = v1;
    var v5 = v4.sort();
    for (var v6 in v5) {
    }
}
main();

Backtrace with -fsanitize=undefined and debug assertions enabled

*** FATAL ERROR: assertion failed: DUK_HOBJECT_E_GET_KEY(thr->heap, obj, i) != key (duk_hobject_props.c:1482)
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==12820==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000426bb9 bp 0x7ffe2d9414a0 sp 0x7ffe2d941450 T12820)
==12820==The signal is caused by a WRITE memory access.
==12820==Hint: address points to the zero page.
    #0 0x426bb8 in duk_default_fatal_handler (duktape/duk-fuzzilli+0x426bb8)
    #1 0x5bf4b8 in duk__hobject_alloc_entry_checked (duktape/duk-fuzzilli+0x5bf4b8)
    #2 0x6f2d1a in duk_hobject_putprop (duktape/duk-fuzzilli+0x6f2d1a)
    #3 0x4b582d in duk__put_prop_shared (duktape/duk-fuzzilli+0x4b582d)
    #4 0x4b565a in duk_put_prop (duktape/duk-fuzzilli+0x4b565a)
    #5 0x7f38f5 in duk__add_enum_key (duktape/duk-fuzzilli+0x7f38f5)
    #6 0x4e01a0 in duk_hobject_enumerator_create (duktape/duk-fuzzilli+0x4e01a0)
    #7 0xa54941 in duk__handle_op_initenum (duktape/duk-fuzzilli+0xa54941)
    #8 0xa2e17a in duk__js_execute_bytecode_inner (duktape/duk-fuzzilli+0xa2e17a)
    #9 0x8ea036 in duk_js_execute_bytecode (duktape/duk-fuzzilli+0x8ea036)
    #10 0x8ca495 in duk__handle_call_raw (duktape/duk-fuzzilli+0x8ca495)
    #11 0x43f9ca in duk_handle_call_unprotected (duktape/duk-fuzzilli+0x43f9ca)
    #12 0x43fe84 in duk_call_method (duktape/duk-fuzzilli+0x43fe84)
    #13 0xa986fc in wrapped_compile_execute (duktape/duk-fuzzilli+0xa986fc)
    #14 0xa84be7 in duk__handle_safe_call_inner (duktape/duk-fuzzilli+0xa84be7)
    #15 0x446e36 in duk_handle_safe_call (duktape/duk-fuzzilli+0x446e36)
    #16 0x4433f2 in duk_safe_call (duktape/duk-fuzzilli+0x4433f2)
    #17 0xa9743d in handle_fh (duktape/duk-fuzzilli+0xa9743d)
    #18 0xa97029 in handle_file (duktape/duk-fuzzilli+0xa97029)
    #19 0xa956c6 in main (duktape/duktape/duk-fuzzilli+0xa956c6)
    #20 0x7f74d1255b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #21 0x4042f9 in _start (duktape/duk-fuzzilli+0x4042f9)

UndefinedBehaviorSanitizer can not provide additional info.
==12820==ABORTING

[svaarala]

I can share the setup if you're interested (requires code cleanup on my end, and a patch to Fuzzilli).

I'd be very interested, it would be great to have some automatic fuzzing in the CI runs 👍

In case it matters, I'm fine with being able to run the fuzzing with Docker, which might simplify cleaning up the setup.

[svaarala]

The assert is triggered because property write code is trying to add a certain key as a new property table entry when the key already exists (which is obviously not supposed to happen). The offending key seems to be "0" when creating the enumerator. I'll have to dig deeper later.

[WilliamParks]

What I have currently is thrown together on top of both Fuzzilli and Duktape, and breaks standard usage of both. Cleaning it up should be straightforward, to where it's a new profile in Fuzzilli, and an additional make target in Duktape.

FYI, the two bugs here, and the two I emailed separately were found with 7 cores over 3 days, to give a sense of scale.

I'll make an issue to track, and get a couple design decisions from you, as well as make an issue to add the profile to Fuzzilli.

[svaarala]

Cleaning it up should be straightforward, to where it's a new profile in Fuzzilli, and an additional make target in Duktape.

Ok, sounds good!

FYI, the two bugs here, and the two I emailed separately were found with 7 cores over 3 days, to give a sense of scale.

Good to know - the best solution is probably then to have the setup easily executable from this repo so that anyone can run it if they wish. I can then run it offline with master, and before releases.

[svaarala]

Ok, I think I figured this out. When the enum code sorts an internal temporary object's keys, it's missing a rehash so the hash part becomes invalid. This matters if the enumeration involves inherited duplicate keys. I'll merge the fix tomorrow.