Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=dom-workers-and-storage-reviewers,janv
authorSimon Giesecke <sgiesecke@mozilla.com>
Mon, 13 Jul 2020 12:32:06 +0000
changeset 540220 223990c25952ba0f9e55351fe6618205c4efd712
parent 540219 1a69fcdf7bd921334a9c174b88179eff8675fc9e
child 540221 ac47fa4e5e441d56f7621a5cc06413f32277ef88
push id121553
push usersgiesecke@mozilla.com
push dateMon, 13 Jul 2020 16:40:49 +0000
treeherderautoland@223990c25952 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdom-workers-and-storage-reviewers, janv
bugs1652409
milestone80.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=dom-workers-and-storage-reviewers,janv Differential Revision: https://phabricator.services.mozilla.com/D83300
dom/indexedDB/ActorsParent.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -632,18 +632,18 @@ void WriteCompressedNumber(uint64_t aNum
     }
   }
 
   MOZ_ASSERT(buffer > bufferStart);
   MOZ_ASSERT(uint32_t(buffer - bufferStart) ==
              CompressedByteCountForNumber(originalNumber));
 }
 
-std::pair<uint64_t, mozilla::Span<const uint8_t>> ReadCompressedNumber(
-    const Span<const uint8_t> aSpan) {
+Result<std::pair<uint64_t, mozilla::Span<const uint8_t>>, nsresult>
+ReadCompressedNumber(const Span<const uint8_t> aSpan) {
   uint8_t shiftCounter = 0;
   uint64_t result = 0;
 
   const auto end = aSpan.cend();
 
   const auto newPos =
       std::find_if(aSpan.cbegin(), end, [&result, &shiftCounter](uint8_t byte) {
         MOZ_ASSERT(shiftCounter <= 56, "Shifted too many bits!");
@@ -651,36 +651,43 @@ std::pair<uint64_t, mozilla::Span<const 
         result += (uint64_t(byte & 0x7f) << shiftCounter);
         shiftCounter += 7;
 
         return !(byte & 0x80);
       });
 
   if (NS_WARN_IF(newPos == end)) {
     MOZ_ASSERT(false);
-    // XXX Shouldn't we return an error in this case?
-  }
-
-  return {result, Span{newPos + 1, end}};
+    IDB_REPORT_INTERNAL_ERR();
+    return Err(NS_ERROR_FILE_CORRUPTED);
+  }
+
+  return std::pair{result, Span{newPos + 1, end}};
 }
 
 void WriteCompressedIndexId(IndexOrObjectStoreId aIndexId, bool aUnique,
                             uint8_t** aIterator) {
   MOZ_ASSERT(aIndexId);
   MOZ_ASSERT(UINT64_MAX - uint64_t(aIndexId) >= uint64_t(aIndexId),
              "Overflow!");
   MOZ_ASSERT(aIterator);
   MOZ_ASSERT(*aIterator);
 
   const uint64_t indexId = (uint64_t(aIndexId * 2) | (aUnique ? 1 : 0));
   WriteCompressedNumber(indexId, aIterator);
 }
 
