Medium firefox Memory Corruption 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactmoderate
DescriptionThe JavaScript garbage collector could mis-color cross-compartment objects if OOM conditions were detected at the right point between two passes. This could have led to memory corruption.
ComponentSpiderMonkey
Bug ClassMemory Corruption
Tracker1911288
Fix commita107c5b58903 (firefox) +55/-21
CISA KEVNot listed
Creditedthe Mozilla Fuzzing Team
Disclosed2024-09-03

Changed Functions

FunctionChangeNotes
MakeScopeExit
js/src/gc/Sweeping.cpp
modified
if
js/src/gc/Sweeping.cpp
modified
oomTest
js/src/jit-test/tests/gc/bug-1911288.js
modified
if
js/src/vm/Compartment.cpp
modified
switch
js/src/vm/Compartment.cpp
modified

Files Changed

  • js/src/gc/Sweeping.cpp
  • js/src/gc/Verifier.cpp
  • js/src/jit-test/tests/gc/bug-1911288.js
  • js/src/vm/Compartment.cpp
  • js/src/vm/Compartment.h
diff --git a/js/src/gc/Sweeping.cpp b/js/src/gc/Sweeping.cpp
index 4175fc68be2..2d32900fc02 100644
--- a/js/src/gc/Sweeping.cpp
+++ b/js/src/gc/Sweeping.cpp
@@ -566,23 +566,32 @@ IncrementalProgress GCRuntime::markWeakReferencesInCurrentGroup(
 
 IncrementalProgress GCRuntime::markGrayRoots(SliceBudget& budget,
                                              gcstats::PhaseKind phase) {
-  MOZ_ASSERT(marker().markColor() == MarkColor::Gray);
+  MOZ_ASSERT(marker().markColor() == MarkColor::Black);
 
   gcstats::AutoPhase ap(stats(), phase);
 
-  AutoUpdateLiveCompartments updateLive(this);
-  marker().setRootMarkingMode(true);
-  auto guard =
-      mozilla::MakeScopeExit([this]() { marker().setRootMarkingMode(false); });
+  {
+    AutoSetMarkColor setColorGray(marker(), MarkColor::Gray);
 
-  IncrementalProgress result =
-      traceEmbeddingGrayRoots(marker().tracer(), budget);
-  if (result == NotFinished) {
-    return NotFinished;
+    AutoUpdateLiveCompartments updateLive(this);
+    marker().setRootMarkingMode(true);
+    auto guard = mozilla::MakeScopeExit(
+        [this]() { marker().setRootMarkingMode(false); });
+
+    IncrementalProgress result =
+        traceEmbeddingGrayRoots(marker().tracer(), budget);
+    if (result == NotFinished) {
+      return NotFinished;
+    }
+
+    Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
+        marker().tracer(), Compartment::GrayEdges);
   }
 
+  // Also mark any incoming cross compartment edges that were originally gray
+  // but have been marked black by a barrier.
   Compartment::traceIncomingCrossCompartmentEdgesForZoneGC(
-      marker().tracer(), Compartment::GrayEdges);
+      marker().tracer(), Compartment::BlackEdges);
 
   return Finished;
 }
@@ -1144,8 +1153,6 @@ IncrementalProgress GCRuntime::markGrayRootsInCurrentGroup(
     JS::GCContext* gcx, SliceBudget& budget) {
   gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::MARK);
 
