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.

Sign up for GitHub

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

Jump to bottom

fs: fix stack overflow in fs.readdirSync #18647

Closed
wants to merge 1 commit into from
Closed

fs: fix stack overflow in fs.readdirSync #18647

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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.

Fixes: #18645

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@joyeecheung
fs: fix stack overflow in fs.readdirSync
29faa41 
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.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Feb 8, 2018
@joyeecheung
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13039/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018
@joyeecheung @BridgeAR
fs: fix stack overflow in fs.readdirSync
b4ed550 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 10, 2018 •
edited
Loading

Landed in c5c9515 🎉

@BridgeAR BridgeAR closed this Feb 10, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 10, 2018
@joyeecheung @BridgeAR
fs: fix stack overflow in fs.readdirSync
c5c9515 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@joyeecheung @MylesBorins
fs: fix stack overflow in fs.readdirSync
cd1f35d 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@joyeecheung @MylesBorins
fs: fix stack overflow in fs.readdirSync
d2c9557 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@joyeecheung @MylesBorins
fs: fix stack overflow in fs.readdirSync
f19bd9e 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@joyeecheung @MylesBorins
fs: fix stack overflow in fs.readdirSync
34f4934 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
@joyeecheung @gibfahn
fs: fix stack overflow in fs.readdirSync
2995506 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@joyeecheung @MayaLekova
fs: fix stack overflow in fs.readdirSync
ccb1cdf 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stack overflow in fs.readdirSync causes fatal error
7 participants
@joyeecheung @BridgeAR @jasnell @addaleax @cjihrig @gibfahn @nodejs-github-bot