Bug 1605478 - Add temporary MOZ_DIAGNOSTIC_ASSERT to narrow down UntrustedModulesData's integrity problem. r=aklotz
authorToshihito Kikuchi <tkikuchi@mozilla.com>
Sun, 08 Mar 2020 23:13:48 +0000
changeset 517485 2540a369a5a8d0df5a30d56a39225435844f548d
parent 517484 ba3ca434e3bc0ce194987e856412a961736555b0
child 517486 f714fc6d0d908979703f4c04961c79a808ec05fd
push id37194
push usershindli@mozilla.com
push dateMon, 09 Mar 2020 03:45:29 +0000
treeherdermozilla-central@2540a369a5a8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz
bugs1605478, 1603714
milestone75.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 1605478 - Add temporary MOZ_DIAGNOSTIC_ASSERT to narrow down UntrustedModulesData's integrity problem. r=aklotz Bug 1603714 showed there were `UntrustedModulesData` instances in which a load event pointed to a module which did not exist in the modules list. This patch adds `MOZ_DIAGNOSTIC_ASSERT` to the following places to narrow down when it happened. Given that the number of the impected users seems big (~200 crashes/day on Nightly), we activate the assers with a probability of 1/16 (~12.5 crashes/day). 1. When processing load events 1-1. [Content] `UntrustedModulesProcessor::CompleteProcessing:` Verify events of a trusted module were eliminated by `GetModulesTrust` 1-2. [Content] `UntrustedModulesData::AddNewLoads`: Verify a new `ModuleRecord` matches the event 1-3. [Content] `UntrustedModulesProcessor::CompleteProcessing`: Verify processed data after new items were appended. 2. When processed data is sent 2-1. [Content] `UntrustedModulesProcessor::GetAllProcessedData`: Verify processed data before serialization. 2-2. [Content] `ParamTraits<mozilla::UntrustedModulesData>::WriteEvent`: Verify processed data before transferring to the browser process 2-3. [Browser] `ParamTraits<mozilla::UntrustedModulesData>::ReadEvent`: A final point to catch this integrity problem. We had an IPC error here. Differential Revision: https://phabricator.services.mozilla.com/D59964
toolkit/xre/UntrustedModulesData.cpp
toolkit/xre/UntrustedModulesData.h
toolkit/xre/UntrustedModulesProcessor.cpp
--- a/toolkit/xre/UntrustedModulesData.cpp
+++ b/toolkit/xre/UntrustedModulesData.cpp
@@ -8,16 +8,17 @@
 
 #include <windows.h>
 
 #include "mozilla/CmdLineAndEnvUtils.h"
 #include "mozilla/DynamicallyLinkedFunctionPtr.h"
 #include "mozilla/FileUtilsWin.h"
 #include "mozilla/Likely.h"
 #include "mozilla/MathAlgorithms.h"
+#include "mozilla/RandomNum.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/Unused.h"
 #include "mozilla/WinDllServices.h"
 #include "ModuleEvaluator.h"
 #include "nsCOMPtr.h"
 #include "nsDebug.h"
 #include "nsXULAppAPI.h"
 #include "WinUtils.h"
@@ -296,16 +297,47 @@ bool ProcessedModuleLoadEvent::IsXULLoad
 bool ProcessedModuleLoadEvent::IsTrusted() const {
   if (!mModule) {
     return false;
   }
 
   return mModule->IsTrusted();
 }
 
