1452784 - Allocator mismatch involving StructuredCloneWriteCallback() and ~DataOwner()

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P2)

Description

[mozillabugs]

Attachment: test2.html

StructuredCloneWriteCallback() (dom\indexedDB\IDBObjectStore.cpp) creates objects using new[] that MemoryBlobImpl::DataOwner::~DataOwner() (dom\file\MemoryBlobImpl.cpp/.h) later releases using free().

This mismatch could cause heap corruption, depending upon how new[] is implemented. In the debug build of FF 58.0.2, the source (mozalloc.h) implements it by calling moz_xmalloc(), and the compiler appears not to add any additional data to the allocation, so the mismatch appears safe. Other compilers (or VS with different switches) could, however, add additional data to the allocation that new[] and delete[] use internally. In that case, using free() on the pointer returned by new[] would cause deallocation of only part of the block allocated by new[], which would cause heap corruption and possibly other issues with internal data maintained by new[] and delete[].

Attached is a POC that demonstrates the issue. Use it by putting the POC files on a webserver, starting FF, and putting a BP on StructuredCloneWriteCallback() line 633:

477: bool
478: StructuredCloneWriteCallback(JSContext* aCx,
479:                              JSStructuredCloneWriter* aWriter,
480:                              JS::Handle<JSObject*> aObj,
481:                              void* aClosure)
482: {
...
633:   if (JS::IsWasmModuleObject(aObj)) {
634:     RefPtr<JS::WasmModule> module = JS::GetWasmModule(aObj);
635:     MOZ_ASSERT(module);
636: 
637:     size_t bytecodeSize = module->bytecodeSerializedSize();
638:     UniquePtr<uint8_t[]> bytecode(new uint8_t[bytecodeSize]);
639:     module->bytecodeSerialize(bytecode.get(), bytecodeSize);
640: 
641:     RefPtr<BlobImpl> blobImpl =
642:       new MemoryBlobImpl(bytecode.release(), bytecodeSize, EmptyString());
643: 
644:     RefPtr<Blob> bytecodeBlob = Blob::Create(nullptr, blobImpl);
645: 
646:     if (module->compilationComplete()) {
647:       size_t compiledSize = module->compiledSerializedSize();
648:       UniquePtr<uint8_t[]> compiled(new uint8_t[compiledSize]);
649:       module->compiledSerialize(compiled.get(), compiledSize);
650: 
651:       blobImpl =
652:         new MemoryBlobImpl(compiled.release(), compiledSize, EmptyString());
653:     } else {
654:       nsCOMPtr<nsIInputStream> stream(new WasmCompiledModuleStream(module));
655: 
656:       blobImpl = StreamBlobImpl::Create(stream, EmptyString(), UINT64_MAX);
657:     }
658: 
659:     RefPtr<Blob> compiledBlob = Blob::Create(nullptr, blobImpl);
660:
... [queue the blob for writing]
688: }

and on ~DataOwner() (in the .h file, line 89).

80:     ~DataOwner() {
81:       mozilla::StaticMutexAutoLock lock(sDataOwnerMutex);
82: 
83:       remove();
84:       if (sDataOwners->isEmpty()) {
85:         // Free the linked list if it's empty.
86:         sDataOwners = nullptr;
87:       }
88: 
89:       free(mData);
90:     }

Now load test2.html in FF. When the BP in StructuredCloneWriteCallback() fires, trace the allocation on line 638 and remember the pointer returned by new[]. Do the same for the allocation on line 648.

Now proceed. When the BP on ~DataOwner() fires, verify that |mData| is one of the values that you recorded above, and that it is released by calling free(). Proceed and do the same thing again for the other value.

Comment 1

[mozillabugs]

Attachment: test2.js

Comment 2

[mozillabugs]

Attachment: test2.wasm

Comment 3

[Christian Holler (:decoder)]

I think these allocator mismatches are not exploitable in our codebase because we use jemalloc for both. But I would like to defer that question to someone who knows it much better than I do.

Comment 4

[Mike Hommey [:glandium]]

:decoder is right. In Firefox, new[] and new and implemented in terms of (derivatives of) malloc, and delete and delete[] in terms of (derivatives of) free. Which makes allocator mismatch not a practical problem. In fact, in many cases, they're inlined, and you wouldn't even observe calls to new/new[]/delete/delete[]. This can sometimes cause the converse problem, where you can observe a mismatch between e.g. malloc/delete when the code actually correctly uses new and delete.

Comment 5

[Mike Hommey [:glandium]]

I'll add, though, that it's always useful to fix those mismatches even when they're not a practical problem.

Comment 6

[mozillabugs]

(In reply to Mike Hommey [:glandium] from comment #4)
> :decoder is right. In Firefox, new[] and new and implemented in terms of
> (derivatives of) malloc, and delete and delete[] in terms of (derivatives
> of) free. Which makes allocator mismatch not a practical problem. In fact,
> in many cases, they're inlined, and you wouldn't even observe calls to
> new/new[]/delete/delete[].

This appears to be true when new (new[]) is used to allocate a POD (or array of PODs) and free() is used to deallocate it. However, allocation of non-PODs via new (new[]), combined with deallocation by free(), often will not work correctly. For one thing, free() will not call a non-POD's destructor. This could, for example, leave a freed object like:

   struct Foo {
      Foo *m_flink;
      Foo *m_blink;
      ...
      virtual ~Foo() {
         Foo *flink = m_flink;
         m_blink -> m_flink = m_flink;  // remove from list
         flink -> m_blink = mblink;
         ...
      }
   };

present in a linked list, causing a use after free bug. Conversely, allocation of a non-POD by malloc(), followed by deallocation by delete (delete[]) will cause use of (and a destructor call for) an object for the constructor was never called, which is UB.

Comment 7

[mozillabugs]

an object for the constructor was never called => an object for *which* the constructor was never called,

Comment 8

[Christian Holler (:decoder)]

Most of the code we have on Gecko uses private destructors with refcounting to ensure that things are deleted properly, or is handled by garbage collection. I'm not sure if/where we actually call delete on non-POD structures.

In this particular case it doesn't matter though because all types involved are PODs. Nevertheless, like Mike said, we should fix this.

Comment 9

[Frederik Braun [:freddy]]

decoder asked me to make the bug public.

Comment 10

[Marion Daly [:mdaly]]

Want me to find a new owner for this bug?

Comment 11

[Andrea Marchesini [:baku]]

Ah! I completely ignored this bug. I don't receive notifications when a bug is assigned to me. Sorry for this delay.
I work on it.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: allocation.patch

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b78c2999400
Use malloc() instead of new[] in IDBObjectStorage to match the use of free() in memory BlobImpl, r=decoder

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8b78c2999400

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8b78c2999400

Comment 16

[Daniel Veditz [:dveditz]]

Minusing for a bounty because this can't be a problem in our codebase (not even the theoretically-possible mismatch that can come up in the case of using system NSPR since that's not involved here).