src: fix crash on FSReqPromise destructor by santigimeno · Pull Request #43533 · nodejs/node · GitHub

We are deciding whether to end fs promises by checking
can_call_into_js() whereas in the FSReqPromise destructor we're
using the is_stopping() check. Though this may look as semantically
correct it has issues because though both values are modified before
termination on Environment::ExitEnv() and both are atomic they are not
synchronized together so it may happen that when reaching the destructor
call_into_js may be set to false whereas is_stopping remains
false causing the crash. Fix this by using the same checks everywhere.

Fixes: #43499

/cc @addaleax as you already commented in the original PR. Do you think this is the correct approach or just use the can_call_into_js() everywhere? Thanks

Do you think this is the correct approach or just use the can_call_into_js() everywhere?

I’d turn the line in the destructor into CHECK_IMPLIES(!finished_, !env()->can_call_into_js());, yeah. During the original review I missed that this is about checking that the precondition for not finishing (which should be !env->can_call_into_js()) was fulfilled if the promise wasn’t finished, rather than being a standalone check that was unrelated to the other conditionals introduced in the original PR.

We are deciding whether to end `fs` promises by checking `can_call_into_js()` whereas in the `FSReqPromise` destructor we're using the `is_stopping()` check. Though this may look as semantically correct it has issues because though both values are modified before termination on `Environment::ExitEnv()` and both are atomic they are not syncronized together so it may happen that when reaching the destructor `call_into_js` may be set to `false` whereas `is_stopping` remains `false` causing the crash. Fix this by checking with `can_call_into_js()` also in the destructor. Fixes: nodejs#43499

@addaleax I've updated the PR with your suggestion. Thanks!!
While we're here, one question if you don't mind: I understand the difference in semantics between stopping and can_call_into_js but while looking at where and when they're set it's not completely clear to me why they can't be folded into one variable. Could you clarify it? Thanks in advance

I understand the difference in semantics between stopping and can_call_into_js but while looking at where and when they're set it's not completely clear to me why they can't be folded into one variable. Could you clarify it? Thanks in advance

I think that is potentially possible for sure, but as you mention, there is a difference in semantics. I could imagine us setting can_call_into_js to false temporarily if we introduce a DisallowJavascriptExecutionScope at some point, for example, but that shouldn’t affect is_stopping.

The Jenkins CI attempts to rebase before running the tests, so a manual rebase is not strictly necessary.

The Jenkins CI attempts to rebase before running the tests, so a manual rebase is not strictly necessary.

I dont know that. Thanks for the info. Then let us rerun the ci and see if ci is happy now.

Hi, collaborators. could you help to trigger another resume ci run?

The last ci failure is mainly caused by flaky test-stream-finished. This test is already fixed by #43641. we could trigger another ci run.

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

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

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

Landed in 92d26e7

We are deciding whether to end `fs` promises by checking `can_call_into_js()` whereas in the `FSReqPromise` destructor we're using the `is_stopping()` check. Though this may look as semantically correct it has issues because though both values are modified before termination on `Environment::ExitEnv()` and both are atomic they are not syncronized together so it may happen that when reaching the destructor `call_into_js` may be set to `false` whereas `is_stopping` remains `false` causing the crash. Fix this by checking with `can_call_into_js()` also in the destructor. Fixes: #43499 PR-URL: #43533 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

We are deciding whether to end `fs` promises by checking `can_call_into_js()` whereas in the `FSReqPromise` destructor we're using the `is_stopping()` check. Though this may look as semantically correct it has issues because though both values are modified before termination on `Environment::ExitEnv()` and both are atomic they are not syncronized together so it may happen that when reaching the destructor `call_into_js` may be set to `false` whereas `is_stopping` remains `false` causing the crash. Fix this by checking with `can_call_into_js()` also in the destructor. Fixes: nodejs/node#43499 PR-URL: nodejs/node#43533 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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. needs-ci PRs that need a full CI run. labels Jun 22, 2022

santigimeno force-pushed the santi/fix_43499 branch from 8f8e71a to 8310739 Compare June 22, 2022 17:47

santigimeno added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2022