Skip to content
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

Jump to bottom

[18.8.0] char* node::Buffer::Data(v8::Local<v8::Object> val) returns a NULL pointer when given a valid v8::Object #44554

Open
gierschv opened this issue Sep 7, 2022 · 3 comments · May be fixed by #44579
Open

[18.8.0] char* node::Buffer::Data(v8::Local<v8::Object> val) returns a NULL pointer when given a valid v8::Object #44554

gierschv opened this issue Sep 7, 2022 · 3 comments · May be fixed by #44579

Comments

@gierschv
Copy link

gierschv commented Sep 7, 2022

Version

18.8.0

Platform

macOS Monterey 12.4

Subsystem

No response

What steps will reproduce the bug?

Try to initialise a lame Encoder using this lib : https://github.com/FlatIO/node-lame (check Example : https://github.com/FlatIO/node-lame#example)

How often does it reproduce? Is there a required condition?

Everytime

What is the expected behavior?

node::Buffer::Data() returns a valid pointer when a valid v8::Object is provided.
Then, the lame Encoder is initialized without any issue.

What do you see instead?

node::Buffer::Data() returns a NULL pointer.
The lame encoder returns this error: “error setting prop “channels”: -1"

Additional information

This was working fine with node v18.7.0 and all the previous versions so this is very probably an issue with Node.
I did some investigation and found out that it was caused by a NULL pointer returned by node::Buffer::Data() when provided with a seemingly valid v8::Object
I tested the v8::Object with ->IsObject(), it returned true, and with ->IsNullOrUndefined() and it returned false;
The v8::Object (GFP) represents Lame global flags, and node::Buffer::Data is used to unwrap the pointer after sending it through a JS binding
@targos
Copy link
Member

targos commented Sep 7, 2022

/cc @nodejs/cpp-reviewers @kvakil. This seems caused by 8c35a4e#diff-81f77f34c4f5b8cc35f1a506808b94e2a281f3341c0f1a38ef2b8a8ba730b22bL247 ?

@bnoordhuis
Copy link
Member

I suspect the problem is here: it calls Nan::NewBuffer() with length == 0.

Maybe that worked before but it's basically saying "this is an empty buffer" and those don't have to point to anything.

kvakil added a commit to kvakil/node that referenced this issue Sep 9, 2022
@kvakil
buffer: revert GetBackingStore optimization in API
8df27df 
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
@kvakil kvakil linked a pull request Sep 9, 2022 that will close this issue
Open
kvakil added a commit to kvakil/node that referenced this issue Sep 9, 2022
@kvakil
buffer: revert GetBackingStore optimization in API
5ca238a 
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
@kvakil
Copy link
Contributor

kvakil commented Sep 9, 2022

Thanks @targos for the bisect. I've confirmed Ben's comment that this is an edge case with zero-length buffers: if you bump the "length" in WrapPointer from 0 to 1, then everything works. I am a little ambivalent about fixing the issue, but a codesearch did reveal several repos which are using this pattern, so I think we should just revert.

A fix is available here; #44579

tniessen pushed a commit to kvakil/node that referenced this issue Sep 12, 2022
@kvakil @tniessen
buffer: revert GetBackingStore optimization in API
c947744 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

buffer: revert GetBackingStore optimization in API kvakil/node
4 participants
@bnoordhuis @gierschv @targos @kvakil