bind() segfault.

[xeioex]

From #106 (comment)

function foo() {
    var t = 2;

    function baz() {
        t = 3;
    }

    baz.bind()()
}

foo();

./build/njs bind_bug.js                                                                                                       
AddressSanitizer:DEADLYSIGNAL                                                                                                   
=================================================================                                                                
==2195==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004c6c12 bp 0x7ffec1e80c70 sp 0x7ffec1e80418 T0
)                                                                                                                             
==2195==The signal is caused by a READ memory access.                                                                            
==2195==Hint: address points to the zero page.                                                                             
    #0 0x4c6c11 in __asan::QuickCheckForUnpoisonedRegion(unsigned long, unsigned long) (/home/xeioex/workspace/nginx/nginScript/n
js/build/njs+0x4c6c11)                                                                                                          
    #1 0x4c6b71 in __asan_memcpy (/home/xeioex/workspace/nginx/nginScript/njs/build/njs+0x4c6b71)                                
    #2 0x515973 in njs_vmcode_interpreter /home/xeioex/workspace/nginx/nginScript/njs/njs/njs_vm.c:176:27                      
    #3 0x513db4 in njs_vm_start /home/xeioex/workspace/nginx/nginScript/njs/njs/njs.c:594:11                                     
...

[hongzhidao]

@xeioex

Take a look.

diff -r 1bce636aed3d njs/njs_function.c
--- a/njs/njs_function.c	Sun Mar 31 22:59:04 2019 +0800
+++ b/njs/njs_function.c	Sat Apr 06 17:05:32 2019 +0800
@@ -83,7 +83,7 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return copy;
     }

-    copy->closure = 1;
+    copy->closure = 1; /* It seems wrong. */

     n = 0;

@@ -434,6 +434,10 @@ njs_function_lambda_call(njs_vm_t *vm, n
     n = 0;
     nesting = lambda->nesting;

+    /*
+     * explain why do we need function->closure?
+     * what's the diffenence of function->closures and vm->active_frame->closures?
+     */
     if (nesting != 0) {
         closures = (function->closure) ? function->closures
                                        : vm->active_frame->closures;

Another place.

static njs_function_t *
njs_parser_function_alloc(njs_vm_t *vm, njs_parser_t *parser,
    njs_variable_t *var)
{
    njs_value_t            *value;
    njs_function_t         *function;
    njs_function_lambda_t  *lambda;

    lambda = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_function_lambda_t));
    if (nxt_slow_path(lambda == NULL)) {
        njs_memory_error(vm);
        return NULL;
    }

    /* TODO:
     *  njs_function_t is used to pass lambda to
     *  njs_generate_function_declaration() and is not actually needed.
     *  real njs_function_t is created by njs_vmcode_function() in runtime.
     */
    /* So it still need more work? */

    function = njs_function_alloc(vm, lambda, NULL, 1);
    if (nxt_slow_path(function == NULL)) {
        return NULL;
    }

[hongzhidao]

This bug also happens in expression function.

function foo() {
    var a;
    (function() {
        a = 1;
    }).bind()()
}
foo();

[hongzhidao]

@xeioex

Review the patch, please.

# HG changeset patch
# User hongzhidao <hongzhidao@gmail.com>
# Date 1554566949 -28800
# Node ID 824c384406bf93708d78fd5d3981f0858e4bb01d
# Parent  7dba758fda64edab90d80a7b44f4a29fc3ce5f91
Fixed bind() segfault.

diff -r 7dba758fda64 -r 824c384406bf njs/njs_function.c
--- a/njs/njs_function.c	Fri Apr 05 17:49:22 2019 +0300
+++ b/njs/njs_function.c	Sun Apr 07 00:09:09 2019 +0800
@@ -8,6 +8,8 @@
 #include <string.h>


+static njs_function_t *njs_function_copy(njs_vm_t *vm,
+    njs_function_t *function);
 static njs_native_frame_t *njs_function_frame_alloc(njs_vm_t *vm, size_t size);
 static njs_ret_t njs_normalize_args(njs_vm_t *vm, njs_value_t *args,
     uint8_t *args_types, nxt_uint_t nargs);
@@ -71,8 +73,6 @@ fail:
 njs_function_t *
 njs_function_value_copy(njs_vm_t *vm, njs_value_t *value)
 {
-    size_t          size;
-    nxt_uint_t      n, nesting;
     njs_function_t  *function, *copy;

     function = value->data.u.function;
@@ -81,6 +81,25 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return function;
     }

+    copy = njs_function_copy(vm, function);
+    if (nxt_slow_path(copy == NULL)) {
+        njs_memory_error(vm);
+        return NULL;
+    }
+
+    value->data.u.function = copy;
+
+    return copy;
+}
+
+
+static njs_function_t *
+njs_function_copy(njs_vm_t *vm, njs_function_t *function)
+{
+    size_t          size;
+    nxt_uint_t      n, nesting;
+    njs_function_t  *copy;
+
     nesting = (function->native) ? 0 : function->u.lambda->nesting;

     size = sizeof(njs_function_t) + nesting * sizeof(njs_closure_t *);
@@ -91,8 +110,6 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return NULL;
     }

