Skip to content

Commit

Permalink
buffer: revert GetBackingStore optimization in API
Browse files Browse the repository at this point in the history
In an earlier PR, I replaced a lot of instances of
`GetBackingStore()->Data()` with `Data()`. Apparently these two are not
equivalent in the case of zero-length buffers: the former returns a
"valid" address, while the latter returns `NULL`. At least one library
in the ecosystem (see the referenced issue) abuses zero-length buffers
to wrap arbitrary pointers, which is broken by this difference. It is
unfortunate that every library needs to take a performance hit because
of this edge-case, and somebody should figure out if this is actually a
reasonable contract to uphold long-term.

I have not traced down exactly why this divergence occurs, but I have
verified that reverting this line fixes the referenced issue.

Refs: nodejs#44080
Fixes: nodejs#44554
  • Loading branch information
kvakil authored and tniessen committed Sep 12, 2022
1 parent bf4d390 commit c947744
Showing 1 changed file with 14 additions and 1 deletion.
15 changes: 14 additions & 1 deletion src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,20 @@ bool HasInstance(Local<Object> obj) {
char* Data(Local<Value> val) {
CHECK(val->IsArrayBufferView());
Local<ArrayBufferView> ui = val.As<ArrayBufferView>();
return static_cast<char*>(ui->Buffer()->Data()) + ui->ByteOffset();
// GetBackingStore() is slow, and this would be faster if we just did
// ui->Buffer()->Data(). However these two are not equivalent in the case of
// zero-length buffers: the former returns a "valid" address, while the
// latter returns `NULL`. At least one library in the ecosystem (see the
// referenced issue) abuses zero-length buffers to wrap arbitrary pointers,
// which is broken by this difference. It is unfortunate that every library
// needs to take a performance hit because of this edge-case, and somebody
// should figure out if this is actually a reasonable contract to uphold
// long-term.
//
// See: https://github.com/nodejs/node/issues/44554
return static_cast<char*>(ui->Buffer()->GetBackingStore()->Data()) +

ui->ByteOffset();
}


Expand Down

0 comments on commit c947744

Please sign in to comment.