Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflow in fs.readdirSync causes fatal error #18645

Closed
joliss opened this issue Feb 8, 2018 · 2 comments
Closed

Stack overflow in fs.readdirSync causes fatal error #18645

joliss opened this issue Feb 8, 2018 · 2 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@joliss
Copy link
Contributor

joliss commented Feb 8, 2018

  • Version: v9.5.0
  • Platform: Linux sujin 4.4.0-101-generic # 124-Ubuntu SMP Fri Nov 10 18:29:59 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

The following code causes node to abort with FATAL ERROR on my system, when I would expect it to throw a "RangeError: Maximum call stack size exceeded" instead.

let fs = require("fs");
function f() {
  fs.readdirSync(".");
  f();
}
f();
$ node readdir-sync-bug.js
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [node]
 2: 0x8c807c [node]
 3: v8::Utils::ReportApiFailure(char const*, char const*) [node]
 4: 0x8fe7b6 [node]
 5: 0xf835c887147
Aborted (core dumped)

Running Node with GDB suggests that the problem is happening here:

#5  0x00000000008fe7b6 in node::ReadDir(v8::FunctionCallbackInfo<v8::Value> const&) ()
GDB output
$ gdb --args node readdir-sync-bug.js
GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
...
Reading symbols from node...done.
(gdb) run
Starting program: /home/ubuntu/.nvm/versions/node/v9.5.0/bin/node readdir-sync-bug.js
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff6b42700 (LWP 123862)]
[New Thread 0x7ffff6341700 (LWP 123863)]
[New Thread 0x7ffff5b40700 (LWP 123864)]
[New Thread 0x7ffff533f700 (LWP 123865)]
[New Thread 0x7ffff7ff5700 (LWP 123869)]
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [/home/ubuntu/.nvm/versions/node/v9.5.0/bin/node]
 2: 0x8c807c [/home/ubuntu/.nvm/versions/node/v9.5.0/bin/node]
 3: v8::Utils::ReportApiFailure(char const*, char const*) [/home/ubuntu/.nvm/versions/node/v9.5.0/bin/node]
 4: 0x8fe7b6 [/home/ubuntu/.nvm/versions/node/v9.5.0/bin/node]
 5: 0x2e361c207147

Thread 1 "node" received signal SIGABRT, Aborted.
0x00007ffff6b78428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff6b78428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff6b7a02a in __GI_abort () at abort.c:89
#2  0x00000000008c8041 in node::Abort() ()
#3  0x00000000008c807c in node::OnFatalError(char const*, char const*) ()
#4  0x0000000000a36085 in v8::Utils::ReportApiFailure(char const*, char const*) ()
#5  0x00000000008fe7b6 in node::ReadDir(v8::FunctionCallbackInfo<v8::Value> const&) ()
#6  0x00002e361c207147 in ?? ()
#7  0x00007ffffff07818 in ?? ()
#8  0x00007ffffff07860 in ?? ()
#9  0x0000000000000002 in ?? ()
#10 0x0000000002233b50 in ?? ()
#11 0x00002e361c207021 in ?? ()
#12 0x00007ffffff077d0 in ?? ()
#13 0x0000000000000006 in ?? ()
#14 0x00007ffffff078a0 in ?? ()
#15 0x00002e361c2075c1 in ?? ()
#16 0x0000020b35d02201 in ?? ()
#17 0x00000000021cd8e0 in ?? ()
#18 0x00001263620822d1 in ?? ()
#19 0x00001263620822d1 in ?? ()
#20 0x00000adadb40c5d1 in ?? ()
#21 0x00002e00b2d5e111 in ?? ()
#22 0x0000020b35d02239 in ?? ()
#23 0x00001263620822d1 in ?? ()
#24 0x00001263620822d1 in ?? ()
#25 0x00001263620834b1 in ?? ()
#26 0x0000020b35d02201 in ?? ()
#27 0x0000020b35d02201 in ?? ()
#28 0x00001263620822d1 in ?? ()
#29 0x0000000000000000 in ?? ()
(gdb)
@joyeecheung
Copy link
Member

joyeecheung commented Feb 8, 2018

It's caused by the function returned bypush_values_to_array_function being called and then ToLocalChecked in C++ right away. I have a fix for readdirSync, but note that there are other synchronous APIs using that function this way and should be updated as well.

@joyeecheung joyeecheung added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. labels Feb 8, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Feb 10, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: nodejs#18647
Fixes: nodejs#18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: #18647
Fixes: #18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: #18647
Fixes: #18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: #18647
Fixes: #18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: #18647
Fixes: #18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this issue Apr 13, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: #18647
Fixes: #18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Previously, fs.readdirSync calls the function returned by
env->push_values_to_array_function() in batch and check the returned
Maybe right away in C++, which can lead to assertions if the call stack
already reaches the maximum size. This patch fixes that by returning
early the call fails so the stack overflow error will be properly
thrown into JS land.

PR-URL: nodejs#18647
Fixes: nodejs#18645
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@littlekeith
Copy link

If I got hundred of thousands files in directory ?use thread with readdirSync?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants