From a500e7d588aea3abe4e924567d3108543ae9b45b Mon Sep 17 00:00:00 2001 From: Thomas Barber Date: Fri, 23 Feb 2024 08:52:34 +0000 Subject: [PATCH 1/3] Foxhound: Fixing GC crashes due to JSONParser strings The CurrentJsonPath method was causing tab crashes when parsing large JSON files. These seem to occur when the JSON parser was running at the same time as Nursery Strings were moved to Tenured space. My feeling is that the JSONPath strings were being created faster than they could be moved out of the nursery, causing some nasty segfaults. A quick for this appears to be to create the JSONPath strings directly in the Tenured heap. This is probably not ideal, as these strings are only used to create TaintOperations. This is probably worth revisiting to see if there is a better solution. There seems to be a lot of upcoming upstream changes related to String GC, so probably worth waiting for the next release (see e.g. 7e2b43ba2a92968f752a60f90448ea47b75bf66e) --- js/src/vm/JSONParser.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/js/src/vm/JSONParser.cpp b/js/src/vm/JSONParser.cpp index 945e1edecda70..9c6b0c5d70281 100644 --- a/js/src/vm/JSONParser.cpp +++ b/js/src/vm/JSONParser.cpp @@ -690,7 +690,7 @@ JSString* JSONFullParseHandlerAnyChar::CurrentJsonPath(const Vector::setStringValue(CharPtr start, // TaintFox: propagate taint. if (ST != JSONStringType::PropertyName && taint.hasTaint()) { str->setTaint(cx, taint); - TaintOperation op = parser ? - TaintOperationFromContextJSString(cx, "JSON.parse", true, parser->CurrentJsonPath()) : + JSString* jsonPath = parser ? parser->CurrentJsonPath() : nullptr; + TaintOperation op = jsonPath ? + TaintOperationFromContextJSString(cx, "JSON.parse", true, jsonPath) : TaintOperationFromContext(cx, "JSON.parse", true); str->taint().extend(op); } @@ -744,8 +745,9 @@ inline bool JSONFullParseHandler::setStringValue( // TaintFox: Add taint operation. if (str->taint().hasTaint()) { - TaintOperation op = parser ? - TaintOperationFromContextJSString(cx, "JSON.parse", true, parser->CurrentJsonPath()) : + JSString* jsonPath = parser ? parser->CurrentJsonPath() : nullptr; + TaintOperation op = jsonPath ? + TaintOperationFromContextJSString(cx, "JSON.parse", true, jsonPath) : TaintOperationFromContext(cx, "JSON.parse", true); str->taint().extend(op); } From ba6e38b5ef761d8c6e549ea8744a0069c2e59bc2 Mon Sep 17 00:00:00 2001 From: Thomas Barber Date: Fri, 23 Feb 2024 08:58:59 +0000 Subject: [PATCH 2/3] Foxhound: Enable String dump with DEBUG_TAINT --- js/src/vm/StringType.cpp | 10 +++++----- js/src/vm/StringType.h | 16 ++++++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/js/src/vm/StringType.cpp b/js/src/vm/StringType.cpp index 0438e57632063..c78d99ccb516b 100644 --- a/js/src/vm/StringType.cpp +++ b/js/src/vm/StringType.cpp @@ -254,7 +254,7 @@ mozilla::Maybe > JSString::encodeUTF8Partial( return mozilla::Some(std::make_tuple(totalRead, totalWritten)); } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) template /*static */ @@ -556,7 +556,7 @@ bool JSRope::hash(uint32_t* outHash) const { return true; } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void JSRope::dumpRepresentation(js::GenericPrinter& out, int indent) const { dumpRepresentationHeader(out, "JSRope"); indent += 2; @@ -1067,7 +1067,7 @@ static inline void FillFromCompatible(unsigned char* dest, const char16_t* src, AsWritableChars(Span(dest, length))); } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void JSDependentString::dumpRepresentation(js::GenericPrinter& out, int indent) const { dumpRepresentationHeader(out, "JSDependentString"); @@ -1512,7 +1512,7 @@ bool JS::SourceText::initMaybeBorrowed( return initImpl(fc, chars, length, taint, ownership); } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void JSAtom::dump(js::GenericPrinter& out) { out.printf("JSAtom* (%p) = ", (void*)this); this->JSString::dump(out); @@ -1972,7 +1972,7 @@ JSString* NewMaybeExternalString(JSContext* cx, const char16_t* s, size_t n, } /* namespace js */ -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void JSExtensibleString::dumpRepresentation(js::GenericPrinter& out, int indent) const { dumpRepresentationHeader(out, "JSExtensibleString"); diff --git a/js/src/vm/StringType.h b/js/src/vm/StringType.h index 26de929213ef5..c18c59be127e8 100644 --- a/js/src/vm/StringType.h +++ b/js/src/vm/StringType.h @@ -726,7 +726,7 @@ class JSString : public js::gc::CellWithLengthAndFlags { return kind; } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dump(); // Debugger-friendly stderr dump. void dump(js::GenericPrinter& out); void dumpNoNewline(js::GenericPrinter& out); @@ -834,7 +834,7 @@ class JSRope : public JSString { void traceChildren(JSTracer* trc); -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dumpRepresentation(js::GenericPrinter& out, int indent) const; #endif @@ -1011,7 +1011,7 @@ class JSLinearString : public JSString { inline void finalize(JS::GCContext* gcx); inline size_t allocSize() const; -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dumpRepresentationChars(js::GenericPrinter& out, int indent) const; void dumpRepresentation(js::GenericPrinter& out, int indent) const; #endif @@ -1063,7 +1063,7 @@ class JSDependentString : public JSLinearString { setNonInlineChars(chars + offset); } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dumpRepresentation(js::GenericPrinter& out, int indent) const; #endif @@ -1092,7 +1092,7 @@ class JSExtensibleString : public JSLinearString { return d.s.u3.capacity; } -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dumpRepresentation(js::GenericPrinter& out, int indent) const; #endif }; @@ -1119,7 +1119,7 @@ class JSInlineString : public JSLinearString { template static bool lengthFits(size_t length); -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dumpRepresentation(js::GenericPrinter& out, int indent) const; #endif @@ -1252,7 +1252,7 @@ class JSExternalString : public JSLinearString { // kind. inline void finalize(JS::GCContext* gcx); -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dumpRepresentation(js::GenericPrinter& out, int indent) const; #endif }; @@ -1316,7 +1316,7 @@ class JSAtom : public JSLinearString { inline js::HashNumber hash() const; inline void initHash(js::HashNumber hash); -#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) +#if defined(DEBUG) || defined(JS_JITSPEW) || defined(JS_CACHEIR_SPEW) || defined(TAINT_DEBUG) void dump(js::GenericPrinter& out); void dump(); #endif From 991a5c5ed143cd5a003e60fa1322a510ff9f3a06 Mon Sep 17 00:00:00 2001 From: Thomas Barber Date: Fri, 23 Feb 2024 08:59:26 +0000 Subject: [PATCH 3/3] Foxhound: Fixing memory leak due to untidy StringTaints --- dom/base/Element.cpp | 2 +- parser/html/nsHtml5StreamParser.cpp | 4 ++-- parser/html/nsHtml5TreeBuilder.cpp | 1 + parser/html/nsHtml5UTF16Buffer.h | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp index 627fa6d29f505..f2c37c5179c3f 100644 --- a/dom/base/Element.cpp +++ b/dom/base/Element.cpp @@ -5055,7 +5055,7 @@ void Element::TaintSelectorOperation(const char* operation, const nsAString& aEl // Here we want to save a list of all selector operations performed on the element // Check if there is a direct flow - const StringTaint aTaint = aElementId.Taint(); + const StringTaint& aTaint = aElementId.Taint(); TaintFlow flow; if (aTaint.hasTaint()) { // Take the first range diff --git a/parser/html/nsHtml5StreamParser.cpp b/parser/html/nsHtml5StreamParser.cpp index a7bebd9b33ab0..dce8624fc04fa 100644 --- a/parser/html/nsHtml5StreamParser.cpp +++ b/parser/html/nsHtml5StreamParser.cpp @@ -1642,7 +1642,7 @@ nsresult nsHtml5StreamParser::OnDataAvailable(nsIRequest* aRequest, return mExecutor->MarkAsBroken(NS_ERROR_OUT_OF_MEMORY); } Buffer data(std::move(*maybe)); - StringTaint taint; + SafeStringTaint taint; if (taintInputStream) { rv = taintInputStream->TaintedRead(reinterpret_cast(data.Elements()), data.Length(), &taint, &totalRead); @@ -1684,7 +1684,7 @@ nsresult nsHtml5StreamParser::OnDataAvailable(nsIRequest* aRequest, return NS_ERROR_OUT_OF_MEMORY; } Buffer data(std::move(*maybe)); - StringTaint taint; + SafeStringTaint taint; if (taintInputStream) { rv = taintInputStream->TaintedRead(reinterpret_cast(data.Elements()), diff --git a/parser/html/nsHtml5TreeBuilder.cpp b/parser/html/nsHtml5TreeBuilder.cpp index 599a274de0542..3f40fde512343 100644 --- a/parser/html/nsHtml5TreeBuilder.cpp +++ b/parser/html/nsHtml5TreeBuilder.cpp @@ -146,6 +146,7 @@ void nsHtml5TreeBuilder::startTokenization(nsHtml5Tokenizer* self) { charBufferLen = 0; charBuffer = nullptr; framesetOk = true; + charTaint.clear(); if (fragment) { nsIContentHandle* elt; if (contextNode) { diff --git a/parser/html/nsHtml5UTF16Buffer.h b/parser/html/nsHtml5UTF16Buffer.h index 0146364a49f91..091619a691d19 100644 --- a/parser/html/nsHtml5UTF16Buffer.h +++ b/parser/html/nsHtml5UTF16Buffer.h @@ -59,7 +59,7 @@ class nsHtml5Portability; class nsHtml5UTF16Buffer { private: char16_t* buffer; - StringTaint taint; + SafeStringTaint taint; int32_t start; int32_t end;