From 23753d24a44b8881fb562ac8f788a2cac45f3c5a Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 17 Sep 2024 14:09:32 -0400 Subject: [PATCH] enforce isolate lock and track allocations on v8 --- src/workerd/api/node/zlib-util.c++ | 36 +++++++++++++------------ src/workerd/api/node/zlib-util.h | 20 +++++++------- src/workerd/api/streams/compression.c++ | 36 ++++++++++++++++++------- src/workerd/api/streams/compression.h | 9 ++----- 4 files changed, 57 insertions(+), 44 deletions(-) diff --git a/src/workerd/api/node/zlib-util.c++ b/src/workerd/api/node/zlib-util.c++ index 3e87a57859e..6dc5770ff7d 100644 --- a/src/workerd/api/node/zlib-util.c++ +++ b/src/workerd/api/node/zlib-util.c++ @@ -444,8 +444,8 @@ void ZlibContext::setOutputBuffer(kj::ArrayPtr output) { template jsg::Ref> ZlibUtil::CompressionStream< - CompressionContext>::constructor(ZlibModeValue mode) { - return jsg::alloc(static_cast(mode)); + CompressionContext>::constructor(jsg::Lock& js, ZlibModeValue mode) { + return jsg::alloc(js, static_cast(mode)); } template @@ -584,8 +584,9 @@ void ZlibUtil::CompressionStream::reset(jsg::Lock& js) { } } -jsg::Ref ZlibUtil::ZlibStream::constructor(ZlibModeValue mode) { - return jsg::alloc(static_cast(mode)); +jsg::Ref ZlibUtil::ZlibStream::constructor( + jsg::Lock& js, ZlibModeValue mode) { + return jsg::alloc(js, static_cast(mode)); } void ZlibUtil::ZlibStream::initialize(int windowBits, @@ -752,8 +753,8 @@ kj::Maybe BrotliDecoderContext::getError() const { template jsg::Ref> ZlibUtil::BrotliCompressionStream< - CompressionContext>::constructor(ZlibModeValue mode) { - return jsg::alloc(static_cast(mode)); + CompressionContext>::constructor(jsg::Lock& js, ZlibModeValue mode) { + return jsg::alloc(js, static_cast(mode)); } template @@ -806,10 +807,10 @@ static kj::Array syncProcessBuffer(Context& ctx, GrowableBuffer& resul } // namespace kj::Array ZlibUtil::zlibSync( - ZlibUtil::InputSource data, ZlibContext::Options opts, ZlibModeValue mode) { + jsg::Lock& js, ZlibUtil::InputSource data, ZlibContext::Options opts, ZlibModeValue mode) { // Any use of zlib APIs constitutes an implicit dependency on Allocator which must // remain alive until the zlib stream is destroyed - CompressionAllocator allocator; + CompressionAllocator allocator(js); ZlibContext ctx(static_cast(mode)); allocator.configure(ctx.getStream()); @@ -849,7 +850,7 @@ void ZlibUtil::zlibWithCallback(jsg::Lock& js, CompressCallback cb) { // Capture only relevant errors so they can be passed to the callback auto res = js.tryCatch([&]() { - return CompressCallbackArg(zlibSync(kj::mv(data), kj::mv(options), mode)); + return CompressCallbackArg(zlibSync(js, kj::mv(data), kj::mv(options), mode)); }, [&](jsg::Value&& exception) { return CompressCallbackArg(jsg::JsValue(exception.getHandle(js))); }); @@ -859,10 +860,11 @@ void ZlibUtil::zlibWithCallback(jsg::Lock& js, } template -kj::Array ZlibUtil::brotliSync(InputSource data, BrotliContext::Options opts) { +kj::Array ZlibUtil::brotliSync( + jsg::Lock& js, InputSource data, BrotliContext::Options opts) { // Any use of brotli APIs constitutes an implicit dependency on Allocator which must // remain alive until the brotli state is destroyed - CompressionAllocator allocator; + CompressionAllocator allocator(js); Context ctx(Context::Mode); auto chunkSize = opts.chunkSize.orDefault(ZLIB_PERFORMANT_CHUNK_SIZE); @@ -914,7 +916,7 @@ void ZlibUtil::brotliWithCallback( jsg::Lock& js, InputSource data, BrotliContext::Options options, CompressCallback cb) { // Capture only relevant errors so they can be passed to the callback auto res = js.tryCatch([&]() { - return CompressCallbackArg(brotliSync(kj::mv(data), kj::mv(options))); + return CompressCallbackArg(brotliSync(js, kj::mv(data), kj::mv(options))); }, [&](jsg::Value&& exception) { return CompressCallbackArg(jsg::JsValue(exception.getHandle(js))); }); @@ -933,25 +935,25 @@ void ZlibUtil::brotliWithCallback( jsg::Optional> input, int inputOffset, int inputLength, \ kj::Array output, int outputOffset, int outputLength); \ template jsg::Ref> ZlibUtil::CompressionStream::constructor( \ - ZlibModeValue mode); + jsg::Lock& js, ZlibModeValue mode); CREATE_TEMPLATE(ZlibContext) CREATE_TEMPLATE(BrotliEncoderContext) CREATE_TEMPLATE(BrotliDecoderContext) template jsg::Ref> ZlibUtil:: - BrotliCompressionStream::constructor(ZlibModeValue mode); + BrotliCompressionStream::constructor(jsg::Lock& js, ZlibModeValue mode); template jsg::Ref> ZlibUtil:: - BrotliCompressionStream::constructor(ZlibModeValue mode); + BrotliCompressionStream::constructor(jsg::Lock& js, ZlibModeValue mode); template bool ZlibUtil::BrotliCompressionStream::initialize( jsg::Lock&, jsg::BufferSource, jsg::BufferSource, jsg::Function); template bool ZlibUtil::BrotliCompressionStream::initialize( jsg::Lock&, jsg::BufferSource, jsg::BufferSource, jsg::Function); template kj::Array ZlibUtil::brotliSync( - InputSource data, BrotliContext::Options opts); + jsg::Lock& js, InputSource data, BrotliContext::Options opts); template kj::Array ZlibUtil::brotliSync( - InputSource data, BrotliContext::Options opts); + jsg::Lock& js, InputSource data, BrotliContext::Options opts); template void ZlibUtil::brotliWithCallback( jsg::Lock& js, InputSource data, BrotliContext::Options options, CompressCallback cb); template void ZlibUtil::brotliWithCallback( diff --git a/src/workerd/api/node/zlib-util.h b/src/workerd/api/node/zlib-util.h index 0c9a4e8db1f..f01b853ae2c 100644 --- a/src/workerd/api/node/zlib-util.h +++ b/src/workerd/api/node/zlib-util.h @@ -293,13 +293,12 @@ class ZlibUtil final: public jsg::Object { template class CompressionStream: public jsg::Object { public: - explicit CompressionStream(ZlibMode _mode): context_(_mode) {} - CompressionStream() = default; + explicit CompressionStream(jsg::Lock& js, ZlibMode _mode): allocator(js), context_(_mode) {} // TODO(soon): Find a way to add noexcept(false) to this destructor. ~CompressionStream(); KJ_DISALLOW_COPY_AND_MOVE(CompressionStream); - static jsg::Ref constructor(ZlibModeValue mode); + static jsg::Ref constructor(jsg::Lock& js, ZlibModeValue mode); void close(); bool checkError(jsg::Lock& js); @@ -359,9 +358,9 @@ class ZlibUtil final: public jsg::Object { class ZlibStream final: public CompressionStream { public: - explicit ZlibStream(ZlibMode mode): CompressionStream(mode) {} + explicit ZlibStream(jsg::Lock& js, ZlibMode mode): CompressionStream(js, mode) {} KJ_DISALLOW_COPY_AND_MOVE(ZlibStream); - static jsg::Ref constructor(ZlibModeValue mode); + static jsg::Ref constructor(jsg::Lock& js, ZlibModeValue mode); // Instance methods void initialize(int windowBits, @@ -384,10 +383,10 @@ class ZlibUtil final: public jsg::Object { template class BrotliCompressionStream: public CompressionStream { public: - explicit BrotliCompressionStream(ZlibMode _mode) - : CompressionStream(_mode) {} + explicit BrotliCompressionStream(jsg::Lock& js, ZlibMode _mode) + : CompressionStream(js, _mode) {} KJ_DISALLOW_COPY_AND_MOVE(BrotliCompressionStream); - static jsg::Ref constructor(ZlibModeValue mode); + static jsg::Ref constructor(jsg::Lock& js, ZlibModeValue mode); bool initialize(jsg::Lock& js, jsg::BufferSource params, @@ -423,10 +422,11 @@ class ZlibUtil final: public jsg::Object { ZlibContext::Options options, ZlibModeValue mode, CompressCallback cb); - kj::Array zlibSync(InputSource data, ZlibContext::Options options, ZlibModeValue mode); + kj::Array zlibSync( + jsg::Lock& js, InputSource data, ZlibContext::Options options, ZlibModeValue mode); template - kj::Array brotliSync(InputSource data, BrotliContext::Options options); + kj::Array brotliSync(jsg::Lock& js, InputSource data, BrotliContext::Options options); template void brotliWithCallback( jsg::Lock& js, InputSource data, BrotliContext::Options options, CompressCallback cb); diff --git a/src/workerd/api/streams/compression.c++ b/src/workerd/api/streams/compression.c++ index 4caafa2aec0..e2cd0563736 100644 --- a/src/workerd/api/streams/compression.c++ +++ b/src/workerd/api/streams/compression.c++ @@ -12,6 +12,10 @@ namespace workerd::api { +CompressionAllocator::CompressionAllocator(jsg::Lock& js) { + memoryAdjustment = js.getExternalMemoryAdjustment(0); +} + void CompressionAllocator::configure(z_stream* stream) { stream->zalloc = AllocForZlib; stream->zfree = FreeForZlib; @@ -27,17 +31,28 @@ void* CompressionAllocator::AllocForZlib(void* data, uInt items, uInt size) { // TODO(soon): Enabling V8 reporting breaks CompressionStreamImpl on edgeworker. Investigate. // This is mostly likely due to running CompressionStream without an isolate lock. void* CompressionAllocator::AllocForBrotli(void* opaque, size_t size) { - auto* thisAllocator = static_cast(opaque); - auto data = kj::heapArray(size); + auto* allocator = static_cast(opaque); + auto data = kj::heapArray(size); auto begin = data.begin(); - thisAllocator->allocations.insert(begin, kj::mv(data)); + *reinterpret_cast(begin) = size; + auto isolate = v8::Isolate::GetCurrent(); + KJ_REQUIRE_NONNULL(isolate, "Isolate lock is required"_kj); + auto& js = jsg::Lock::from(isolate); + allocator->memoryAdjustment.set(js, allocator->memoryAdjustment.getAmount() + size); + allocator->allocations.insert(begin, kj::mv(data)); return begin; } void CompressionAllocator::FreeForZlib(void* opaque, void* pointer) { if (KJ_UNLIKELY(pointer == nullptr)) return; - auto* thisAllocator = static_cast(opaque); - JSG_REQUIRE(thisAllocator->allocations.erase(pointer), Error, "Zlib allocation should exist"_kj); + auto* allocator = static_cast(opaque); + auto* real_pointer = static_cast(pointer) - sizeof(size_t); + size_t real_size = *reinterpret_cast(real_pointer); + auto isolate = v8::Isolate::GetCurrent(); + KJ_REQUIRE_NONNULL(isolate, "Isolate lock is required"_kj); + auto& js = jsg::Lock::from(isolate); + allocator->memoryAdjustment.set(js, allocator->memoryAdjustment.getAmount() - real_size); + JSG_REQUIRE(allocator->allocations.erase(real_pointer), Error, "Zlib allocation should exist"_kj); } namespace { @@ -59,8 +74,9 @@ public: kj::ArrayPtr buffer; }; - explicit Context(Mode mode, kj::StringPtr format, ContextFlags flags) + explicit Context(jsg::Lock& js, Mode mode, kj::StringPtr format, ContextFlags flags) : mode(mode), + allocator(js), strictCompression(flags) { // Configure allocator before any stream operations. allocator.configure(&ctx); @@ -225,8 +241,8 @@ class CompressionStreamImpl: public kj::Refcounted, public ReadableStreamSource, public WritableStreamSink { public: - explicit CompressionStreamImpl(kj::String format, Context::ContextFlags flags) - : context(mode, format, flags) {} + explicit CompressionStreamImpl(jsg::Lock& js, kj::String format, Context::ContextFlags flags) + : context(js, mode, format, flags) {} // WritableStreamSink implementation --------------------------------------------------- @@ -472,7 +488,7 @@ jsg::Ref CompressionStream::constructor(jsg::Lock& js, kj::St "The compression format must be either 'deflate', 'deflate-raw' or 'gzip'."); auto readableSide = kj::refcounted>( - kj::mv(format), Context::ContextFlags::NONE); + js, kj::mv(format), Context::ContextFlags::NONE); auto writableSide = kj::addRef(*readableSide); auto& ioContext = IoContext::current(); @@ -486,7 +502,7 @@ jsg::Ref DecompressionStream::constructor(jsg::Lock& js, kj "The compression format must be either 'deflate', 'deflate-raw' or 'gzip'."); auto readableSide = - kj::refcounted>(kj::mv(format), + kj::refcounted>(js, kj::mv(format), FeatureFlags::get(js).getStrictCompression() ? Context::ContextFlags::STRICT : Context::ContextFlags::NONE); auto writableSide = kj::addRef(*readableSide); diff --git a/src/workerd/api/streams/compression.h b/src/workerd/api/streams/compression.h index 7418964c0bb..1e9c0bf25fb 100644 --- a/src/workerd/api/streams/compression.h +++ b/src/workerd/api/streams/compression.h @@ -16,7 +16,7 @@ namespace workerd::api { // isolate pointer and use that to get the external memory adjustment. class CompressionAllocator final { public: - CompressionAllocator() = default; + CompressionAllocator(jsg::Lock& js); void configure(z_stream* stream); static void* AllocForZlib(void* data, uInt items, uInt size); @@ -24,12 +24,7 @@ class CompressionAllocator final { static void FreeForZlib(void* data, void* pointer); private: - // TODO(soon): Use this instead of array after fixing edgeworker. - // struct Allocation { - // kj::Array data; - // kj::Maybe memoryAdjustment; - // }; - + jsg::ExternalMemoryAdjustment memoryAdjustment; kj::HashMap> allocations; };