Guidance on internals of `v8::ArrayBuffer::Data()` method

[Aapo Alasuutari]

Hello

I'm trying to take a look at the `v8::ArrayBuffer::Data()` method with the intention of fixing this bug: https://bugs.chromium.org/p/v8/issues/detail?id=13488 (and by extension possibly unblock https://bugs.chromium.org/p/v8/issues/detail?id=13489)

Put it short: The method returns a null pointer for all zero-length buffers even when the ArrayBuffer is internally backed by an external pointer. These sorts of externally backed zero-length buffers are sometimes used in eg. Node API to pass opaque pointers to and from JavaScript. Getting the proper pointer requires using the `v8::ArrayBuffer::GetBackingStore()` API, after which its `Data()` API returns the real external pointer.

I've been trying to track where this difference springs from but haven't had much success. The `Data()` method calls the `backing_store()` method of a `i::Handle<i::JSArrayBuffer>` which I _think_ is defined with the `DEF_GETTER` macro in `js-array-buffer-inl.h` line 48:

DEF_GETTER(JSArrayBuffer, backing_store, void*) {
  Address value = ReadSandboxedPointerField(kBackingStoreOffset, cage_base);
  return reinterpret_cast<void*>(value);
}

But here I get confused: `ReadSandboxedPointerField` (in `sandboxed-pointer-inl.h`) seems simple enough that there should be absolutely no checks against the length of the buffer, nor does it seem particularly likely for the backing store offset parameter to be somehow wrong.

If anyone has an idea of where I should look into, that'd be much appreciated

-Aapo Alasuutari

[Clemens Backes]

This is the place where we store the special "empty backing store buffer" in the ArrayBuffer if the passed BackingStore is empty:

https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-array-buffer.cc;l=82;drc=57bf7660f3e50a0f68f329059f0dff8f641effc4

In a non-sandbox build, this will just store nullptr.

That said, I can't tell you why we have this optimization and which part(s) of the system depend on that. As the backing store is kept alive by the ArrayBuffer anyway, I guess we could also just store the actual buffer's start in the ArrayBuffer::backing_store field.

Waiting to be corrected :) 

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/350ede0e-0c6d-433e-b9b3-a85525c7049fn%40googlegroups.com.

--

Clemens Backes

Software Engineer

clem...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian   

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Aapo Alasuutari]

Thank you for the response!

Hopefully this was then much easier than I even expected. I opened this CL: https://groups.google.com/g/v8-dev/c/wuncGizO1EU

Unfortunately I'm not a dry-runner so I cannot start try-bots on this myself. I'll try running some tests locally at least.

-Aapo

[Clemens Backes]

I guess you meant to link to https://crrev.com/c/4896678. I triggered dry-runs, let's see what happens.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/25a7a109-5b44-4250-b5f2-bc060ef307b4n%40googlegroups.com.

[Aapo Alasuutari]

Oops, yeah.

Locally I already have related failed DCHECKs so this ain't gonna be pretty.

# Fatal error in ../../src/sandbox/sandboxed-pointer-inl.h, line 35
# Check failed: GetProcessWideSandbox()->Contains(pointer).
#
#
#
#FailureMessage Object: 0x7fad9b7e54f8
==== C stack trace ===============================

    /sources/aapoalas/v8/v8/out/x64.debug/libv8_libbase.so(v8::base::debug::StackTrace::StackTrace()+0x1e) [0x7fae87a1ef0e]
    /sources/aapoalas/v8/v8/out/x64.debug/libv8_libplatform.so(+0x5219d) [0x7fae8797419d]
    /sources/aapoalas/v8/v8/out/x64.debug/libv8_libbase.so(V8_Fatal(char const*, int, char const*, ...)+0x1ac) [0x7fae879eeeec]
    /sources/aapoalas/v8/v8/out/x64.debug/cctest(v8::internal::WriteSandboxedPointerField(unsigned long, v8::internal::PtrComprCageBase, unsigned long)+0x59) [0x5561832b6a49]
    /sources/aapoalas/v8/v8/out/x64.debug/libv8_for_testing.so(v8::internal::HeapObject::WriteSandboxedPointerField(unsigned long, v8::internal::Isolate*, unsigned long)+0x47) [0x7fae8ce2eee7]
    /sources/aapoalas/v8/v8/out/x64.debug/libv8_for_testing.so(v8::internal::JSArrayBuffer::set_backing_store(v8::internal::Isolate*, void*)+0x40) [0x7fae8da717d0]
    /sources/aapoalas/v8/v8/out/x64.debug/libv8_for_testing.so(v8::internal::JSArrayBuffer::Attach(std::__Cr::shared_ptr<v8::internal::BackingStore>)+0x45c) [0x7fae8da6e81c]

[Aapo Alasuutari]

It seem I got a clean bill of health locally after fixing the obvious bug I had:

>>> Running tests for x64.debug
>>> Running with test processors
[70:10|%  96|+ 18516|-   0]: Done                                            
>>> 19139 base tests produced 18516 (96%) non-filtered tests
>>> 18516 tests ran