WIP: Refactor njs_continuation_t.

[hongzhidao]

Hi @xeioex

Take a look at these patches, please.

1. Refactored njs_function_lambda_call() and style.
   https://gist.github.com/hongzhidao/0a87adeb26e8b0eb71c7203d7a9d82b8

2. Refactor njs continuaction. (Optional)
   https://gist.github.com/hongzhidao/36b74142d3eb08b8d24509da6b543f91
   https://gist.github.com/hongzhidao/4e818cd152da82e1fc77f2f23b10dc59
   https://gist.github.com/hongzhidao/c23d90c783cafd6c16b6b2f270391495

Finally, we refactored out three important APIs.
njs_function_frame, njs_function_activate, njs_function_call

[xeioex]

Hi @hongzhidao,

For the future, please, add

showfunc = 1 

to your ~/.hgrc.

The hg patches, will additionally contain, the name of the changed functions.

[hongzhidao]

Hi @xeioex

Refactored codes related to function release.
https://gist.github.com/hongzhidao/449eeaefa2e9cb2c9f939a381fbecff8

Q. Does frame->skip only happen in "apply/call" method?

[xeioex]

Hi @hongzhidao,

Refactor njs continuaction. (Optional)
https://gist.github.com/hongzhidao/36b74142d3eb08b8d24509da6b543f91

continuation = nxt_mem_cache_alloc(vm->mem_cache_pool, continuation_size);

why do you think that it is a better idea to allocate memory for continuation from heap rather than from stack (as it is now)?

Having it on stack allows us to automatically free the memory for it as a stack unwinds. In you patch I see no nxt_mem_cache_free() invocation. I would leave it as is, on stack, if there is no compelling reasons to do otherwise.

[hongzhidao]

No problem.
Sorry for not been carefully thought out the patch of 'Refactor njs continuaction'.
The purpose is to make continuation more generic, but it's still an experiment.
I notice you if there's any change.

[hongzhidao]

Hi @xeioex

# HG changeset patch
# User hongzhidao <hongzhidao@gmail.com>
# Date 1547439608 -28800
# Node ID 4427aede6e950bc1f175ac53775037642ca28de3
# Parent  fb2c2bca61c2cfd787f1913b5d89707b846d5905
Fixed njs_vm_value_dump().

