Commit 741f066b authored by Doug Thayer's avatar Doug Thayer
Browse files

Bug 1656261 - Don't set scache mData until decompression done r=froydnj 

So far I have not found a way for this issue to cause bug 1656261, but
it's definitely plausible that there's something there. If we hit a
EXCEPTION_IN_PAGE_ERROR in GetBuffer, then we will exit the
DecompressEntry method with the entry's mData being set. In any
subsequent calls to GetBuffer for that key, we will return the
uninitialized memory. If we can hit this path with chrome.manifest,
then we would end up reading garbage from it, and could conclude that
xul.css is missing.

Whether this is the culprit or not, this is a problem that needs
addressing.

Differential Revision: https://phabricator.services.mozilla.com/D87663
parent 1dde07b0

startupcache/StartupCache.cpp

+17 −9
Original line number Diff line number Diff line
@@ -658,8 +658,9 @@ Result<Ok, nsresult> StartupCache::LoadEntriesOffDisk() { 
          MOZ_ASSERT(false);

          return Err(NS_ERROR_UNEXPECTED);

        }

        value.mSharedDataOffset = sharedBuf.cursor();

        value.mData = MaybeOwnedCharPtr(



        auto sharedDataOffset = sharedBuf.cursor();

        auto decompressionBuffer = MaybeOwnedCharPtr(

            (char*)(sharedBuf.write(value.mUncompressedSize)));



        if (sharedBuf.error()) {
@@ -667,8 +668,14 @@ Result<Ok, nsresult> StartupCache::LoadEntriesOffDisk() { 
          return Err(NS_ERROR_UNEXPECTED);

        }



        MOZ_TRY(DecompressEntry(value));

        value.mData = nullptr;

        // NOTE: we decompress the entry here, but we don't want to assign mData

        // for the table entry, because all we have is a pointer into the

        // ipc::MemMapSnapshot's buffer - once we've finalized the snapshot with

        // mem.Finalize below, we can cut pointers into mSharedData's buffer.

        // This happens inside GetBuffer, using value.mSharedDataOffset which we

        // set here.

        MOZ_TRY(DecompressEntry(value, decompressionBuffer));

        value.mSharedDataOffset = sharedDataOffset;

      }



      if (mem.Finalize(mSharedData).isErr()) {
@@ -789,7 +796,8 @@ Result<Ok, nsresult> StartupCache::LoadArchive() { 
  return Ok();

}



Result<Ok, nsresult> StartupCache::DecompressEntry(StartupCacheEntry& aEntry) {

Result<Ok, nsresult> StartupCache::DecompressEntry(StartupCacheEntry& aEntry,

                                                   MaybeOwnedCharPtr& aBuffer) {

  MOZ_ASSERT(XRE_IsParentProcess());

  mLock.AssertCurrentThreadOwns();
@@ -798,13 +806,12 @@ Result<Ok, nsresult> StartupCache::DecompressEntry(StartupCacheEntry& aEntry) { 
  Span<const char> compressed = Span(

      mCacheData.get<char>().get() + mCacheEntriesBaseOffset + aEntry.mOffset,

      aEntry.mCompressedSize);

  Span<char> uncompressed = Span(aEntry.mData.get(), aEntry.mUncompressedSize);

  Span<char> uncompressed = Span(aBuffer.get(), aEntry.mUncompressedSize);

  bool finished = false;

  while (!finished) {

    auto result = mDecompressionContext->Decompress(

        uncompressed.From(totalWritten), compressed.From(totalRead));

    if (NS_WARN_IF(result.isErr())) {

      aEntry.mData = nullptr;

      InvalidateCacheImpl();

      return Err(NS_ERROR_UNEXPECTED);

    }
@@ -885,14 +892,15 @@ nsresult StartupCache::GetBuffer(const char* id, const char** outbuf, 
      return NS_ERROR_NOT_AVAILABLE;

    }



    value.mData = MaybeOwnedCharPtr(value.mUncompressedSize);

    MMAP_FAULT_HANDLER_BEGIN_BUFFER(value.mData.get(), value.mUncompressedSize)



    auto decompressionResult = DecompressEntry(value);

    auto decompressionBuffer = MaybeOwnedCharPtr(value.mUncompressedSize);

    auto decompressionResult = DecompressEntry(value, decompressionBuffer);

    if (decompressionResult.isErr()) {

      return decompressionResult.unwrapErr();

    }



    value.mData = std::move(decompressionBuffer);

    MMAP_FAULT_HANDLER_CATCH(NS_ERROR_FAILURE)



    label = Telemetry::LABELS_STARTUP_CACHE_REQUESTS::HitDisk;

startupcache/StartupCache.h

+2 −1
Original line number Diff line number Diff line
@@ -389,7 +389,8 @@ class StartupCache : public nsIMemoryReporter { 
  // Writes the cache to disk

  Result<Ok, nsresult> WriteToDisk();



  Result<Ok, nsresult> DecompressEntry(StartupCacheEntry& aEntry);

  Result<Ok, nsresult> DecompressEntry(StartupCacheEntry& aEntry,

                                       MaybeOwnedCharPtr& aBuffer);



  Result<Ok, nsresult> LoadEntriesOffDisk();