Skip to content

Commit

Permalink
Bug 1872555: Transfer can only realloc when the jemalloc arena is kno…
Browse files Browse the repository at this point in the history 
…wn. 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
  • Loading branch information
@anba
anba committed Jan 24, 2024
1 parent 12002be commit 19b4834
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 52 deletions.
12 changes: 12 additions & 0 deletions js/src/jit-test/tests/typedarray/arraybuffer-transfer-unknown-arena.js
View file
Original file line number Diff line number Diff line change
@@ -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);
2 changes: 1 addition & 1 deletion js/src/shell/js.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ArrayBufferObject*> arrayBuffer(
cx, ArrayBufferObject::createForContents(cx, length, contents));
if (!arrayBuffer) {
Expand Down
72 changes: 35 additions & 37 deletions js/src/vm/ArrayBufferObject.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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?");
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1204,9 +1202,6 @@ void ArrayBufferObject::releaseData(JS::GCContext* gcx) {
freeInfo()->freeFunc(dataPointer(), freeInfo()->freeUserData);
}
break;
case BAD1:
MOZ_CRASH("invalid BufferKind encountered");
break;
}
}

Expand All @@ -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");
Expand Down Expand Up @@ -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");
}
}
Expand All @@ -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);
}

Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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);
Expand All @@ -1693,7 +1695,7 @@ ArrayBufferObject::createBufferAndData(
/* static */ ArrayBufferObject* ArrayBufferObject::copyAndDetachSteal(
JSContext* cx, JS::Handle<ArrayBufferObject*> source) {
MOZ_ASSERT(!source->isDetached());
MOZ_ASSERT(source->bufferKind() == MALLOCED);
MOZ_ASSERT(source->isMalloced());

size_t byteLength = source->byteLength();
MOZ_ASSERT(byteLength > ArrayBufferObject::MaxInlineBytes,
Expand All @@ -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());
Expand All @@ -1727,7 +1730,7 @@ ArrayBufferObject::createBufferAndData(
JSContext* cx, size_t newByteLength,
JS::Handle<ArrayBufferObject*> 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,
Expand All @@ -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(),
Expand All @@ -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());
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);

Expand All @@ -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");
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -2028,8 +2028,6 @@ void ArrayBufferObject::addSizeOfExcludingThis(
}
}
break;
case BAD1:
MOZ_CRASH("bad bufferKind()");
}
}

Expand Down Expand Up @@ -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);
}

Expand Down
38 changes: 26 additions & 12 deletions js/src/vm/ArrayBufferObject.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
};
Expand Down Expand Up @@ -285,8 +290,14 @@ class ArrayBufferObject : public ArrayBufferObjectMaybeShared {
return BufferContents(static_cast<uint8_t*>(data), INLINE_DATA);
}

static BufferContents createMalloced(void* data) {
return BufferContents(static_cast<uint8_t*>(data), MALLOCED);
static BufferContents createMallocedArrayBufferContentsArena(void* data) {
return BufferContents(static_cast<uint8_t*>(data),
MALLOCED_ARRAYBUFFER_CONTENTS_ARENA);
}

static BufferContents createMallocedUnknownArena(void* data) {
return BufferContents(static_cast<uint8_t*>(data),
MALLOCED_UNKNOWN_ARENA);
}

static BufferContents createNoData() {
Expand Down Expand Up @@ -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_; }
Expand Down Expand Up @@ -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; }

Expand Down
8 changes: 6 additions & 2 deletions js/src/vm/StructuredClone.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 19b4834

Please sign in to comment.