-    value->data.u.function = copy;
-
     *copy = *function;
     copy->object.__proto__ = &vm->prototypes[NJS_PROTOTYPE_FUNCTION].object;
     copy->object.shared = 0;
@@ -1050,18 +1067,12 @@ njs_function_prototype_bind(njs_vm_t *vm
         return NXT_ERROR;
     }

-    function = nxt_mp_alloc(vm->mem_pool, sizeof(njs_function_t));
+    function = njs_function_copy(vm, args[0].data.u.function);
     if (nxt_slow_path(function == NULL)) {
         njs_memory_error(vm);
         return NXT_ERROR;
     }

-    *function = *args[0].data.u.function;
-
-    function->object.__proto__ = &vm->prototypes[NJS_PROTOTYPE_FUNCTION].object;
-    function->object.shared = 0;
-    function->object.extensible = 1;
-
     if (nargs == 1) {
         args = (njs_value_t *) &njs_value_undefined;

diff -r 7dba758fda64 -r 824c384406bf njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c	Fri Apr 05 17:49:22 2019 +0300
+++ b/njs/test/njs_unit_test.c	Sun Apr 07 00:09:09 2019 +0800
@@ -6356,6 +6356,12 @@ static njs_unit_test_t  njs_test[] =
                  "var b = f.bind('1', '2', '3'); b.apply()"),
       nxt_string("123") },

