Foxhound: fixing memory leaks and crashses

The fix to clean up all the StringTaint from Nursery strings
wasn't quite thorough enough and left some dangling pointers
lying around. This fix makes sure we catch all the Nursery
allocated Strings by moving String registration to Allocator-inl.h

This fix revealed the root cause of the segmentation faults which
were occuring on client-heavy webapps (UI5 I'm looking at you).
There were a couple of occasions where strings were being created
(but not rooted) and then additional strings constructed for
TaintOperation arguments (e.g. JSONParser.cpp and RegExp.cpp).
As the TaintOperations were trying to allocate memory, this
could cause a GC which would clean up the original strings.

This fix makes sure that the TaintOperations are created before
the new Strings or that they are properly rooted.

Author: tmbrbr

js/src/builtin/RegExp.cpp

@@ -109,6 +109,9 @@ bool js::CreateRegExpMatchResult(JSContext* cx, HandleRegExpShared re,
     return false;
   }
 
+  Rooted<JSAtom*> src(cx, re->getSource());
+  RootedString srcStr(cx, EscapeRegExpPattern(cx, src));
+
   // Steps 28-29 and 33 a-d: Initialize the elements of the match result.
   // Store a Value for each match pair.
   for (size_t i = 0; i < numPairs; i++) {
@@ -125,10 +128,9 @@ bool js::CreateRegExpMatchResult(JSContext* cx, HandleRegExpShared re,
         return false;
       }
       // Taintfox: taint propagated by NewDependentString, just need
-      // to add the operation here
+      // to add the operation here. Do this after adding to the rooted
+      // array to avoid GC issues.
       if (str->taint().hasTaint()) {
-        Rooted<JSAtom*> src(cx, re->getSource());
-        JSString* srcStr = EscapeRegExpPattern(cx, src);
         str->taint().extend(
           TaintOperation("RegExp.prototype.exec", true, TaintLocationFromContext(cx),
                          { taintarg_jsstring_full(cx, srcStr), taintarg_jsstring(cx, str), taintarg(cx, i) }));

js/src/gc/Allocator-inl.h

@@ -69,6 +69,8 @@ T* CellAllocator::NewString(JSContext* cx, gc::Heap heap, Args&&... args) {
     return nullptr;
   }
 
+  JSString* str = reinterpret_cast<JSString*>(ptr);
+  JSString::registerNurseryString(cx, str);
   return new (mozilla::KnownNotNull, ptr) T(std::forward<Args>(args)...);
 }
 

js/src/jsapi.cpp

@@ -4953,7 +4953,7 @@ JS_ReportTaintSink(JSContext* cx, JS::HandleString str, const char* sink, JS::Ha
   // Print a message to stdout. Also include the current JS backtrace.
   auto& firstRange = *str->taint().begin();
 
-  // std::cerr << "!!! Tainted flow into " << sink << " from " << firstRange.flow().source().name() << " !!!" << std::endl;
+  std::cerr << "!!! Tainted flow into " << sink << " from " << firstRange.flow().source().name() << " !!!" << std::endl;
   // DumpBacktrace(cx);
 
   // Report a warning to show up on the web console

js/src/vm/JSONParser.cpp

@@ -690,7 +690,7 @@ JSString* JSONFullParseHandlerAnyChar::CurrentJsonPath(const Vector<StackEntry,
       }
     }
   }
-  return builder.finishAtom();
+  return builder.finishString(gcHeap);
 }
 
 inline void JSONFullParseHandlerAnyChar::setNumberValue(double d) {
@@ -703,6 +703,14 @@ inline bool JSONFullParseHandler<CharT>::setStringValue(CharPtr start,
                                                         size_t length,
                                                         const StringTaint& taint,
                                                         const ParserT* parser) {
+  TaintOperation op("JSON.parse");
+  // TaintFox: propagate taint.
+  if (ST != JSONStringType::PropertyName && taint.hasTaint()) {
+    JSString* path = parser->CurrentJsonPath();
+    RootedString jsonPath(cx, path);
+    op = TaintOperationFromContextJSString(cx, "JSON.parse", true, jsonPath);
+  }
+
   JSString* str;
   if constexpr (ST == JSONStringType::PropertyName) {
     str = AtomizeChars(cx, start.get(), length);
@@ -717,8 +725,6 @@ inline bool JSONFullParseHandler<CharT>::setStringValue(CharPtr start,
   // TaintFox: propagate taint.
   if (ST != JSONStringType::PropertyName && taint.hasTaint()) {
     str->setTaint(cx, taint);
-    RootedString jsonPath(cx, parser->CurrentJsonPath());
-    TaintOperation op = TaintOperationFromContextJSString(cx, "JSON.parse", true, jsonPath);
     str->taint().extend(op);
   }
 
@@ -730,6 +736,15 @@ template <typename CharT>
 template <JSONStringType ST, typename ParserT>
 inline bool JSONFullParseHandler<CharT>::setStringValue(
     StringBuilder& builder, const ParserT* parser) {
+
+  TaintOperation op("JSON.parse");
+  // TaintFox: propagate taint.
+  if (ST != JSONStringType::PropertyName && builder.buffer.taint()) {
+    JSString* path = parser->CurrentJsonPath();
+    RootedString jsonPath(cx, path);
+    op = TaintOperationFromContextJSString(cx, "JSON.parse", true, jsonPath);
+  }
+
   JSString* str;
   if constexpr (ST == JSONStringType::PropertyName) {
     str = builder.buffer.finishAtom();
@@ -743,8 +758,6 @@ inline bool JSONFullParseHandler<CharT>::setStringValue(
 
   // TaintFox: Add taint operation.
   if (str->taint().hasTaint()) {
-    RootedString jsonPath(cx, parser->CurrentJsonPath());
-    TaintOperation op = TaintOperationFromContextJSString(cx, "JSON.parse", true, jsonPath);
     str->taint().extend(op);
   }
 

js/src/vm/StringType-inl.h

@@ -262,9 +262,7 @@ MOZ_ALWAYS_INLINE JSRope* JSRope::new_(
   if (MOZ_UNLIKELY(!validateLengthInternal<allowGC>(cx, length))) {
     return nullptr;
   }
-  JSRope* str = cx->newCell<JSRope, allowGC>(heap, left, right, length);
-  registerNurseryString(cx, str);
-  return str;
+  return cx->newCell<JSRope, allowGC>(heap, left, right, length);
 }
 
 inline JSDependentString::JSDependentString(JSLinearString* base, size_t start,
@@ -318,14 +316,11 @@ MOZ_ALWAYS_INLINE JSLinearString* JSDependentString::new_(
   JSDependentString* str =
       cx->newCell<JSDependentString, js::NoGC>(heap, baseArg, start, length, &taint);
   if (str) {
-    registerNurseryString(cx, str);
     return str;
   }
 
   JS::Rooted<JSLinearString*> base(cx, baseArg);
-  str = cx->newCell<JSDependentString>(heap, base, start, length, &taint);
-  registerNurseryString(cx, str);
-  return str;
+  return cx->newCell<JSDependentString>(heap, base, start, length, &taint);
 }
 
 inline JSLinearString::JSLinearString(const char16_t* chars, size_t length) {
@@ -362,9 +357,7 @@ MOZ_ALWAYS_INLINE JSLinearString* JSLinearString::new_(
     return nullptr;
   }
 
-  JSLinearString* str = newValidLength<allowGC>(cx, std::move(chars), length, heap);
-  registerNurseryString(cx, str);
-  return str;
+  return newValidLength<allowGC>(cx, std::move(chars), length, heap);
 }
 
 template <js::AllowGC allowGC, typename CharT>
@@ -440,18 +433,14 @@ template <js::AllowGC allowGC>
 MOZ_ALWAYS_INLINE JSThinInlineString* JSThinInlineString::new_(
     JSContext* cx, js::gc::Heap heap) {
   MOZ_ASSERT(!cx->zone()->isAtomsZone());
-  JSThinInlineString* str = cx->newCell<JSThinInlineString, allowGC>(heap);
-  registerNurseryString(cx, str);
-  return str;
+  return cx->newCell<JSThinInlineString, allowGC>(heap);
 }
 
 template <js::AllowGC allowGC>
 MOZ_ALWAYS_INLINE JSFatInlineString* JSFatInlineString::new_(
     JSContext* cx, js::gc::Heap heap) {
   MOZ_ASSERT(!cx->zone()->isAtomsZone());
-  JSFatInlineString* str = cx->newCell<JSFatInlineString, allowGC>(heap);
-  registerNurseryString(cx, str);
-  return str;
+  return cx->newCell<JSFatInlineString, allowGC>(heap);
 }
 
 inline JSThinInlineString::JSThinInlineString(size_t length,
@@ -523,7 +512,6 @@ MOZ_ALWAYS_INLINE JSExternalString* JSExternalString::new_(
   MOZ_ASSERT(str->isTenured());
   js::AddCellMemory(str, nbytes, js::MemoryUse::StringContents);
 
-  registerNurseryString(cx, str);
   return str;
 }
 
@@ -681,4 +669,23 @@ inline void JSExternalString::finalize(JS::GCContext* gcx) {
   clearTaint();
 }
 
+/* static */
+void JSString::registerNurseryString(JSContext* cx, JSString* str) {
+  if (!str) {
+    return;
+  }
+
+  if (!IsInsideNursery(str)) {
+    return;
+  }
+
+  if (!cx->nursery().addStringWithNurseryMemory(str)) {
+    ReportOutOfMemory(cx);
+    return;
+  }
+
+  return;
+}
+
+
 #endif /* vm_StringType_inl_h */

js/src/vm/StringType.cpp

@@ -367,6 +367,12 @@ void JSString::dumpRepresentation() const {
   out.putChar('\n');
 }
 
+void JSString::dumpRepresentationHeader() const {
+  js::Fprinter out(stderr);
+  dumpRepresentationHeader(out, 0);
+  out.putChar('\n');
+}
+
 void JSString::dumpRepresentation(js::GenericPrinter& out, int indent) const {
   if (isRope()) {
     asRope().dumpRepresentation(out, indent);
@@ -392,6 +398,7 @@ void JSString::dumpRepresentationHeader(js::GenericPrinter& out,
   // copy-and-paste into a debugger.
   out.printf("((%s*) %p) length: %zu  flags: 0x%x", subclass, this, length(),
              flags);
+  if (isForwarded()) out.put(" FORWARDED");
   if (flags & LINEAR_BIT) out.put(" LINEAR");
   if (flags & DEPENDENT_BIT) out.put(" DEPENDENT");
   if (flags & INLINE_CHARS_BIT) out.put(" INLINE_CHARS");
@@ -404,7 +411,7 @@ void JSString::dumpRepresentationHeader(js::GenericPrinter& out,
   if (!isAtom() && inStringToAtomCache()) out.put(" IN_STRING_TO_ATOM_CACHE");
   if (flags & INDEX_VALUE_BIT) out.printf(" INDEX_VALUE(%u)", getIndexValue());
   if (!isTenured()) out.put(" NURSERY");
-  if (!isTainted()) out.put(" TAINTED");
+  if (isTainted()) out.put(" TAINTED");
   out.putChar('\n');
 }
 
@@ -2387,21 +2394,24 @@ void JSString::sweepAfterMinorGC(JS::GCContext* gcx, JSString* str) {
   }
 
   if (IsInsideNursery(str) && !IsForwarded(str) && str->isTainted()) {
+#ifdef TAINT_DEBUG_NURSERY
+    printf("-----------------------------------------------------\n");
+    printf("Str: %p\n", str);
+    str->dumpRepresentationHeader();
+#endif
+    auto* ptr = reinterpret_cast<uint8_t*>(str) + offsetOfTaint();
+#ifdef TAINT_DEBUG_NURSERY
+    printf("Ptr: %p\n", ptr);
+    printf("Before: %p\n", *reinterpret_cast<void**>(ptr));
+#endif
     str->clearTaint();
+#ifdef TAINT_DEBUG_NURSERY
+    printf("After Clear: %p\n", *reinterpret_cast<void**>(ptr));
+#endif
+    AlwaysPoison(ptr, 0x7A, sizeof(StringTaint), MemCheckKind::MakeNoAccess);
+#ifdef TAINT_DEBUG_NURSERY
+    printf("After Poison: %p\n", *reinterpret_cast<void**>(ptr));
+    printf("-----------------------------------------------------\n");
+#endif
   }
 }
-
-/* static */
-void JSString::registerNurseryString(JSContext* cx, JSString* str) {
-  if (!str) {
-    return;
-  }
-
-  if (!IsInsideNursery(str)) {
-    return;
-  }
-
-  if (!cx->nursery().addStringWithNurseryMemory(str)) {
-    ReportOutOfMemory(cx);
-  }
-}

js/src/vm/StringType.h

@@ -675,7 +675,7 @@ class JSString : public js::gc::CellWithLengthAndFlags {
   static void sweepAfterMinorGC(JS::GCContext* gcx, JSString* str);
 
   /* Taintfox: register string in nursery */
-  static void registerNurseryString(JSContext* cx, JSString* str);
+  static inline void registerNurseryString(JSContext* cx, JSString* str);
 
  private:
   // To help avoid writing Spectre-unsafe code, we only allow MacroAssembler
@@ -740,6 +740,7 @@ class JSString : public js::gc::CellWithLengthAndFlags {
   void dumpRepresentation(js::GenericPrinter& out, int indent) const;
   void dumpRepresentationHeader(js::GenericPrinter& out,
                                 const char* subclass) const;
+  void dumpRepresentationHeader() const;
   void dumpCharsNoQuote(js::GenericPrinter& out);
 
   template <typename CharT>