Skip to content

src: warn about FastOneByteString invalidation #59275

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

Closed
wants to merge 1 commit into from
Closed

src: warn about FastOneByteString invalidation #59275

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,14 @@ uint32_t FastWriteString(Local<Value> receiver,
uint32_t max_length,
// NOLINTNEXTLINE(runtime/references)
FastApiCallbackOptions& options) {
// Just a heads up... this is a v8 fast api function. The use of
// FastOneByteString has some caveats. Specifically, a GC occurring
// between the time the FastOneByteString is created and the time
// we use it below can cause the FastOneByteString to become invalid
// and produce garbage data. This is not a problem here because we
// are not performing any allocations or other operations that would
// trigger a GC before the FastOneByteString is used. Take care when
// modifying this code to ensure that no operations would trigger a GC.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do something like DebugSealHandleScope, i.e. actually enforce this constraint through code? E.g. a DebugDisallowJavascriptExecutionScope that defers to V8's DisallowJavascriptExecutionScope? (That may not be 100% equivalent to disallowing GC, but maybe it's close enough?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly? My knowledge of DebugSealHandleScope is super rusty tho so I'm not entirely sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My knowledge of DebugSealHandleScope is super rusty tho so I'm not entirely sure.

The point is just that it's possible to at least partially implement something that mechanically prevents GC execution, which is typically better than a comment 🙂 The analogy to DebugSealHandleScope would be that if an explicit DisallowJavascriptExecutionScope is too expensive (it might be?), we could create a debug-only variant of it similar to how DebugSealHandleScope is the debug-only variant of SealHandleScope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a great idea... just would take me a bit of time to page back in the knowledge to do that ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now, let's get this warning comment landed and we can work on the scope impl separately?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Not a blocker by any means, it just seems simpler that way

HandleScope handle_scope(options.isolate);
SPREAD_BUFFER_ARG(dst_obj, dst);
CHECK(offset <= dst_length);
Expand Down
Loading