Njs: Long parentheses string segfault.

[xeioex]

printf "%0.s(" {1..6546} | ./build/njs -
Segmentation fault

repeated {, [ and ` are also affected.

[HongxuChen]

BTW, except for (, {, [, files with long strings of ` can also cause the segfault.

[hongzhidao]

@xeioex

> printf "%0.s(" {1..6546} | ./build/njs
Shown, I tested in macOS and linux.
SyntaxError: Unexpected end of input in bug.js:1

[drsm]

@hongzhidao

I reproduced with a larger constant:

$ printf "%0.s(" {1..6546} | ./build/njs -
SyntaxError: Unexpected end of input in -:1
$ printf "%0.s(" {1..10000} | ./build/njs -
Segmentation fault
$ uname -a
Linux L7480 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
$ free -h
              total        used        free      shared  buff/cache   available
Mem:            15G        1.3G         10G        341M        3.3G         13G
Swap:          1.9G          0B        1.9G
$ printf "%0.s{" {1..40000} | ./build/njs -
SyntaxError: Unexpected end of input in -:1
$ printf "%0.s{" {1..45000} | ./build/njs -
Segmentation fault

[hongzhidao]

@xeioex @drsm

These tokens (, {, [, ` can make their corresponding parser function calling, kind of like recursion, finally, it causes stack-overflow. But I don't know how to fix them.

[xeioex]

@hongzhidao

The simplest solution is to count and limit recursion in parser to a reasonable limit. Like 128-512.
For example we can increment counter in njs_parser_statement().

[hongzhidao]

On it.

[hongzhidao]

@xeioex @drsm

take a look.

deleted.

My thoughts.

1. there are two places need to be limited: statement and terminal. And I don't want to break njs_parser_statement().
2. reuse parser->count in statement and terminal.
3. I tested (, {, [, it's easier to add tests in Function(...). But I can't reproduce the token `.

[drsm]

@hongzhidao

there is no consensus about type of error thrown:

root@node:~# eshost -x "(new Function('{'.repeat(2**5)))()" 
#### ch

SyntaxError: Expected '}'

#### jsc

SyntaxError: Unexpected end of script

#### sm

SyntaxError: missing } in compound statement:

#### v8

SyntaxError: Unexpected token ')'

#### xs

SyntaxError: invalid token )

root@node:~# eshost -x "(new Function('{'.repeat(2**20)))()"
#### ch

Error: Out of stack space

#### jsc

RangeError: Maximum call stack size exceeded.

#### sm

InternalError: too much recursion

#### v8

RangeError: Maximum call stack size exceeded

#### xs

I think it should be InternalError as the code may be valid, but we have no memory to parse it

[hongzhidao]

Updated.

deleted.

@xeioex It seems it's better to commit this patch with new Function patch.

[xeioex]

@drsm @hongzhidao

RangeError: Maximum call stack size exceeded.

I like this variant, because it what we already have for function recursion and this is what V8 does.

[drsm]

@hongzhidao

But I can't reproduce the token `.

 printf "%0.s\`" {1..90000} | ./build/njs -
Segmentation fault

(, {, [, {[, {; are fine. Thanks!

Also found:

$ printf "%0.s1;" {1..40000} | ./build/njs -
Segmentation fault

[hongzhidao]

$ printf "%0.s1;" {1..40000} | ./build/njs -
Segmentation fault

It seems it's easy to cause a problem like this way.
Do you think we need to fix them? (need to check all the places may call recursion.)

diff --git a/src/njs_parser.c b/src/njs_parser.c
--- a/src/njs_parser.c
+++ b/src/njs_parser.c
@@ -249,11 +249,20 @@ njs_parser_statement_chain(njs_vm_t *vm,

     last = *child;

+    if (njs_slow_path(parser->count > 512)) {
+        njs_range_error(vm, "Maximum call stack size exceeded");
+        return NJS_TOKEN_ERROR;
+    }
+
+    parser->count++;
+
     token = njs_parser_statement(vm, parser, token);
     if (njs_slow_path(token <= NJS_TOKEN_ILLEGAL)) {
         return njs_parser_unexpected_token(vm, parser, token);
     }

+    parser->count--;
+
     if (parser->node == NULL) {
         /* The statement is empty block or just semicolon. */
         return token;
@@ -411,6 +420,8 @@ njs_parser_statement(njs_vm_t *vm, njs_p
             return NJS_TOKEN_ILLEGAL;
         }
     }
+
+    parser->count--;
 }


diff --git a/src/njs_parser.h b/src/njs_parser.h
--- a/src/njs_parser.h
+++ b/src/njs_parser.h
@@ -73,6 +73,7 @@ struct njs_parser_s {
     njs_lexer_t                     *lexer;
     njs_parser_node_t               *node;
     njs_parser_scope_t              *scope;
+    njs_uint_t                      count;
 };

diff --git a/src/njs_parser_terminal.c b/src/njs_parser_terminal.c
--- a/src/njs_parser_terminal.c
+++ b/src/njs_parser_terminal.c
@@ -44,6 +44,13 @@ njs_parser_terminal(njs_vm_t *vm, njs_pa
     njs_int_t          ret;
     njs_parser_node_t  *node;

+    if (njs_slow_path(parser->count > 512)) {
+        njs_range_error(vm, "Maximum call stack size exceeded");
+        return NJS_TOKEN_ERROR;
+    }
+
+    parser->count++;
+
     ret = njs_parser_match_arrow_expression(vm, parser, token);
     if (ret == NJS_OK) {
         return njs_parser_arrow_expression(vm, parser, token);
@@ -197,6 +204,8 @@ njs_parser_terminal(njs_vm_t *vm, njs_pa

     parser->node = node;

+    parser->count--;
+
     return njs_parser_token(vm, parser);
 }

@@ -921,6 +930,13 @@ njs_parser_template_literal(njs_vm_t *vm
     njs_index_t        index;
     njs_parser_node_t  *node, *array;

+    if (njs_slow_path(parser->count > 512)) {
+        njs_range_error(vm, "Maximum call stack size exceeded");
+        return NJS_TOKEN_ERROR;
+    }
+
+    parser->count++;
+
     tagged_template = (parent->token != NJS_TOKEN_TEMPLATE_LITERAL);

     index = NJS_SCOPE_CALLEE_ARGUMENTS;
@@ -992,6 +1008,8 @@ njs_parser_template_literal(njs_vm_t *vm

         expression = !expression;
     }
+
+    parser->count--;
 }

[xeioex]

@hongzhidao

It seems it's easy to cause a problem like this way.

I think there are not many of them. But the with float numbers is a separate patch. Will do.

[hongzhidao]

@xeioex
Think about refactoring out two macros.

+    if (njs_slow_path(parser->count > 512)) {
+        njs_range_error(vm, "Maximum call stack size exceeded");
+        return NJS_TOKEN_ERROR;
+    }
+
+    parser->count++;

+    parser->count--;

[xeioex]

@hongzhidao

printf "%0.s\`" {1..90000} | ./build/njs - should be limited in generator not parser. On it.

./build/njs  -d
interactive njs 0.3.6

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

>> ```` // is a valid js expression == ""()
shell:main
00000 ARRAY             245A1A0 1
00032 PROP INIT         245A1C0 245A1A0 245A1B0
00064 TEMPLATE LITERAL  245A1A0
00080 FUNCTION FRAME    245A1A0 1
00112 ARRAY             0002 1
00144 PROP INIT         245A1C0 0002 245A1B0
00176 FUNCTION CALL     245A1D0
00192 STOP              245A1D0

TypeError: string is not a function
    at main (native)

>> 

the same:

printf 'a'`printf "%0.s()" {1..100000}` | ./build/njs -

[xeioex]

@hongzhidao @drsm

Take a look: https://gist.github.com/7df5712ed64a7186cf43d80c7fdf95fb
It should protect from any deep recursion in parser or generator. (printf "%0.s1;" {1..40000} | ./build/njs - is a separate issue)

The second issue with the original patch was:
count++ and count-- was not paired for all code branches, so count can became inconsistent (it tended to always to grow).

[hongzhidao]

@xeioex

It's very clear, I like it, except the naming.
What bout?

njs_parser_enter  -> njs_parser_use_add
njs_parser_exit   -> njs_parser_use_dec

Or njs_parser_count_xxx?

[xeioex]

@igorsysoev is OK with njs_parser_enter() and njs_parser_leave()