Njs: Null-pointer dereference in nxt_lvlhsh_find nxt/nxt_lvlhsh.c:178

[wrauner]

Minified testcase:

Object((new Date()).toJSON())+0

njs version:
changeset: 965:e0fdef4eb478

Stack trace (from ASAN):

==3586==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x555ab2061ccd bp 0xbebebebebebebebe sp 0x7ffc2bb79b40 T0)
==3586==The signal is caused by a READ memory access.
==3586==Hint: address points to the zero page.
    #0 0x555ab2061ccc in nxt_lvlhsh_find nxt/nxt_lvlhsh.c:178
    #1 0x555ab20ed6ab in njs_object_property njs/njs_object.c:249
    #2 0x555ab207a626 in njs_primitive_value njs/njs_vm.c:2943
    #3 0x555ab207b107 in njs_vmcode_addition_primitive njs/njs_vm.c:2746
    #4 0x555ab207cd47 in njs_vmcode_interpreter njs/njs_vm.c:159
    #5 0x555ab2077b83 in njs_vm_start njs/njs.c:594
    #6 0x555ab205acc3 in njs_process_script njs/njs_shell.c:770
    #7 0x555ab2053f48 in njs_process_file njs/njs_shell.c:619
    #8 0x555ab2053f48 in main njs/njs_shell.c:281
    #9 0x7fb00d825b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #10 0x555ab2055ef9 in _start (/home/build/njs/build/njs+0x2aef9)

Found by fluff

[hongzhidao]

@xeioex

Simplified.

var a = new Array(1, 2);
function foo() {
        console.log(arguments[0]);     // now, wrong output with 1
}
foo(a.toString());

test.js:foo
00000 ARGUMENTS         0004
00024 METHOD FRAME      1B46F20 1B51500 1
00064 PROPERTY GET      0002 0004 1B51510
00104 FUNCTION CALL     0014
00128 RETURN            1B51520
test.js:main
00000 FUNCTION FRAME    0011 2 CTOR
00032 MOVE              0002 1B514E0
00064 MOVE              0012 1B514F0
00096 FUNCTION CALL     0141
00120 FUNCTION FRAME    0151 1
00152 METHOD FRAME      0141 1B51530 0
00192 FUNCTION CALL     0002     // a.toString() should set value in 0002, but not.
00216 FUNCTION CALL     0161
00240 STOP              0161



md5-9a8e24dc316f0a7557efbe0c57a76139



static njs_ret_t
njs_array_prototype_to_string_continuation(njs_vm_t *vm, njs_value_t *args,
    nxt_uint_t nargs, njs_index_t retval)
{
    /* Skip retval update. */
    vm->top_frame->skip = 1;

    return NXT_OK;
}

njs_ret_t
njs_function_native_call(njs_vm_t *vm, njs_function_native_t native,
    njs_value_t *args, uint8_t *args_types, nxt_uint_t nargs,
    njs_index_t retval, u_char *return_address)
{
        ...
        if (!frame->skip) {
            value = njs_vmcode_operand(vm, retval);
            /*
             * GC: value external/internal++ depending
             * on vm->retval and retval type
             */
            *value = vm->retval;
        }
        ...
}

Now it happens in array and date, their reason is the same.
It seems njs_function_apply is OK, but I'm not sure how to fix it.
It's a litter similar to the commit: https://github.com/nginx/njs/commit/6fb662600f028c2207ba7925523037f92cf1807a

[hongzhidao]

deleted.

[hongzhidao]

@xeioex @igorsysoev take a look again.

Fixed Array.toString() and Date.toJSON().
https://gist.github.com/hongzhidao/34170e9275e114f74f571989b6bae86f

It also fixed the other issues #163 #164 .

[xeioex]

@hongzhidao

+            /* Skip the "join" method frame. */
+            vm->top_frame->previous->skip = 1;

BTW, don't we skipping "toString" method frame here? After njs_function_apply() the current frame becames "previous".

