Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=janv, a=RyanVM
authorSimon Giesecke <sgiesecke@mozilla.com>
Mon, 13 Jul 2020 12:32:06 +0000
changeset 602094 aeb2043f8ae6b20680d44be9d9550ecbaf6d971d
parent 602093 e2fb5bb5b4fbe9059dee724ff55582d35f6336e2
child 602095 8b8c4c23c0bcf047000ec959931f5df2b57a08ea
push id13419
push userryanvm@gmail.com
push dateThu, 16 Jul 2020 17:39:13 +0000
treeherdermozilla-beta@aeb2043f8ae6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanv, RyanVM
bugs1652409
milestone79.0
Bug 1652409 - Ensure that a corrupted file encountered during ReadCompressedNumber does not cause an assertion failure. r=janv, a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D83300
dom/indexedDB/ActorsParent.cpp
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -637,18 +637,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!");
@@ -656,36 +656,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
@@ -786,55 +793,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;
@@ -3494,28 +3510,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);
     }
 
@@ -3525,18 +3551,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;