High firefox OOB 🔧 Commit mapped

Overview

High
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impacthigh
DescriptionAn inconsistent comparator in xslt/txNodeSorter could have resulted in potentially exploitable out-of-bounds access. Only affected version 122 and later.
ComponentDOM
Bug ClassOOB
Tracker1944313
Fix commitd674323f853b (firefox) +60/-92
CISA KEVNot listed
CreditedIvan Fratric of Google Project Zero
Disclosed2025-03-04

Changed Functions

FunctionChangeNotes
for
dom/xslt/xslt/txNodeSorter.cpp
modified
if
dom/xslt/xslt/txNodeSorter.cpp
modified
if
dom/xslt/xslt/txXPathResultComparator.cpp
modified

Files Changed

  • dom/xslt/xslt/txNodeSorter.cpp
  • dom/xslt/xslt/txNodeSorter.h
  • dom/xslt/xslt/txXPathResultComparator.cpp
  • dom/xslt/xslt/txXPathResultComparator.h
diff --git a/dom/xslt/xslt/txNodeSorter.cpp b/dom/xslt/xslt/txNodeSorter.cpp
index cc7ac3e3540..2da7871cc04 100644
--- a/dom/xslt/xslt/txNodeSorter.cpp
+++ b/dom/xslt/xslt/txNodeSorter.cpp
@@ -92,8 +92,12 @@ nsresult txNodeSorter::addSortElement(Expr* aSelectExpr, Expr* aLangExpr,
       }
     }
 
-    key->mComparator =
-        new txResultStringComparator(ascending, upperFirst, lang);
+    UniquePtr<txResultStringComparator> comparator =
+        MakeUnique<txResultStringComparator>(ascending, upperFirst);
+    rv = comparator->init(lang);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    key->mComparator = comparator.release();
   } else if (TX_StringEqualsAtom(dataType, nsGkAtoms::number)) {
     // Number comparator
     key->mComparator = new txResultNumberComparator(ascending);
@@ -134,7 +138,7 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
 
   nsTArray<uint32_t> indexes(len.value());
   indexes.SetLengthAndRetainStorage(len.value());
-  nsTArray<txObject*> sortValues(numSortValues.value());
+  nsTArray<UniquePtr<txObject>> sortValues(numSortValues.value());
   sortValues.SetLengthAndRetainStorage(numSortValues.value());
   // txObject* has no null initializing constructor, so we init manually.
   memset(sortValues.Elements(), 0, sortValuesSize.value());
@@ -147,11 +151,7 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
   auto nodeSetContext = MakeUnique<txNodeSetContext>(aNodes, aEs);
 
   // Sort the indexarray
-  SortData sortData{};
-  sortData.mNodeSorter = this;
-  sortData.mContext = nodeSetContext.get();
-  sortData.mSortValues = sortValues.Elements();
-  sortData.mRv = NS_OK;
+  SortData sortData{this, nodeSetContext.get(), sortValues.Elements(), NS_OK};
 
   aEs->pushEvalContext(nodeSetContext.release());
 
@@ -159,12 +159,6 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
     return compareNodes(left, right, sortData);
   });
 