[hongzhidao]

@xeioex

don't we skipping "toString" method frame here?

No. See this. The key point is the argument value is set by toString, not join.

njs_date_prototype_to_json_continuation(njs_vm_t *vm, njs_value_t *args,
     nxt_uint_t nargs, njs_index_t retval)
 {
-    /* Skip retval update. */
-    vm->top_frame->skip = 1;
-
     return NXT_OK;
 }

After njs_function_apply() the current frame becames "previous".

var a = new Array(1, 2);
function foo() {
    console.log(arguments[0]); 
}
foo(a.toString()); 

We call a.toString and pass it as foo argument, see (i).
a.toString: frame1
join: frame2 => I think this frame should be skip, it only generate value vm->retval.

test.js:foo
00000 ARGUMENTS         0004
00024 METHOD FRAME      1B46F20 1B51500 1
00064 PROPERTY GET      0002 0004 1B51510
00104 FUNCTION CALL     0014
00128 RETURN            1B51520
test.js:main
00000 FUNCTION FRAME    0011 2 CTOR
00032 MOVE              0002 1B514E0
00064 MOVE              0012 1B514F0
00096 FUNCTION CALL     0141
00120 FUNCTION FRAME    0151 1
00152 METHOD FRAME      0141 1B51530 0
00192 FUNCTION CALL     0002     // (i)
00216 FUNCTION CALL     0161
00240 STOP              0161

[xeioex]

@hongzhidao

don't we skipping "toString" method frame here?
No. See this. The key point is the argument value is set by toString, not join.

I am sorry, didn't get it.

interactive njs 0.3.3

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> Object((new Date(0)).toJSON())+0

Breakpoint 1, njs_date_prototype_to_json (vm=0x61f00000ee80, args=0x619000002bb0, nargs=1, retval=2) at njs/njs_date.c:1907
1907    {

(gdb) p *vm->top_frame->function.u.native
$8 = {njs_ret_t (njs_vm_t *, njs_value_t *, nxt_uint_t, njs_index_t)} 0x471369 <njs_date_prototype_to_json>
..
1923                ret = njs_function_apply(vm, prop->value.data.u.function,
(gdb) n

(gdb) p *vm->top_frame->function->u.native 
$11 = {njs_ret_t (njs_vm_t *, njs_value_t *, nxt_uint_t, njs_index_t)} 0x46c68d <njs_date_prototype_to_iso_string>
(gdb) p *vm->top_frame->previous->function->u.native  // 
$12 = {njs_ret_t (njs_vm_t *, njs_value_t *, nxt_uint_t, njs_index_t)} 0x471369 <njs_date_prototype_to_json>

so vm->top_frame->previous points to njs_date_prototype_to_json (the current function),
*vm->top_frame points to applied function frame.

That is why, I think we have to change the commentary:
Skip the "toISOString" method frame. -> Skip the "toJSON" method frame.

[xeioex]

@hongzhidao

Take a look: https://gist.github.com/xeioex/57a27d560dbf1142c9b04650b1a25d13

[hongzhidao]

@xeioex

You are correct, looks much better.

[xeioex]

@hongzhidao

even a bit cleaner

+    nxt_int_t  ret;
+
+    /*
+     * njs_function_apply() allocs a new function frame,
+     * in order to preserve the retval of a new function and ignore
+     * retval of the current function during stack unwinding
+     * skip flag is needed.
+     */
+    vm->top_frame->skip = 1;
+
+    ret = njs_function_apply(vm, function, args, nargs, retval);
+    if (nxt_slow_path(ret == NXT_ERROR)) {
+        return ret;
+    }
+
+    return NJS_APPLIED;

[hongzhidao]

@xeioex

I thought about it, it's better to put skip setting after calling njs_function_apply successfully.
Well, but both seem OK since skip condition is only used (ret == NXT_OK)

[xeioex]

Duplicated of #166.