[v14.x] Backport environment teardown Node-API reference double free fixes #37802
Conversation
gabrielschulhof
added
c++
gabrielschulhof
changed the title
gabrielschulhof
force-pushed
the
backport-teardown-gc-ref-fix-to-v14.x
branch
from
9083454
to
e31b2e3
Compare
There was a problem hiding this comment.
I think we should figure out nodejs/node-addon-api#906 before this lands.
|
@legendecas can you verify that this fixes #37236? @mhdawson if so, we may want to land it for that alone. |
|
@gabrielschulhof nodejs/node-addon-api#906 has landed so as long as the 2 land together we should be good to go. EDIT: The PR that landed was: #37876 and we should make sure that it lands along with this one in 14.x (ie either both or neither) |
|
@gabrielschulhof I tried to cherry-picked @mhdawson's fix and ran with debug build and get segv rarely with stack: and the data of frame 2: Not sure why it is crashing.. Will try to dig into it to see if the problem is related to the one we are resolving. |
|
@legendecas @mhdawson I have now cherry-picked #37876 on top. Please re-review! @legendecas does @mhdawson's fix prevent the crash you are seeing? Please let us know if this additional patch fixes your issue, and whether this issue occurs in production for you. We need to know because there are currently no v14.x releases planned so, if this is an important fix, we may need to arrange for an additional v14.x release. |
|
@gabrielschulhof So I find out that v8 doesn't clear the second pass callback if the weakness was cleared (see https://github.com/nodejs/node/blob/master/deps/v8/src/handles/global-handles.cc#L532). The fix in the PR itself is self-contained, however, v8 does call into invalidated callbacks (https://github.com/nodejs/node/blob/master/deps/v8/src/handles/global-handles.cc#L1387) as the callback was not been cleared. Maybe we need to file a fix to v8 first. |
| // Thus, we don't want any stray gc passes to trigger a second call to | ||
| // `Finalize()`, so let's reset the persistent here if nothing is | ||
| // keeping it alive. | ||
| if (is_env_teardown && _persistent.IsWeak()) { |
There was a problem hiding this comment.
After digging into v8::PersistentBase and the second pass callbacks of GlobalHandles, I find out that _persistent.IsWeak() is always false after the first pass weak callback (in which we reset the persistent).
So this fix ultimately doesn't seem to be applied to the issue at all 😵 , so sorry for not picking this up earlier.
Anyway, the Persistent handle has to be reset on the first pass weak callback, thus the persistent no longer holds an address value of the global handle -- and unable to cancel the second pass callback by any means after the first pass callback. So here we have to ensure that the parameters of the weak info of second pass callbacks been kept alive until the second pass callbacks have been invoked.
I've submitted an issue to https://bugs.chromium.org/p/v8/issues/detail?id=11608. However, while I'm thinking that in the nature of the Persistent, it is possible that we can split the weak parameter lifetime from the v8impl::Reference. Trying to sum up a fix so that we can determine if the approach is acceptable.
MylesBorins
force-pushed
the
v14.x-staging
branch
from
11505ad
to
6b115d7
Compare
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
A gc may happen during environment teardown. Thus, during finalization initiated by environment teardown we must remove the V8 finalizer before calling the Node-API finalizer. Fixes: nodejs#37236 PR-URL: nodejs#37616 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#37876 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
targos
force-pushed
the
backport-teardown-gc-ref-fix-to-v14.x
branch
from
fecc3f4
to
180c33d
Compare
|
Landed in 5a3e12b...8413759 |
The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Backport-PR-URL: #37802 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
A gc may happen during environment teardown. Thus, during finalization initiated by environment teardown we must remove the V8 finalizer before calling the Node-API finalizer. Fixes: #37236 PR-URL: #37616 Backport-PR-URL: #37802 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Refs: nodejs/node-addon-api#906 Refs: #37616 Fix crash introduced by #37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #37876 Backport-PR-URL: #37802 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
These two fixes are necessary in order to prevent an issue on v14.x whereby a gc happens during environment shutdown causing a double free of a Node-API reference.