+void UntrustedModulesData::VerifyConsistency() const {
+#ifdef NIGHTLY_BUILD
+  if (!mIsDiagnosticsAssertEnabled) {
+    return;
+  }
+
+  for (auto& evt : mEvents) {
+    MOZ_DIAGNOSTIC_ASSERT(evt.mModule, "Empty module");
+    MOZ_DIAGNOSTIC_ASSERT(!evt.mModule->mResolvedNtName.IsEmpty(),
+                          "Empty mResolvedNtName");
+    MOZ_DIAGNOSTIC_ASSERT(mModules.Get(evt.mModule->mResolvedNtName, nullptr),
+                          "No match in the table");
+  }
+#endif  // NIGHTLY_BUILD
+}
+
+/* static */
+bool UntrustedModulesData::IsDiagnosticsAssertEnabled() {
+#ifdef NIGHTLY_BUILD
+  // Trigger MOZ_DIAGNOSTIC_ASSERT with a probability of 1/16
+  constexpr double kDiagnosticsAssertRatio = 0.0625;
+
+  constexpr uint64_t kBoundary =
+      std::numeric_limits<uint64_t>::max() * kDiagnosticsAssertRatio;
+  Maybe<uint64_t> randomNum = RandomUint64();
+  return randomNum.isSome() && randomNum.value() <= kBoundary;
+#else
+  return false;
+#endif  // NIGHTLY_BUILD
+}
+
 void UntrustedModulesData::AddNewLoads(
     const ModulesMap& aModules, Vector<ProcessedModuleLoadEvent>&& aEvents,
     Vector<Telemetry::ProcessedStack>&& aStacks) {
   MOZ_ASSERT(aEvents.length() == aStacks.length());
 
   for (auto iter = aModules.ConstIter(); !iter.Done(); iter.Next()) {
     if (iter.Data()->IsTrusted()) {
       // Filter out trusted module records
@@ -314,16 +346,19 @@ void UntrustedModulesData::AddNewLoads(
 
     auto addPtr = mModules.LookupForAdd(iter.Key());
     if (addPtr) {
       // |mModules| already contains this record
       continue;
     }
 
     RefPtr<ModuleRecord> rec(iter.Data());
+    if (mIsDiagnosticsAssertEnabled) {
+      MOZ_DIAGNOSTIC_ASSERT(rec->mResolvedNtName == iter.Key());
+    }
     addPtr.OrInsert([rec = std::move(rec)]() { return rec; });
   }
 
   // This constant matches the maximum in Telemetry::CombinedStacks
   const size_t kMaxEvents = 50;
   MOZ_ASSERT(mEvents.length() <= kMaxEvents);
 
   if (mEvents.length() + aEvents.length() > kMaxEvents) {
--- a/toolkit/xre/UntrustedModulesData.h
+++ b/toolkit/xre/UntrustedModulesData.h
@@ -166,17 +166,18 @@ class ModulesMap final
 };
 
 class UntrustedModulesData final {
  public:
   UntrustedModulesData()
       : mProcessType(XRE_GetProcessType()),
         mPid(::GetCurrentProcessId()),
         mSanitizationFailures(0),
-        mTrustTestFailures(0) {}
+        mTrustTestFailures(0),
+        mIsDiagnosticsAssertEnabled(IsDiagnosticsAssertEnabled()) {}
 
   UntrustedModulesData(UntrustedModulesData&&) = default;
   UntrustedModulesData& operator=(UntrustedModulesData&&) = default;
 
   UntrustedModulesData(const UntrustedModulesData&) = delete;
   UntrustedModulesData& operator=(const UntrustedModulesData&) = delete;
 
   explicit operator bool() const {
@@ -185,25 +186,32 @@ class UntrustedModulesData final {
   }
 
   void AddNewLoads(const ModulesMap& aModulesMap,
                    Vector<ProcessedModuleLoadEvent>&& aEvents,
                    Vector<Telemetry::ProcessedStack>&& aStacks);
 
   void Swap(UntrustedModulesData& aOther);
 
+  void VerifyConsistency() const;
+  static bool IsDiagnosticsAssertEnabled();
+
   GeckoProcessType mProcessType;
   DWORD mPid;
   TimeDuration mElapsed;
   ModulesMap mModules;
   Vector<ProcessedModuleLoadEvent> mEvents;
   Telemetry::CombinedStacks mStacks;
   Maybe<double> mXULLoadDurationMS;
   uint32_t mSanitizationFailures;
   uint32_t mTrustTestFailures;
+
+  // This is not serialized.
+  // Cannot be const as we have the default move ctor.
+  bool mIsDiagnosticsAssertEnabled;
 };
 
 class ModulesMapResult final {
  public:
   ModulesMapResult() : mTrustTestFailures(0) {}
 
   ModulesMapResult(const ModulesMapResult& aOther) = delete;
   ModulesMapResult(ModulesMapResult&& aOther) = default;
@@ -421,16 +429,18 @@ struct ParamTraits<mozilla::ModulePaths>
   }
 };
 
 template <>
 struct ParamTraits<mozilla::UntrustedModulesData> {
   typedef mozilla::UntrustedModulesData paramType;
 
   static void Write(Message* aMsg, const paramType& aParam) {
+    aParam.VerifyConsistency();
+
     aMsg->WriteUInt32(aParam.mProcessType);
     aMsg->WriteULong(aParam.mPid);
     WriteParam(aMsg, aParam.mElapsed);
     WriteParam(aMsg, aParam.mModules);
 
     aMsg->WriteUInt32(aParam.mEvents.length());
     for (auto& evt : aParam.mEvents) {
       WriteEvent(aMsg, evt);
@@ -470,17 +480,17 @@ struct ParamTraits<mozilla::UntrustedMod
     }
 
     if (!aResult->mEvents.resize(eventsLen)) {
       return false;
     }
 
     for (uint32_t curEventIdx = 0; curEventIdx < eventsLen; ++curEventIdx) {
       if (!ReadEvent(aMsg, aIter, &(aResult->mEvents[curEventIdx]),
-                     aResult->mModules)) {
+                     aResult->mModules, aResult->mIsDiagnosticsAssertEnabled)) {
         return false;
       }
     }
 
     if (!ReadParam(aMsg, aIter, &aResult->mStacks)) {
       return false;
     }
 
@@ -518,17 +528,18 @@ struct ParamTraits<mozilla::UntrustedMod
     WriteParam(aMsg, aParam.mModule->mResolvedNtName);
   }
 
   // Because ProcessedModuleLoadEvent depends on a hash table from
   // UntrustedModulesData, we do its deserialization as part of this
   // specialization.
   static bool ReadEvent(const Message* aMsg, PickleIterator* aIter,
                         mozilla::ProcessedModuleLoadEvent* aResult,
-                        const mozilla::ModulesMap& aModulesMap) {
+                        const mozilla::ModulesMap& aModulesMap,
+                        bool aIsDiagnosticsAssertEnabled) {
     if (!aMsg->ReadUInt64(aIter, &aResult->mProcessUptimeMS)) {
       return false;
     }
 
     if (!ReadParam(aMsg, aIter, &aResult->mLoadDurationMS)) {
       return false;
     }
 
@@ -553,16 +564,21 @@ struct ParamTraits<mozilla::UntrustedMod
       return false;
     }
 
     // NB: While bad data integrity might for some reason result in a null
     // mModule, we do not fail the deserialization; this is a data error,
     // rather than an IPC error. The error is detected and dealt with in
     // telemetry.
     aResult->mModule = aModulesMap.Get(resolvedNtName);
+    if (!aResult->mModule && aIsDiagnosticsAssertEnabled) {
+      MOZ_DIAGNOSTIC_ASSERT(aModulesMap.Count() > 0, "Empty module list");
+      MOZ_DIAGNOSTIC_ASSERT(!resolvedNtName.IsEmpty(), "Empty resolvedNtName");
+      MOZ_DIAGNOSTIC_ASSERT(false, "Something else");
+    }
 
     return true;
   }
 };
 
 template <>
 struct ParamTraits<mozilla::ModulesMapResult> {
   typedef mozilla::ModulesMapResult paramType;
--- a/toolkit/xre/UntrustedModulesProcessor.cpp
+++ b/toolkit/xre/UntrustedModulesProcessor.cpp
@@ -376,16 +376,18 @@ RefPtr<UntrustedModulesPromise> Untruste
   if (!mProcessedModuleLoads) {
     return UntrustedModulesPromise::CreateAndResolve(Nothing(), aSource);
   }
 
   result.Swap(mProcessedModuleLoads);
 
   result.mElapsed = TimeStamp::Now() - TimeStamp::ProcessCreation();
 
+  result.VerifyConsistency();
+
   return UntrustedModulesPromise::CreateAndResolve(
       Some(UntrustedModulesData(std::move(result))), aSource);
 }
 
 RefPtr<UntrustedModulesPromise>
 UntrustedModulesProcessor::GetProcessedDataInternalChildProcess() {
   AssertRunningOnLazyIdleThread();
   MOZ_ASSERT(!XRE_IsParentProcess());
@@ -870,16 +872,22 @@ void UntrustedModulesProcessor::Complete
         // continue.
         continue;
       }
 
       if (!mAllowProcessing) {
         return;
       }
 
+      // Trusted modules should have been eliminated by GetModulesTrustInternal
+      // in the browser process
+      if (mProcessedModuleLoads.mIsDiagnosticsAssertEnabled) {
+        MOZ_DIAGNOSTIC_ASSERT(!event.IsTrusted());
+      }
+
       Telemetry::ProcessedStack processedStack =
           stackProcessor.GetStackAndModules(backtrace);
 
       Unused << processedStacks.emplaceBack(std::move(processedStack));
       Unused << processedEvents.emplaceBack(std::move(event));
     }
   }
 
@@ -890,16 +898,19 @@ void UntrustedModulesProcessor::Complete
   }
 
   if (!mAllowProcessing) {
     return;
   }
 
   mProcessedModuleLoads.AddNewLoads(modules, std::move(processedEvents),
                                     std::move(processedStacks));
+
+  mProcessedModuleLoads.VerifyConsistency();
+
   if (maybeXulLoadDuration) {
     MOZ_ASSERT(!mProcessedModuleLoads.mXULLoadDurationMS);
     mProcessedModuleLoads.mXULLoadDurationMS = maybeXulLoadDuration;
   }
 
   mProcessedModuleLoads.mSanitizationFailures += sanitizationFailures;
   mProcessedModuleLoads.mTrustTestFailures += trustTestFailures;
 }