Skip to content

Commit

Permalink
fs: fix stack overflow in fs.readdirSync
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
joyeecheung authored and gibfahn committed Apr 13, 2018
1 parent 973488b commit 2995506
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 3 deletions.
12 changes: 9 additions & 3 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -951,14 +951,20 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
name_v[name_idx++] = filename.ToLocalChecked();

if (name_idx >= arraysize(name_v)) {
fn->Call(env->context(), names, name_idx, name_v)
.ToLocalChecked();
MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx,
name_v);
if (ret.IsEmpty()) {
return;
}
name_idx = 0;
}
}

if (name_idx > 0) {
fn->Call(env->context(), names, name_idx, name_v).ToLocalChecked();
MaybeLocal<Value> ret = fn->Call(env->context(), names, name_idx, name_v);
if (ret.IsEmpty()) {
return;
}
}

args.GetReturnValue().Set(names);
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-fs-readdir-stack-overflow.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

const common = require('../common');

const fs = require('fs');

function recurse() {
fs.readdirSync('.');
recurse();
}

common.expectsError(
() => recurse(),
{
type: RangeError,
message: 'Maximum call stack size exceeded'
}
);

0 comments on commit 2995506

Please sign in to comment.