From 19b4834914334ee7916c8f648bb2c5c4f6f6eada Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Bargull?= Date: Wed, 24 Jan 2024 12:42:16 +0000 Subject: [PATCH] Bug 1872555: Transfer can only realloc when the jemalloc arena is known. r=sfink `ArrayBufferObject::copyAndDetachRealloc()` calls `ReallocateArrayBufferContents` to realloc the source buffer into `ArrayBufferContentsArena`. This is only valid when the source buffer was also created in `ArrayBufferContentsArena`. To track if the ArrayBuffer contents were allocated in `ArrayBufferContentsArena`, split `BufferKind::MALLOCED` into `MALLOCED_ARRAYBUFFER_CONTENTS_ARENA` and `MALLOCED_UNKNOWN_ARENA`. Differential Revision: https://phabricator.services.mozilla.com/D197492 --- .../arraybuffer-transfer-unknown-arena.js | 12 ++++ js/src/shell/js.cpp | 2 +- js/src/vm/ArrayBufferObject.cpp | 72 +++++++++---------- js/src/vm/ArrayBufferObject.h | 38 ++++++---- js/src/vm/StructuredClone.cpp | 8 ++- 5 files changed, 80 insertions(+), 52 deletions(-) create mode 100644 js/src/jit-test/tests/typedarray/arraybuffer-transfer-unknown-arena.js diff --git a/js/src/jit-test/tests/typedarray/arraybuffer-transfer-unknown-arena.js b/js/src/jit-test/tests/typedarray/arraybuffer-transfer-unknown-arena.js new file mode 100644 index 0000000000000..2d586913e9489 --- /dev/null +++ b/js/src/jit-test/tests/typedarray/arraybuffer-transfer-unknown-arena.js @@ -0,0 +1,12 @@ +const headerSize = 8; +const undefinedSize = 8; + +let source = serialize(undefined).arraybuffer +assertEq(source.detached, false); +assertEq(source.byteLength, headerSize + undefinedSize); + +let target = source.transfer(128); +assertEq(source.detached, true); +assertEq(source.byteLength, 0); +assertEq(target.detached, false); +assertEq(target.byteLength, 128); diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index 3561ed4ca85d3..deb459efe8b92 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -2405,7 +2405,7 @@ static bool CacheEntry_setBytecode(JSContext* cx, HandleObject cache, using BufferContents = ArrayBufferObject::BufferContents; - BufferContents contents = BufferContents::createMalloced(buffer); + BufferContents contents = BufferContents::createMallocedUnknownArena(buffer); Rooted arrayBuffer( cx, ArrayBufferObject::createForContents(cx, length, contents)); if (!arrayBuffer) { diff --git a/js/src/vm/ArrayBufferObject.cpp b/js/src/vm/ArrayBufferObject.cpp index f144c2bd1ddcb..19aa3549cde91 100644 --- a/js/src/vm/ArrayBufferObject.cpp +++ b/js/src/vm/ArrayBufferObject.cpp @@ -1105,7 +1105,8 @@ bool ArrayBufferObject::prepareForAsmJS() { "prior size checking should have excluded empty buffers"); switch (bufferKind()) { - case MALLOCED: + case MALLOCED_ARRAYBUFFER_CONTENTS_ARENA: + case MALLOCED_UNKNOWN_ARENA: case MAPPED: case EXTERNAL: // It's okay if this uselessly sets the flag a second time. @@ -1134,10 +1135,6 @@ bool ArrayBufferObject::prepareForAsmJS() { case WASM: MOZ_ASSERT(!isPreparedForAsmJS()); return false; - - case BAD1: - MOZ_ASSERT_UNREACHABLE("invalid bufferKind() encountered"); - return false; } MOZ_ASSERT_UNREACHABLE("non-exhaustive kind-handling switch?"); @@ -1173,7 +1170,8 @@ void ArrayBufferObject::releaseData(JS::GCContext* gcx) { case INLINE_DATA: // Inline data doesn't require releasing. break; - case MALLOCED: + case MALLOCED_ARRAYBUFFER_CONTENTS_ARENA: + case MALLOCED_UNKNOWN_ARENA: gcx->free_(this, dataPointer(), byteLength(), MemoryUse::ArrayBufferContents); break; @@ -1204,9 +1202,6 @@ void ArrayBufferObject::releaseData(JS::GCContext* gcx) { freeInfo()->freeFunc(dataPointer(), freeInfo()->freeUserData); } break; - case BAD1: - MOZ_CRASH("invalid BufferKind encountered"); - break; } } @@ -1226,10 +1221,10 @@ size_t ArrayBufferObject::byteLength() const { } inline size_t ArrayBufferObject::associatedBytes() const { - if (bufferKind() == MALLOCED) { + if (isMalloced()) { return byteLength(); } - if (bufferKind() == MAPPED) { + if (isMapped()) { return RoundUp(byteLength(), js::gc::SystemPageSize()); } MOZ_CRASH("Unexpected buffer kind"); @@ -1547,7 +1542,8 @@ ArrayBufferObject* ArrayBufferObject::createForContents( if (contents.kind() == MAPPED) { nAllocated = RoundUp(nbytes, js::gc::SystemPageSize()); } else { - MOZ_ASSERT(contents.kind() == MALLOCED, + MOZ_ASSERT(contents.kind() == MALLOCED_ARRAYBUFFER_CONTENTS_ARENA || + contents.kind() == MALLOCED_UNKNOWN_ARENA, "should have handled all possible callers' kinds"); } } @@ -1567,7 +1563,9 @@ ArrayBufferObject* ArrayBufferObject::createForContents( buffer->initialize(nbytes, contents); - if (contents.kind() == MAPPED || contents.kind() == MALLOCED) { + if (contents.kind() == MAPPED || + contents.kind() == MALLOCED_ARRAYBUFFER_CONTENTS_ARENA || + contents.kind() == MALLOCED_UNKNOWN_ARENA) { AddCellMemory(buffer, nAllocated, MemoryUse::ArrayBufferContents); } @@ -1615,7 +1613,8 @@ ArrayBufferObject::createBufferAndData( uint8_t* toFill; if (data) { toFill = data.release(); - buffer->initialize(nbytes, BufferContents::createMalloced(toFill)); + buffer->initialize( + nbytes, BufferContents::createMallocedArrayBufferContentsArena(toFill)); AddCellMemory(buffer, nbytes, MemoryUse::ArrayBufferContents); } else { auto contents = @@ -1674,11 +1673,14 @@ ArrayBufferObject::createBufferAndData( "caller must validate the byte count it passes"); if (newByteLength > ArrayBufferObject::MaxInlineBytes && - source->bufferKind() == ArrayBufferObject::MALLOCED) { + source->isMalloced()) { if (newByteLength == source->byteLength()) { return copyAndDetachSteal(cx, source); } - return copyAndDetachRealloc(cx, newByteLength, source); + if (source->bufferKind() == + ArrayBufferObject::MALLOCED_ARRAYBUFFER_CONTENTS_ARENA) { + return copyAndDetachRealloc(cx, newByteLength, source); + } } auto* newBuffer = ArrayBufferObject::copy(cx, newByteLength, source); @@ -1693,7 +1695,7 @@ ArrayBufferObject::createBufferAndData( /* static */ ArrayBufferObject* ArrayBufferObject::copyAndDetachSteal( JSContext* cx, JS::Handle source) { MOZ_ASSERT(!source->isDetached()); - MOZ_ASSERT(source->bufferKind() == MALLOCED); + MOZ_ASSERT(source->isMalloced()); size_t byteLength = source->byteLength(); MOZ_ASSERT(byteLength > ArrayBufferObject::MaxInlineBytes, @@ -1707,7 +1709,8 @@ ArrayBufferObject::createBufferAndData( // Extract the contents from |source|. BufferContents contents = source->contents(); MOZ_ASSERT(contents); - MOZ_ASSERT(contents.kind() == MALLOCED); + MOZ_ASSERT(contents.kind() == MALLOCED_ARRAYBUFFER_CONTENTS_ARENA || + contents.kind() == MALLOCED_UNKNOWN_ARENA); // Overwrite |source|'s data pointer *without* releasing the data. source->setDataPointer(BufferContents::createNoData()); @@ -1727,7 +1730,7 @@ ArrayBufferObject::createBufferAndData( JSContext* cx, size_t newByteLength, JS::Handle source) { MOZ_ASSERT(!source->isDetached()); - MOZ_ASSERT(source->bufferKind() == MALLOCED); + MOZ_ASSERT(source->bufferKind() == MALLOCED_ARRAYBUFFER_CONTENTS_ARENA); MOZ_ASSERT(newByteLength > ArrayBufferObject::MaxInlineBytes, "prefer copying small buffers"); MOZ_ASSERT(newByteLength <= ArrayBufferObject::MaxByteLength, @@ -1745,7 +1748,7 @@ ArrayBufferObject::createBufferAndData( // Extract the contents from |source|. BufferContents contents = source->contents(); MOZ_ASSERT(contents); - MOZ_ASSERT(contents.kind() == MALLOCED); + MOZ_ASSERT(contents.kind() == MALLOCED_ARRAYBUFFER_CONTENTS_ARENA); // Reallocate the data pointer. auto newData = ReallocateArrayBufferContents(cx, contents.data(), @@ -1754,7 +1757,8 @@ ArrayBufferObject::createBufferAndData( // If reallocation failed, the old pointer is still valid, so just return. return nullptr; } - auto newContents = BufferContents::createMalloced(newData.release()); + auto newContents = + BufferContents::createMallocedArrayBufferContentsArena(newData.release()); // Overwrite |source|'s data pointer *without* releasing the data. source->setDataPointer(BufferContents::createNoData()); @@ -1828,7 +1832,8 @@ ArrayBufferObject* ArrayBufferObject::createFromNewRawBuffer( CheckStealPreconditions(buffer, cx); switch (buffer->bufferKind()) { - case MALLOCED: { + case MALLOCED_ARRAYBUFFER_CONTENTS_ARENA: + case MALLOCED_UNKNOWN_ARENA: { uint8_t* stolenData = buffer->dataPointer(); MOZ_ASSERT(stolenData); @@ -1867,10 +1872,6 @@ ArrayBufferObject* ArrayBufferObject::createFromNewRawBuffer( "memory.grow operation that shouldn't call this " "function"); return nullptr; - - case BAD1: - MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data"); - return nullptr; } MOZ_ASSERT_UNREACHABLE("garbage kind computed"); @@ -1900,10 +1901,12 @@ ArrayBufferObject::extractStructuredCloneContents( } ArrayBufferObject::detach(cx, buffer); - return BufferContents::createMalloced(copiedData.release()); + return BufferContents::createMallocedArrayBufferContentsArena( + copiedData.release()); } - case MALLOCED: + case MALLOCED_ARRAYBUFFER_CONTENTS_ARENA: + case MALLOCED_UNKNOWN_ARENA: case MAPPED: { MOZ_ASSERT(contents); @@ -1928,10 +1931,6 @@ ArrayBufferObject::extractStructuredCloneContents( "external ArrayBuffer shouldn't have passed the " "structured-clone preflighting"); break; - - case BAD1: - MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data"); - break; } MOZ_ASSERT_UNREACHABLE("garbage kind computed"); @@ -1962,7 +1961,7 @@ bool ArrayBufferObject::ensureNonInline(JSContext* cx, return false; } BufferContents outOfLineContents = - BufferContents::createMalloced(copy.release()); + BufferContents::createMallocedArrayBufferContentsArena(copy.release()); buffer->setDataPointer(outOfLineContents); AddCellMemory(buffer, nbytes, MemoryUse::ArrayBufferContents); @@ -1995,7 +1994,8 @@ void ArrayBufferObject::addSizeOfExcludingThis( // Inline data's size should be reported by this object's size-class // reporting. break; - case MALLOCED: + case MALLOCED_ARRAYBUFFER_CONTENTS_ARENA: + case MALLOCED_UNKNOWN_ARENA: if (buffer.isPreparedForAsmJS()) { info->objectsMallocHeapElementsAsmJS += mallocSizeOf(buffer.dataPointer()); @@ -2028,8 +2028,6 @@ void ArrayBufferObject::addSizeOfExcludingThis( } } break; - case BAD1: - MOZ_CRASH("bad bufferKind()"); } } @@ -2377,7 +2375,7 @@ JS_PUBLIC_API JSObject* JS::NewArrayBufferWithContents( using BufferContents = ArrayBufferObject::BufferContents; - BufferContents contents = BufferContents::createMalloced(data); + BufferContents contents = BufferContents::createMallocedUnknownArena(data); return ArrayBufferObject::createForContents(cx, nbytes, contents); } diff --git a/js/src/vm/ArrayBufferObject.h b/js/src/vm/ArrayBufferObject.h index 72166f7e37625..24069b3473396 100644 --- a/js/src/vm/ArrayBufferObject.h +++ b/js/src/vm/ArrayBufferObject.h @@ -196,8 +196,11 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared { /** Inline data kept in the repurposed slots of this ArrayBufferObject. */ INLINE_DATA = 0b000, - /* Data allocated using the SpiderMonkey allocator. */ - MALLOCED = 0b001, + /* + * Data allocated using the SpiderMonkey allocator, created within + * js::ArrayBufferContentsArena. + */ + MALLOCED_ARRAYBUFFER_CONTENTS_ARENA = 0b001, /** * No bytes are associated with this buffer. (This could be because the @@ -218,9 +221,11 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared { MAPPED = 0b101, EXTERNAL = 0b110, - // These kind-values are currently invalid. We intend to expand valid - // BufferKinds in the future to either partly or fully use these values. - BAD1 = 0b111, + /** + * Data allocated using the SpiderMonkey allocator, created within an + * unknown memory arena. + */ + MALLOCED_UNKNOWN_ARENA = 0b111, KIND_MASK = 0b111 }; @@ -285,8 +290,14 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared { return BufferContents(static_cast(data), INLINE_DATA); } - static BufferContents createMalloced(void* data) { - return BufferContents(static_cast(data), MALLOCED); + static BufferContents createMallocedArrayBufferContentsArena(void* data) { + return BufferContents(static_cast(data), + MALLOCED_ARRAYBUFFER_CONTENTS_ARENA); + } + + static BufferContents createMallocedUnknownArena(void* data) { + return BufferContents(static_cast(data), + MALLOCED_UNKNOWN_ARENA); } static BufferContents createNoData() { @@ -314,10 +325,10 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared { } static BufferContents createFailed() { - // There's no harm in tagging this as MALLOCED, even tho obviously it - // isn't. And adding an extra tag purely for this case is a complication - // that presently appears avoidable. - return BufferContents(nullptr, MALLOCED); + // There's no harm in tagging this as MALLOCED_ARRAYBUFFER_CONTENTS_ARENA, + // even tho obviously it isn't. And adding an extra tag purely for this + // case is a complication that presently appears avoidable. + return BufferContents(nullptr, MALLOCED_ARRAYBUFFER_CONTENTS_ARENA); } uint8_t* data() const { return data_; } @@ -461,7 +472,10 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared { } bool isInlineData() const { return bufferKind() == INLINE_DATA; } - bool isMalloced() const { return bufferKind() == MALLOCED; } + bool isMalloced() const { + return bufferKind() == MALLOCED_ARRAYBUFFER_CONTENTS_ARENA || + bufferKind() == MALLOCED_UNKNOWN_ARENA; + } bool isNoData() const { return bufferKind() == NO_DATA; } bool hasUserOwnedData() const { return bufferKind() == USER_OWNED; } diff --git a/js/src/vm/StructuredClone.cpp b/js/src/vm/StructuredClone.cpp index 8b36e3e42aca7..1f12c89dfdc51 100644 --- a/js/src/vm/StructuredClone.cpp +++ b/js/src/vm/StructuredClone.cpp @@ -2295,8 +2295,12 @@ bool JSStructuredCloneWriter::transferOwnership() { if (bufContents.kind() == ArrayBufferObject::MAPPED) { ownership = JS::SCTAG_TMO_MAPPED_DATA; } else { - MOZ_ASSERT(bufContents.kind() == ArrayBufferObject::MALLOCED, - "failing to handle new ArrayBuffer kind?"); + MOZ_ASSERT( + bufContents.kind() == + ArrayBufferObject::MALLOCED_ARRAYBUFFER_CONTENTS_ARENA || + bufContents.kind() == + ArrayBufferObject::MALLOCED_UNKNOWN_ARENA, + "failing to handle new ArrayBuffer kind?"); ownership = JS::SCTAG_TMO_ALLOC_DATA; } extraData = nbytes;