-auto ReadCompressedIndexId(const Span<const uint8_t> aData) {
-  const auto [indexId, remainder] = ReadCompressedNumber(aData);
+Result<std::tuple<IndexOrObjectStoreId, bool, Span<const uint8_t>>, nsresult>
+ReadCompressedIndexId(const Span<const uint8_t> aData) {
+  auto readNumberOrErr = ReadCompressedNumber(aData);
+  if (NS_WARN_IF(readNumberOrErr.isErr())) {
+    return readNumberOrErr.propagateErr();
+  }
+
+  const auto [indexId, remainder] = readNumberOrErr.unwrap();
 
   MOZ_ASSERT(UINT64_MAX / 2 >= uint64_t(indexId), "Bad index id!");
 
   return std::tuple{IndexOrObjectStoreId(indexId >> 1), indexId % 2 == 1,
                     remainder};
 }
 
 // static
@@ -781,55 +788,64 @@ nsresult ReadCompressedIndexDataValuesFr
   // XXX Is this check still necessary with a Span? Or should it rather be moved
   // to the caller?
   if (uintptr_t(aBlobData.Elements()) > UINTPTR_MAX - aBlobData.LengthBytes()) {
     IDB_REPORT_INTERNAL_ERR();
     return NS_ERROR_FILE_CORRUPTED;
   }
 
   for (auto remainder = aBlobData; !remainder.IsEmpty();) {
+    auto readIndexIdOrErr = ReadCompressedIndexId(remainder);
+    if (NS_WARN_IF(readIndexIdOrErr.isErr())) {
+      return readIndexIdOrErr.unwrapErr();
+    }
+
     const auto [indexId, unique, remainderAfterIndexId] =
-        ReadCompressedIndexId(remainder);
+        readIndexIdOrErr.unwrap();
 
     if (NS_WARN_IF(remainderAfterIndexId.IsEmpty())) {
       IDB_REPORT_INTERNAL_ERR();
       return NS_ERROR_FILE_CORRUPTED;
     }
 
     // Read key buffer length.
+    auto readNumberOrErr = ReadCompressedNumber(remainderAfterIndexId);
+    if (NS_WARN_IF(readNumberOrErr.isErr())) {
+      return readNumberOrErr.unwrapErr();
+    }
+
     const auto [keyBufferLength, remainderAfterKeyBufferLength] =
-        ReadCompressedNumber(remainderAfterIndexId);
+        readNumberOrErr.unwrap();
 
     if (NS_WARN_IF(remainderAfterKeyBufferLength.IsEmpty()) ||
         NS_WARN_IF(keyBufferLength > uint64_t(UINT32_MAX)) ||
-        NS_WARN_IF(keyBufferLength > remainderAfterKeyBufferLength.Length())
-        // XXX Does this sub-condition make any sense?
-        // || NS_WARN_IF(keyBufferLength > uintptr_t(blobDataEnd))
-    ) {
+        NS_WARN_IF(keyBufferLength > remainderAfterKeyBufferLength.Length())) {
       IDB_REPORT_INTERNAL_ERR();
       return NS_ERROR_FILE_CORRUPTED;
     }
 
     const auto [keyBuffer, remainderAfterKeyBuffer] =
         remainderAfterKeyBufferLength.SplitAt(keyBufferLength);
     auto idv =
         IndexDataValue{indexId, unique, Key{nsCString{AsChars(keyBuffer)}}};
 
     // Read sort key buffer length.
+    readNumberOrErr = ReadCompressedNumber(remainderAfterKeyBuffer);
+    if (NS_WARN_IF(readNumberOrErr.isErr())) {
+      return readNumberOrErr.unwrapErr();
+    }
+
     const auto [sortKeyBufferLength, remainderAfterSortKeyBufferLength] =
-        ReadCompressedNumber(remainderAfterKeyBuffer);
+        readNumberOrErr.unwrap();
 
     remainder = remainderAfterSortKeyBufferLength;
     if (sortKeyBufferLength > 0) {
       if (NS_WARN_IF(remainder.IsEmpty()) ||
           NS_WARN_IF(sortKeyBufferLength > uint64_t(UINT32_MAX)) ||
-          NS_WARN_IF(sortKeyBufferLength > remainder.Length())
-          // XXX Does this sub-condition make any sense?
-          // || NS_WARN_IF(sortKeyBufferLength > uintptr_t(blobDataEnd))
-      ) {
+          NS_WARN_IF(sortKeyBufferLength > remainder.Length())) {
         IDB_REPORT_INTERNAL_ERR();
         return NS_ERROR_FILE_CORRUPTED;
       }
 
       const auto [sortKeyBuffer, remainderAfterSortKeyBuffer] =
           remainder.SplitAt(sortKeyBufferLength);
       idv.mLocaleAwarePosition = Key{nsCString{AsChars(sortKeyBuffer)}};
       remainder = remainderAfterSortKeyBuffer;
@@ -3455,28 +3471,38 @@ UpgradeIndexDataValuesFunction::ReadOldC
 
   IndexOrObjectStoreId indexId;
   bool unique;
   bool nextIndexIdAlreadyRead = false;
 
   IndexDataValuesArray result;
   for (auto remainder = aBlobData; !remainder.IsEmpty();) {
     if (!nextIndexIdAlreadyRead) {
-      std::tie(indexId, unique, remainder) = ReadCompressedIndexId(remainder);
+      auto readNumberOrErr = ReadCompressedIndexId(remainder);
+      if (NS_WARN_IF(readNumberOrErr.isErr())) {
+        return readNumberOrErr.propagateErr();
+      }
+
+      std::tie(indexId, unique, remainder) = readNumberOrErr.unwrap();
     }
     nextIndexIdAlreadyRead = false;
 
     if (NS_WARN_IF(remainder.IsEmpty())) {
       IDB_REPORT_INTERNAL_ERR();
       return Err(NS_ERROR_FILE_CORRUPTED);
     }
 
     // Read key buffer length.
+    auto readNumberOrErr = ReadCompressedNumber(remainder);
+    if (NS_WARN_IF(readNumberOrErr.isErr())) {
+      return readNumberOrErr.propagateErr();
+    }
+
     const auto [keyBufferLength, remainderAfterKeyBufferLength] =
-        ReadCompressedNumber(remainder);
+        readNumberOrErr.unwrap();
 
     if (NS_WARN_IF(remainderAfterKeyBufferLength.IsEmpty()) ||
         NS_WARN_IF(keyBufferLength > uint64_t(UINT32_MAX)) ||
         NS_WARN_IF(keyBufferLength > remainderAfterKeyBufferLength.Length())) {
       IDB_REPORT_INTERNAL_ERR();
       return Err(NS_ERROR_FILE_CORRUPTED);
     }
 
@@ -3486,18 +3512,23 @@ UpgradeIndexDataValuesFunction::ReadOldC
                                        Key{nsCString{AsChars(keyBuffer)}}))) {
       IDB_REPORT_INTERNAL_ERR();
       return Err(NS_ERROR_OUT_OF_MEMORY);
     }
 
     remainder = remainderAfterKeyBuffer;
     if (!remainder.IsEmpty()) {
       // Read either a sort key buffer length or an index id.
+      auto readNumberOrErr = ReadCompressedNumber(remainder);
+      if (NS_WARN_IF(readNumberOrErr.isErr())) {
+        return readNumberOrErr.propagateErr();
+      }
+
       const auto [maybeIndexId, remainderAfterIndexId] =
-          ReadCompressedNumber(remainder);
+          readNumberOrErr.unwrap();
 
       // Locale-aware indexes haven't been around long enough to have any users,
       // we can safely assume all sort key buffer lengths will be zero.
       // XXX This duplicates logic from ReadCompressedIndexId.
       if (maybeIndexId != 0) {
         unique = maybeIndexId % 2 == 1;
         indexId = maybeIndexId / 2;
         nextIndexIdAlreadyRead = true;