+    { nxt_string("function f() { var a; return (function() { a = 1; return a; }).bind()() } f()"),
+      nxt_string("1") },
+
+    { nxt_string("function f() { var a; function baz() { a = 1; return a; } return baz.bind()(); } f()"),
+      nxt_string("1") },
+
     { nxt_string("var obj = {prop:'abc'}; "
                  "var func = function(x) { "
                  "    return this === obj && x === 1 && arguments[0] === 1 "

@drsm Welcome to test. Thanks again.

[xeioex]

@hongzhidao

+    /*
+     * explain why do we need function->closure?
+     * what's the diffenence of function->closures and vm->active_frame->closures?
+     */

good question, I also need to dig in.

/* So it still need more work? */

I think so. But it can be improved later, not a problem now.

The patch looks good. Committed, thanks!

[igorsysoev]

 * explain why do we need function->closure?
 * what's the diffenence of function->closures and vm->active_frame->closures?

vm->active_frame->closures is array of parent functions scopes that are referenced in nested functions.
function->closure means that the function has closures (function->closures) captured on function creation.

When parent function calls nested function, the nested function should use current active frame closures. When parent function creates closure from nested function (by returning it or by assigning it to variable or property) the nested function should capture current active frame closures. And when the nested closure function will later be called after parent function will complete, the nested closure function should use the captured closures.

[hongzhidao]

@xeioex @drsm

function foo() {
    var t;

    function baz() {
        return t;
    }

    return baz;
}

foo().bind()();

Not completely fixed. On it.

[hongzhidao]

@igorsysoev @xeioex

Review again.

# HG changeset patch
# User hongzhidao <hongzhidao@gmail.com>
# Date 1554615484 -28800
# Node ID 049800e92ef6fb0b216c2a5351031b0eab5680d5
# Parent  84cb61f23b7e3075dd21ff0ead228157bc0dd0e9
Fixed njs_function_copy().

diff -r 84cb61f23b7e -r 049800e92ef6 njs/njs_function.c
--- a/njs/njs_function.c	Sun Apr 07 00:09:09 2019 +0800
+++ b/njs/njs_function.c	Sun Apr 07 13:38:04 2019 +0800
@@ -9,7 +9,7 @@


 static njs_function_t *njs_function_copy(njs_vm_t *vm,
-    const njs_function_t *function);
+    const njs_function_t *function, njs_closure_t *closures[]);
 static njs_native_frame_t *njs_function_frame_alloc(njs_vm_t *vm, size_t size);
 static njs_ret_t njs_normalize_args(njs_vm_t *vm, njs_value_t *args,
     uint8_t *args_types, nxt_uint_t nargs);
@@ -81,7 +81,7 @@ njs_function_value_copy(njs_vm_t *vm, nj
         return function;
     }

-    copy = njs_function_copy(vm, function);
+    copy = njs_function_copy(vm, function, vm->active_frame->closures);
     if (nxt_slow_path(copy == NULL)) {
         njs_memory_error(vm);
         return NULL;
@@ -94,7 +94,8 @@ njs_function_value_copy(njs_vm_t *vm, nj


 static njs_function_t *
-njs_function_copy(njs_vm_t *vm, const njs_function_t *function)
+njs_function_copy(njs_vm_t *vm, const njs_function_t *function,
+    njs_closure_t *closures[])
 {
     size_t          size;
     nxt_uint_t      n, nesting;
@@ -106,7 +107,6 @@ njs_function_copy(njs_vm_t *vm, const nj

     copy = nxt_mp_alloc(vm->mem_pool, size);
     if (nxt_slow_path(copy == NULL)) {
-        njs_memory_error(vm);
         return NULL;
     }

@@ -124,7 +124,7 @@ njs_function_copy(njs_vm_t *vm, const nj

     do {
         /* GC: retain closure. */
-        copy->closures[n] = vm->active_frame->closures[n];
+        copy->closures[n] = closures[n];
         n++;
     } while (n < nesting);

@@ -1060,14 +1060,16 @@ njs_function_prototype_bind(njs_vm_t *vm
 {
     size_t          size;
     njs_value_t     *values;
-    njs_function_t  *function;
+    njs_function_t  *src, *function;

     if (!njs_is_function(&args[0])) {
         njs_type_error(vm, "\"this\" argument is not a function");
         return NXT_ERROR;
     }

-    function = njs_function_copy(vm, args[0].data.u.function);
+    src = args[0].data.u.function;
+
+    function = njs_function_copy(vm, src, src->closures);
     if (nxt_slow_path(function == NULL)) {
         njs_memory_error(vm);
         return NXT_ERROR;
diff -r 84cb61f23b7e -r 049800e92ef6 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c	Sun Apr 07 00:09:09 2019 +0800
+++ b/njs/test/njs_unit_test.c	Sun Apr 07 13:38:04 2019 +0800
@@ -6362,6 +6362,12 @@ static njs_unit_test_t  njs_test[] =
     { nxt_string("function f() { var a; function baz() { a = 1; return a; } return baz.bind()(); } f()"),
       nxt_string("1") },

+    { nxt_string("(function(){ var a = 1; return (function() { return a; })})().bind()()"),
+      nxt_string("1") },
+
+    { nxt_string("function f() { var a = 1; function baz() { return a; } return baz; } f().bind()()"),
+      nxt_string("1") },
+
     { nxt_string("function f(a, b) { return a + b }"
                  "f(3,4) === f.bind()(3,4)"),
       nxt_string("true") },

1. I think it's right that copy closures from function->closures in bind.
2. I'm not sure what closures should be used in OBJECT COPY, is function->closures or vm->active_frame->closures?

BTW, this also fixes the below issue happened in arrow function.

#if 0
    { nxt_string("(function() { return (() => this); })"
                 ".call('abc').bind('bca')()"),
      nxt_string("abc") },
#endif

[xeioex]

@hongzhidao

I think it's right that copy closures from function->closures in bind.

agree.

I'm not sure what closures should be used in OBJECT COPY, is function->closures or vm->active_frame->closures

I looks like to me, it should also be function->closures, but the following test fails for example (if we replace vm->active_frame->closures -> function->closures):

> function f(a) { function g(b) { return a + b } return g }var k = f('a'); k('b')
'ab'

But, maybe it is related to another issue.

[igorsysoev]

I think it's right that copy closures from function->closures in bind.
agree.

As I understand using nested function in bind() should create JS closure. So:

copy_closures = (original_function->closure) ? original_function->closures : vm->active_frame->closures

[igorsysoev]

I looks like to me, it should also be function->closures, but the following test fails for example

No, it should depend on function->closure flag. If it is set, then use function->closures, otherwise vm->active->frame->closures.

[hongzhidao]

@igorsysoev @xeioex

Another issue (coredump).

function f(a) { var g; function g(b) { return a + b } return g }
var k = f('a');
k('b')

typedef struct njs_generator_patch_s   njs_generator_patch_t;
@@ -536,7 +537,11 @@ njs_generate_name(njs_vm_t *vm, njs_gene
         return NXT_ERROR;
     }

-    if (var->type == NJS_VARIABLE_FUNCTION) {
+    /*
+     * Easily making var->type not NJS_VARIABLE_FUNCTION
+     * var f;
+     */
+    if (var->type == NJS_VARIABLE_FUNCTION) {

         node->index = njs_generate_dest_index(vm, generator, node);

It seems the design of variable (including function declare) need to be improved.

BTW, we can hoist function declaration if necessary like import statement.
My thoughts.

1. Don't set variable any value at parser phase, even at the generation phase. Only allowed at runtime phase.
2. function foo() {} => var foo = anonymous function expression().

[hongzhidao]

I have a question about hoist in JS.

It says hoist happens in variable and function declaration.
In njs, as I understand, variable declartion doesn't generate code.
So I think we only consider function declaration hoist if we want to introduce hoist in njs.

@drsm can you explain hoist, that you are good at JS?