diff -r fb2c2bca61c2 -r 4427aede6e95 njs/njs_extern.c
--- a/njs/njs_extern.c	Fri Jan 11 19:20:38 2019 +0800
+++ b/njs/njs_extern.c	Mon Jan 14 12:20:08 2019 +0800
@@ -173,7 +173,7 @@ njs_vm_external_prototype(njs_vm_t *vm,

 nxt_int_t
 njs_vm_external_create(njs_vm_t *vm, njs_value_t *ext_val,
-    const njs_extern_t *proto,  njs_external_ptr_t object)
+    const njs_extern_t *proto, njs_external_ptr_t object)
 {
     void  *obj;

diff -r fb2c2bca61c2 -r 4427aede6e95 njs/njs_json.c
--- a/njs/njs_json.c	Fri Jan 11 19:20:38 2019 +0800
+++ b/njs/njs_json.c	Mon Jan 14 12:20:08 2019 +0800
@@ -2296,7 +2296,7 @@ njs_vm_value_dump(njs_vm_t *vm, nxt_str_
         goto exception;
     }

-    stringify = nxt_mem_cache_alloc(vm->mem_cache_pool,
+    stringify = nxt_mem_cache_zalloc(vm->mem_cache_pool,
                                     sizeof(njs_json_stringify_t));

     if (nxt_slow_path(stringify == NULL)) {

[xeioex]

Fixed njs_vm_value_dump().

Thank!

I am not sure about alloc -> zalloc change.

[hongzhidao]

Keep it for later. I found one more important issue.

Is it ok to add mallopt() for debuging?
If it is, it causes many coredumps.

diff -r 4427aede6e95 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c	Mon Jan 14 12:20:08 2019 +0800
+++ b/njs/test/njs_unit_test.c	Mon Jan 14 21:49:54 2019 +0800
@@ -12,6 +12,7 @@
 #include <stdio.h>
 #include <sys/resource.h>
 #include <time.h>
+#include <malloc.h>


 typedef struct {
@@ -11959,6 +11960,8 @@ main(int argc, char **argv)
     nxt_int_t   ret;
     nxt_bool_t  disassemble, verbose;

+    mallopt(M_PERTURB, 0x55);
+
     disassemble = 0;
     verbose = 0;

[xeioex]

@hongzhidao

it causes many coredumps.

Thank you for the report. Will look into it.

BTW, I usually do the following to test for memory issues:

make clean && CFLAGS='-O0 -DNXT_DEBUG_MEMORY=1 -fsanitize=address' ./configure && make test

[xeioex]

Is it ok to add mallopt() for debuging?

"This function is not specified by POSIX or the C standards."
I would rather not, because it is not a POSIX function, and there are other ways (non-invasive) to look for memory issues.

[xeioex]

@hongzhidao

I can confirm, that -fsanitize=address catches a bug.

==2034==ERROR: AddressSanitizer: heap-use-after-free on address 0x61900001effd at pc 0x0000004515fa bp 0x7fffc472cb50 sp 0x7fffc4
72cb40
READ of size 1 at 0x61900001effd thread T0
    #0 0x4515f9 in njs_function_native_call njs/njs_function.c:546
    #1 0x4122f1 in njs_vmcode_function_call njs/njs_vm.c:2042
    #2 0x40a070 in njs_vmcode_interpreter njs/njs_vm.c:152
    #3 0x40911b in njs_vm_start njs/njs.c:584
    #4 0x404067 in njs_process_script njs/njs_shell.c:603
    #5 0x4031f6 in njs_interactive_shell njs/njs_shell.c:401
    #6 0x40278a in main njs/njs_shell.c:226
    #7 0x7f4ec37f082f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #8 0x402518 in _start (/home/xeioex/workspace/nginx/nginScript/njs/build/njs+0x402518)

0x61900001effd is located 125 bytes inside of 1024-byte region [0x61900001ef80,0x61900001f380)
freed by thread T0 here:
    #0 0x7f4ec43e32ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)
    #1 0x40680a in njs_free njs/njs.c:48
    #2 0x499459 in nxt_mem_cache_free nxt/nxt_mem_cache_pool.c:672
    #3 0x451d3f in njs_function_frame_free njs/njs_function.c:708
    #4 0x4514b1 in njs_function_native_call njs/njs_function.c:531
    #5 0x4122f1 in njs_vmcode_function_call njs/njs_vm.c:2042
    #6 0x40a070 in njs_vmcode_interpreter njs/njs_vm.c:152
    #7 0x40911b in njs_vm_start njs/njs.c:584
    #8 0x404067 in njs_process_script njs/njs_shell.c:603
    #9 0x4031f6 in njs_interactive_shell njs/njs_shell.c:401
    #10 0x40278a in main njs/njs_shell.c:226
    #11 0x7f4ec37f082f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

previously allocated by thread T0 here:
    #0 0x7f4ec43e4076 in __interceptor_posix_memalign (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x99076)
    #1 0x497f11 in nxt_memalign nxt/nxt_malloc.c:26
    #2 0x4067ec in njs_align njs/njs.c:41
    #3 0x498e4b in nxt_mem_cache_alloc_large nxt/nxt_mem_cache_pool.c:600
    #4 0x498c70 in nxt_mem_cache_align nxt/nxt_mem_cache_pool.c:358
    #5 0x450955 in njs_function_frame_alloc njs/njs_function.c:372
    #6 0x44fe61 in njs_function_native_frame njs/njs_function.c:231
    #7 0x41152f in njs_function_frame njs/njs_function.h:187
    #8 0x41152f in njs_function_frame_create njs/njs_vm.c:1888
    #9 0x411f0c in njs_vmcode_method_frame njs/njs_vm.c:2003
    #10 0x40a070 in njs_vmcode_interpreter njs/njs_vm.c:152
    #11 0x40911b in njs_vm_start njs/njs.c:584
    #12 0x404067 in njs_process_script njs/njs_shell.c:603
    #13 0x4031f6 in njs_interactive_shell njs/njs_shell.c:401
    #14 0x40278a in main njs/njs_shell.c:226
    #15 0x7f4ec37f082f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

[hongzhidao]

It's introduced in the patch.
http://hg.nginx.org/njs/rev/045ba10db769

[hongzhidao]

Hi @xeioex

Reason: the variables frame->skip, cont are still be used though frame is freed.

diff -r 4427aede6e95 njs/njs_function.c
--- a/njs/njs_function.c	Mon Jan 14 12:20:08 2019 +0800
+++ b/njs/njs_function.c	Tue Jan 15 06:02:19 2019 +0800
@@ -528,7 +528,6 @@ njs_function_native_call(njs_vm_t *vm, n
         frame = vm->top_frame;

         vm->top_frame = njs_function_previous_frame(frame);
-        njs_function_frame_free(vm, frame);

         /*
          * If a retval is in a callee arguments scope it
@@ -552,6 +551,8 @@ njs_function_native_call(njs_vm_t *vm, n
             *value = vm->retval;
         }

+        njs_function_frame_free(vm, frame);
+
         return NXT_OK;
     }

diff -r 4427aede6e95 njs/njs_vm.c
--- a/njs/njs_vm.c	Mon Jan 14 12:20:08 2019 +0800
+++ b/njs/njs_vm.c	Tue Jan 15 06:02:19 2019 +0800
@@ -2287,12 +2287,14 @@ const njs_vmcode_generic_t  njs_continua
 static njs_ret_t
 njs_vmcode_continuation(njs_vm_t *vm, njs_value_t *invld1, njs_value_t *invld2)
 {
+    u_char              *return_address;
     njs_ret_t           ret;
     njs_native_frame_t  *frame;
     njs_continuation_t  *cont;

     frame = vm->top_frame;
     cont = njs_vm_continuation(vm);
+    return_address = cont->return_address;

     ret = njs_function_native_call(vm, cont->function, frame->arguments,
                                    cont->args_types, frame->nargs,
@@ -2300,7 +2302,7 @@ njs_vmcode_continuation(njs_vm_t *vm, nj

     switch (ret) {
     case NXT_OK:
-        vm->current = cont->return_address;
+        vm->current = return_address;
         /* Fall through. */

     case NJS_APPLIED:

Sorry for my negligence :)

[xeioex]

Sorry for my negligence :)

No problem. I missed it too :).

[hongzhidao]

I can't get useful information from fsanitize, can you share your experience in debugging these similar problems?