-  // Delete these here so we don't have to deal with them at every possible
-  // failurepoint
-  for (i = 0; i < numSortValues.value(); ++i) {
-    delete sortValues[i];
-  }
-
   if (NS_FAILED(sortData.mRv)) {
     // The txExecutionState owns the evalcontext so no need to handle it
     return sortData.mRv;
@@ -188,12 +182,10 @@ nsresult txNodeSorter::sortNodeSet(txNodeSet* aNodes, txExecutionState* aEs,
 
 int txNodeSorter::compareNodes(uint32_t aIndexA, uint32_t aIndexB,
                                SortData& aSortData) {
-  NS_ENSURE_SUCCESS(aSortData.mRv, -1);
-
   txListIterator iter(&aSortData.mNodeSorter->mSortKeys);
-  txObject** sortValuesA =
+  UniquePtr<txObject>* sortValuesA =
       aSortData.mSortValues + aIndexA * aSortData.mNodeSorter->mNKeys;
-  txObject** sortValuesB =
+  UniquePtr<txObject>* sortValuesB =
       aSortData.mSortValues + aIndexB * aSortData.mNodeSorter->mNKeys;
 
   unsigned int i;
@@ -201,19 +193,19 @@ int txNodeSorter::compareNodes(uint32_t aIndexA, uint32_t aIndexB,
   for (i = 0; i < aSortData.mNodeSorter->mNKeys; ++i) {
     SortKey* key = (SortKey*)iter.next();
     // Lazy create sort values
-    if (!sortValuesA[i] &&
-        !calcSortValue(sortValuesA[i], key, &aSortData, aIndexA)) {
-      return -1;
+    if (!sortValuesA[i]) {
+      sortValuesA[i] = calcSortValue(key, &aSortData, aIndexA);
     }
-    if (!sortValuesB[i] &&
-        !calcSortValue(sortValuesB[i], key, &aSortData, aIndexB)) {
-      return 1;
+    if (!sortValuesB[i]) {
+      sortValuesB[i] = calcSortValue(key, &aSortData, aIndexB);
     }
 
     // Compare node values
-    int compRes =
-        key->mComparator->compareValues(sortValuesA[i], sortValuesB[i]);
-    if (compRes != 0) return compRes;
+    int compRes = key->mComparator->compareValues(sortValuesA[i].get(),
+                                                  sortValuesB[i].get());
+    if (compRes != 0) {
+      return compRes;
+    }
   }
 
   // All keys have the same value for these nodes.
@@ -221,16 +213,18 @@ int txNodeSorter::compareNodes(uint32_t aIndexA, uint32_t aIndexB,
 }
 
 // static
-bool txNodeSorter::calcSortValue(txObject*& aSortValue, SortKey* aKey,
-                                 SortData* aSortData, uint32_t aNodeIndex) {
+UniquePtr<txObject> txNodeSorter::calcSortValue(SortKey* aKey,
+                                                SortData* aSortData,
+                                                uint32_t aNodeIndex) {
   aSortData->mContext->setPosition(aNodeIndex + 1);  // position is 1-based
 
-  nsresult rv = aKey->mComparator->createSortableValue(
-      aKey->mExpr, aSortData->mContext, aSortValue);
-  if (NS_FAILED(rv)) {
+  UniquePtr<txObject> sortValue;
+  nsresult rv;
+  std::tie(sortValue, rv) =
+      aKey->mComparator->createSortableValue(aKey->mExpr, aSortData->mContext);
+  if (NS_FAILED(rv) && NS_SUCCEEDED(aSortData->mRv)) {
+    // Record the first failure.
     aSortData->mRv = rv;
-    return false;
   }
-
-  return true;
+  return sortValue;
 }
diff --git a/dom/xslt/xslt/txNodeSorter.h b/dom/xslt/xslt/txNodeSorter.h
index b6c883b6ea5..3ab74763815 100644
--- a/dom/xslt/xslt/txNodeSorter.h
+++ b/dom/xslt/xslt/txNodeSorter.h
@@ -36,7 +36,7 @@ class txNodeSorter {
   struct SortData {
     txNodeSorter* mNodeSorter;
     txNodeSetContext* mContext;
-    txObject** mSortValues;
+    mozilla::UniquePtr<txObject>* mSortValues;
     nsresult mRv;
   };
   struct SortKey {
@@ -46,8 +46,9 @@ class txNodeSorter {
 
   static int compareNodes(uint32_t aIndexA, uint32_t aIndexB,
                           SortData& aSortData);
-  static bool calcSortValue(txObject*& aSortValue, SortKey* aKey,
-                            SortData* aSortData, uint32_t aNodeIndex);
+  static mozilla::UniquePtr<txObject> calcSortValue(SortKey* aKey,
+                                                    SortData* aSortData,
+                                                    uint32_t aNodeIndex);
   txList mSortKeys;
   unsigned int mNKeys;
 };
diff --git a/dom/xslt/xslt/txXPathResultComparator.cpp b/dom/xslt/xslt/txXPathResultComparator.cpp
index 470a55dc503..0331dac2913 100644
--- a/dom/xslt/xslt/txXPathResultComparator.cpp
+++ b/dom/xslt/xslt/txXPathResultComparator.cpp
@@ -19,13 +19,10 @@ using Collator = mozilla::intl::Collator;
 #define kUpperFirst (1 << 1)
 
 txResultStringComparator::txResultStringComparator(bool aAscending,
-                                                   bool aUpperFirst,
-                                                   const nsString& aLanguage) {
+                                                   bool aUpperFirst) {
   mSorting = 0;
   if (aAscending) mSorting |= kAscending;
   if (aUpperFirst) mSorting |= kUpperFirst;
-  nsresult rv = init(aLanguage);
-  if (NS_FAILED(rv)) NS_ERROR("Failed to initialize txResultStringComparator");
 }
 
 nsresult txResultStringComparator::init(const nsString& aLanguage) {
@@ -49,61 +46,35 @@ nsresult txResultStringComparator::init(const nsString& aLanguage) {
   return NS_OK;
 }
 
-nsresult txResultStringComparator::createSortableValue(Expr* aExpr,
-                                                       txIEvalContext* aContext,
-                                                       txObject*& aResult) {
-  UniquePtr<StringValue> val(new StringValue);
-
-  if (!mCollator) {
-    return NS_ERROR_FAILURE;
-  }
-
-  val->mString = MakeUnique<nsString>();
-  nsString& string = *val->mString;
-  nsresult rv = aExpr->evaluateToString(aContext, string);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  aResult = val.release();
-
-  return NS_OK;
+std::pair<UniquePtr<txObject>, nsresult>
+txResultStringComparator::createSortableValue(Expr* aExpr,
+                                              txIEvalContext* aContext) {
+  UniquePtr<nsString> string = MakeUnique<nsString>();
+  nsresult rv = aExpr->evaluateToString(aContext, *string);
+  return std::make_pair(MakeUnique<StringValue>(std::move(string)), rv);
 }
 
 int txResultStringComparator::compareValues(txObject* aVal1, txObject* aVal2) {
   nsString& dval1 = *((StringValue*)aVal1)->mString;
Loading diff…