-  AutoSetMarkColor setColorGray(marker(), MarkColor::Gray);
-
   // Check that the zone state is set correctly for the current sweep group as
   // that determines what gets marked.
   MOZ_ASSERT(atomsZone()->wasGCStarted() ==
diff --git a/js/src/gc/Verifier.cpp b/js/src/gc/Verifier.cpp
index f139d542325..a2a330c3595 100644
--- a/js/src/gc/Verifier.cpp
+++ b/js/src/gc/Verifier.cpp
@@ -607,9 +607,13 @@ void js::gc::MarkingValidator::nonIncrementalMark(AutoGCSession& session) {
       zone->changeGCState(zone->initialMarkingState(), Zone::MarkBlackAndGray);
     }
 
-    AutoSetMarkColor setColorGray(*gcmarker, MarkColor::Gray);
-
+    /*
+     * markAllGrayReferences may mark both gray and black, so it manages the
+     * mark color internally.
+     */
     gc->markAllGrayReferences(gcstats::PhaseKind::MARK_GRAY);
+
+    AutoSetMarkColor setColorGray(*gcmarker, MarkColor::Gray);
     gc->markAllWeakReferences();
 
     /* Restore zone state. */
diff --git a/js/src/jit-test/tests/gc/bug-1911288.js b/js/src/jit-test/tests/gc/bug-1911288.js
new file mode 100644
index 00000000000..335d6adb575
--- /dev/null
+++ b/js/src/jit-test/tests/gc/bug-1911288.js
@@ -0,0 +1,18 @@
+Object.defineProperty(this, "x", {});
+try {
+  oomTest(function() {
+      let m = parseModule(``);
+  });
+  var dbg = new Debugger;
+  dbg.onNewGlobalObject = function (global) {}
+  let t39 = {};
+  addMarkObservers([t39]);
+  let g33 = newGlobal({newCompartment: true});
+  g33.t39 = t39;
+  g33.eval("grayRoot().push(t);");
+} catch (exc) {}
+gcparam("allocationThreshold", 1 /* MB */);
+gcparam("incrementalGCEnabled", false);
+oomTest(function() {
+  var lfGlobal = newGlobal({sameZoneAs: this});
+});
diff --git a/js/src/vm/Compartment.cpp b/js/src/vm/Compartment.cpp
index 75008f0bfd5..45857b197a4 100644
--- a/js/src/vm/Compartment.cpp
+++ b/js/src/vm/Compartment.cpp
@@ -485,13 +485,18 @@ bool Compartment::wrap(JSContext* cx, MutableHandle<GCVector<Value>> vec) {
 
 static inline bool ShouldTraceWrapper(JSObject* wrapper,
                                       Compartment::EdgeSelector whichEdges) {
-  if (whichEdges == Compartment::AllEdges) {
-    return true;
+  switch (whichEdges) {
+    case Compartment::AllEdges:
+      return true;
+    case Compartment::NonGrayEdges:
+      return !wrapper->isMarkedGray();
+    case Compartment::GrayEdges:
+      return wrapper->isMarkedGray();
+    case Compartment::BlackEdges:
+      return wrapper->isMarkedBlack();
+    default:
+      MOZ_CRASH("Unexpected EdgeSelector value");
   }
-
-  bool isGray = wrapper->isMarkedGray();
-  return (whichEdges == Compartment::NonGrayEdges && !isGray) ||
-         (whichEdges == Compartment::GrayEdges && isGray);
 }
 
 void Compartment::traceWrapperTargetsInCollectedZones(JSTracer* trc,
diff --git a/js/src/vm/Compartment.h b/js/src/vm/Compartment.h
index 6cea7a7c3db..34aaf2dd00c 100644
--- a/js/src/vm/Compartment.h
+++ b/js/src/vm/Compartment.h
@@ -417,7 +417,7 @@ class JS::Compartment {
    * dangling (full GCs naturally follow pointers across compartments) and
    * when compacting to update cross-compartment pointers.
    */
-  enum EdgeSelector { AllEdges, NonGrayEdges, GrayEdges };
+  enum EdgeSelector { AllEdges, NonGrayEdges, GrayEdges, BlackEdges };
   void traceWrapperTargetsInCollectedZones(JSTracer* trc,
                                            EdgeSelector whichEdges);
   static void traceIncomingCrossCompartmentEdgesForZoneGC(
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/js/src/jit-test/tests/gc/bug-1911288.js b/js/src/jit-test/tests/gc/bug-1911288.js
new file mode 100644
index 00000000000..335d6adb575
--- /dev/null
+++ b/js/src/jit-test/tests/gc/bug-1911288.js
@@ -0,0 +1,18 @@
+Object.defineProperty(this, "x", {});
+try {
+  oomTest(function() {
+      let m = parseModule(``);
+  });
+  var dbg = new Debugger;
+  dbg.onNewGlobalObject = function (global) {}
+  let t39 = {};
+  addMarkObservers([t39]);
+  let g33 = newGlobal({newCompartment: true});
+  g33.t39 = t39;
+  g33.eval("grayRoot().push(t);");
+} catch (exc) {}
+gcparam("allocationThreshold", 1 /* MB */);
+gcparam("incrementalGCEnabled", false);
+oomTest(function() {
+  var lfGlobal = newGlobal({sameZoneAs: this});
+});
Loading diff…