From c9477440e0021a669dd2abe95741b864efb4d51d Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Fri, 9 Sep 2022 04:41:29 +0000 Subject: [PATCH 1/2] buffer: revert GetBackingStore optimization in API 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: https://github.com/nodejs/node/pull/44080 Fixes: https://github.com/nodejs/node/issues/44554 --- src/node_buffer.cc | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index eb8e541c68635d..00d8493a7955ad 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -244,7 +244,20 @@ bool HasInstance(Local obj) { char* Data(Local val) { CHECK(val->IsArrayBufferView()); Local ui = val.As(); - return static_cast(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(ui->Buffer()->GetBackingStore()->Data()) + + + ui->ByteOffset(); } From e385eef07fc67fc1e6c31e4a153ed27fd2018c66 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Wed, 14 Sep 2022 05:30:22 +0000 Subject: [PATCH 2/2] Fix format, update comment --- src/node_buffer.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 00d8493a7955ad..b96b39a8c31d72 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -250,13 +250,11 @@ char* Data(Local val) { // 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. + // needs to take a performance hit because of this edge-case, so this change + // is only being backported to older Node.js releases. // // See: https://github.com/nodejs/node/issues/44554 return static_cast(ui->Buffer()->GetBackingStore()->Data()) + - ui->ByteOffset(); }