buffer: revert GetBackingStore optimization in API

[kvakil]

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: #44080
Fixes: #44554

[devsnek]

maybe hot take: only land this commit on 18.x and keep the optimization moving forward.

[mscdex]

I agree. I don't think it is worth giving up this optimization going forward for such an edge case.

[tniessen]

Apparently these two are not equivalent in the case of zero-length buffers: the former returns a "valid" address, while the latter returns NULL.

IMHO returning a nullptr is reasonable. Any pointer, even NULL, points to a memory region spanning 0 bytes.

I am also for targeting Node.js 18 with this PR and for keeping the change on the main branch.

Similar discussion: #44543

[tniessen]

@nodejs/releasers Should we change the base branch of this PR to v18.x-staging?

[targos]

Yes, if you want to keep the original change on main

[tniessen]

@kvakil I took the liberty of re-targeting the PR for v18.x. If you disagree with this decision, you can change the base branch to main again and force-push 5ca238a.

[tniessen]

Suggested change

	returnstatic_cast<char*>(ui->Buffer()->GetBackingStore()->Data()) +
	ui->ByteOffset();
	returnstatic_cast<char*>(ui->Buffer()->GetBackingStore()->Data()) +
	ui->ByteOffset();

(Not sure what indentation will make format-cpp happy, though.)

[kvakil]

Thanks all. I'm fine with the change only landing on the release branch. I fixed the spurious newline which snuck in somehow.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46572/

[aapoalas]

Hello from Deno! I did some checking into this in the Deno codebase, specifically to do with the FFI / C API there. I can confirm that the same issue occurs there as well. However, we have an extra wrinkle to this, which necessitates opening an issue to track and fix this bug / feature.

The Deno FFI API uses the V8 Fast API for binding foreign C functions into JavaScript functions. The API offers ArrayBuffers / ArrayBufferViews to the native C function as a struct with a data pointer value and a length. Effectively, we cannot choose how we get the data pointer in these calls, V8 will do it for us. And of course it turns out that V8 internally seems to be using the same or similar code as Buffer()->Data() is. Our Fast API FFI calls get null pointers from foreign-backed zero length ArrayBuffers.

If you already have a V8 issue open about this, I would like a link. Otherwise, I'll make one shortly.

[aapoalas]

https://bugs.chromium.org/p/v8/issues/detail?id=13488

[aapoalas]

This should now be fixed in the most recent V8 versions.

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.