buffer,n-api: release external buffers from BackingStore callback #33321
Conversation
In some situations, it can be useful to use threadsafe callbacks on an `Environment` to perform cleanup operations that should run even when the process would otherwise be ending.
Release `Buffer` and `ArrayBuffer` instances that were created through our addon APIs and have finalizers attached to them only after V8 has called the deleter callback passed to the `BackingStore`, instead of relying on our own GC callback(s). This fixes the following race condition: 1. Addon code allocates pointer P via `malloc`. 2. P is passed into `napi_create_external_buffer` with a finalization callback which calls `free(P)`. P is inserted into V8’s global array buffer table for tracking. 3. The finalization callback is executed on GC. P is freed and returned to the allocator. P is not yet removed from V8’s global array buffer table. (!) 4. Addon code attempts to allocate memory once again. The allocator returns P, as it is now available. 5. P is passed into `napi_create_external_buffer`. P still has not been removed from the v8 global array buffer table. 6. The world ends with `Check failed: result.second`. Since our API contract is to call the finalizer on the JS thread on which the `ArrayBuffer` was created, but V8 may call the `BackingStore` deleter callback on another thread, fixing this requires posting a task back to the JS thread. Refs: nodejs#32463 (comment) Fixes: nodejs#32463
addaleax
added
buffer
nodejs-github-bot
added
c++
|
Updated the test because timing for |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/31263/ (:green_heart:) |
|
This fixes my use case (mmap/munmap), thanks. |
|
Landed in c1ee70e |
Release `Buffer` and `ArrayBuffer` instances that were created through our addon APIs and have finalizers attached to them only after V8 has called the deleter callback passed to the `BackingStore`, instead of relying on our own GC callback(s). This fixes the following race condition: 1. Addon code allocates pointer P via `malloc`. 2. P is passed into `napi_create_external_buffer` with a finalization callback which calls `free(P)`. P is inserted into V8’s global array buffer table for tracking. 3. The finalization callback is executed on GC. P is freed and returned to the allocator. P is not yet removed from V8’s global array buffer table. (!) 4. Addon code attempts to allocate memory once again. The allocator returns P, as it is now available. 5. P is passed into `napi_create_external_buffer`. P still has not been removed from the v8 global array buffer table. 6. The world ends with `Check failed: result.second`. Since our API contract is to call the finalizer on the JS thread on which the `ArrayBuffer` was created, but V8 may call the `BackingStore` deleter callback on another thread, fixing this requires posting a task back to the JS thread. Refs: #32463 (comment) Fixes: #32463 PR-URL: #33321 Reviewed-By: James M Snell <jasnell@gmail.com>
Release `Buffer` and `ArrayBuffer` instances that were created through our addon APIs and have finalizers attached to them only after V8 has called the deleter callback passed to the `BackingStore`, instead of relying on our own GC callback(s). This fixes the following race condition: 1. Addon code allocates pointer P via `malloc`. 2. P is passed into `napi_create_external_buffer` with a finalization callback which calls `free(P)`. P is inserted into V8’s global array buffer table for tracking. 3. The finalization callback is executed on GC. P is freed and returned to the allocator. P is not yet removed from V8’s global array buffer table. (!) 4. Addon code attempts to allocate memory once again. The allocator returns P, as it is now available. 5. P is passed into `napi_create_external_buffer`. P still has not been removed from the v8 global array buffer table. 6. The world ends with `Check failed: result.second`. Since our API contract is to call the finalizer on the JS thread on which the `ArrayBuffer` was created, but V8 may call the `BackingStore` deleter callback on another thread, fixing this requires posting a task back to the JS thread. Refs: #32463 (comment) Fixes: #32463 PR-URL: #33321 Reviewed-By: James M Snell <jasnell@gmail.com>
|
@addaleax I keep getting Any ideas? Get the crash wheather I using finalisers or not. (Update: Misread, I'm not getting the Check failed on create_external_buffer anymore but on get_buffer_info) |
|
@mafintosh I have a hard time following the If you use |
|
@addaleax its a bit of macros on macros I know 😅. That’s a good pointer! I’ll investigate that. The actual crash is coming from the get buffer info call know. Do you know on the top of your head, if that would crash if i made two external buffers from the same pointer? |
|
@mafintosh I don’t, no, sorry – I don’t quite know why it would fail at that point |
|
Ok, I’ll try to poke around it a bit and avoid the dual external buffer on the same pointer and see if it works |
|
@addaleax Danke! |
|
@addaleax I hope the reproducible failure I have filed with package ext2fs issue 76 can help find an even better correction of the underlying problem. |
|
@srguiwiz The problem can’t really be corrected in Node.js (without non-trivial overhead), so this is something to be addressed in the addons themselves, unfortunately. |
(The first commit is #33320, split out due to different backportability, hence the
blockedlabel.)Release
BufferandArrayBufferinstances that were created throughour addon APIs and have finalizers attached to them only after V8 has
called the deleter callback passed to the
BackingStore, instead ofrelying on our own GC callback(s).
This fixes the following race condition:
malloc.napi_create_external_bufferwith a finalizationcallback which calls
free(P). P is inserted into V8’s global arraybuffer table for tracking.
to the allocator. P is not yet removed from V8’s global array
buffer table. (!)
returns P, as it is now available.
napi_create_external_buffer. P still has not beenremoved from the v8 global array buffer table.
Check failed: result.second.Since our API contract is to call the finalizer on the JS thread on
which the
ArrayBufferwas created, but V8 may call theBackingStoredeleter callback on another thread, fixing this requires posting
a task back to the JS thread.
Refs: #32463 (comment)
Fixes: #32463
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes