From f28bf70f00cf1462e7bb0951207cb339c9fb6b44 Mon Sep 17 00:00:00 2001 From: David Klein Date: Mon, 29 Jan 2024 10:02:08 +0100 Subject: [PATCH 1/2] Performance Tweaks After a bit of profiling I noticed that creating TaintOperations is very expensive. In hindsight this is obvious, as we have to walk the stack to get the location etc. We did however create a lot of TaintOperation objects without ever using them, i.e., due to the string itself being untainted. This change does add explicit checks where possible (in some cases we create the TaintOperation object in advance due to GC issues, this is not easily hidden behind a check) so TaintOperations are only created if the string is actually tainted, i.e., we use the TaintOperation to extend the TaintFlow. --- js/src/builtin/Array.cpp | 6 ++- js/src/builtin/String.cpp | 84 +++++++++++++++++++++++++-------------- js/src/vm/SelfHosting.cpp | 34 ++++++++++------ taint/Taint.cpp | 8 ++-- taint/Taint.h | 2 +- 5 files changed, 86 insertions(+), 48 deletions(-) diff --git a/js/src/builtin/Array.cpp b/js/src/builtin/Array.cpp index 6ddd563cbb05d..aea0e546fb206 100644 --- a/js/src/builtin/Array.cpp +++ b/js/src/builtin/Array.cpp @@ -1373,8 +1373,10 @@ bool js::array_join(JSContext* cx, unsigned argc, Value* vp) { return false; } - // TaintFox: add taint operation. - str->taint().extend(TaintOperationFromContext(cx, "Array.join", true, sepstr)); + if(str->isTainted()) { + // TaintFox: add taint operation. + str->taint().extend(TaintOperationFromContext(cx, "Array.join", true, sepstr)); + } args.rval().setString(str); return true; diff --git a/js/src/builtin/String.cpp b/js/src/builtin/String.cpp index c0b735e51f8f3..cd82b37f3e311 100644 --- a/js/src/builtin/String.cpp +++ b/js/src/builtin/String.cpp @@ -518,8 +518,10 @@ static bool str_escape(JSContext* cx, unsigned argc, Value* vp) { } // Taintfox: set new taint - newtaint.extend(TaintOperationFromContext(cx, "escape", true, str)); - res->setTaint(cx, newtaint); + if(newtaint.hasTaint()) { + newtaint.extend(TaintOperationFromContext(cx, "escape", true, str)); + res->setTaint(cx, newtaint); + } args.rval().setString(res); return true; @@ -689,8 +691,10 @@ static bool str_unescape(JSContext* cx, unsigned argc, Value* vp) { } // TaintFox: add taint operation. - newtaint.extend(op); - result->setTaint(cx, newtaint); + if(newtaint.hasTaint()) { + newtaint.extend(op); + result->setTaint(cx, newtaint); + } args.rval().setString(result); return true; @@ -1352,7 +1356,9 @@ static bool str_toLocaleLowerCase(JSContext* cx, unsigned argc, Value* vp) { // TaintFox: propagate taint and add operation MOZ_ASSERT(result.isString()); - result.toString()->setTaint(cx, str->taint().safeCopy().extend(TaintOperationFromContext(cx, "toLocaleLowerCase", true, str))); + if(str->isTainted()) { + result.toString()->setTaint(cx, str->taint().safeCopy().extend(TaintOperationFromContext(cx, "toLocaleLowerCase", true, str))); + } args.rval().set(result); return true; @@ -1595,7 +1601,7 @@ static JSString* ToUpperCase(JSContext* cx, JSLinearString* str) { // TaintFox: disabled. We need to return a new string here (so we can correctly // set the taint). However, we are in an AutoCheckCannotGC block, so cannot // allocate a new string here. - if (i == length) { + if (i == length && taint.hasTaint()) { str->setTaint(cx, taint); return str; } @@ -1656,7 +1662,9 @@ static JSString* ToUpperCase(JSContext* cx, JSLinearString* str) { JSString* res = newChars.mapNonEmpty(toString); // TaintFox: Add taint operation to all taint ranges of the input string. - res->setTaint(cx, taint); + if(taint.hasTaint()) { + res->setTaint(cx, taint); + } return res; } @@ -1778,7 +1786,9 @@ static bool str_toLocaleUpperCase(JSContext* cx, unsigned argc, Value* vp) { // TaintFox: propagate taint and add operation MOZ_ASSERT(result.isString()); - result.toString()->setTaint(cx, str->taint().safeCopy().extend(TaintOperationFromContext(cx, "toLocaleUpperCase", true, str))); + if(str->isTainted()) { + result.toString()->setTaint(cx, str->taint().safeCopy().extend(TaintOperationFromContext(cx, "toLocaleUpperCase", true, str))); + } args.rval().set(result); return true; @@ -1898,7 +1908,7 @@ static bool str_normalize(JSContext* cx, unsigned argc, Value* vp) { // Latin-1 strings are already in Normalization Form C. if (form == NormalizationForm::NFC && str->hasLatin1Chars()) { - if (str->taint().hasTaint()) { + if (str->isTainted()) { str->taint().extend(TaintOperationFromContext(cx, "normalize", true, str)); } // Step 7. @@ -1929,7 +1939,7 @@ static bool str_normalize(JSContext* cx, unsigned argc, Value* vp) { // Return if the input string is already normalized. if (alreadyNormalized.unwrap() == AlreadyNormalized::Yes) { - if (str->taint().hasTaint()) { + if (str->isTainted()) { str->taint().extend(TaintOperationFromContext(cx, "normalize", true, str)); } // Step 7. @@ -1943,7 +1953,7 @@ static bool str_normalize(JSContext* cx, unsigned argc, Value* vp) { } // TaintFox: Add taint operation. - if (str->taint().hasTaint()) { + if (str->isTainted()) { ns->setTaint(cx, str->taint().safeCopy().extend(TaintOperationFromContext(cx, "normalize", true, str))); } @@ -3064,7 +3074,7 @@ static bool TrimString(JSContext* cx, const CallArgs& args, const char* funName, // TaintFox: Add trim operation to current taint flow. // the acutal trimming of taint ranges has been done in // NewDependentString (StringType-inl.h, JSDependentString::init) - if (result->taint().hasTaint()) { + if (result->isTainted()) { AutoCheckCannotGC nogc; if (trimStart && trimEnd) { result->taint().extend(TaintOperationFromContext(cx, "trim", true)); @@ -3685,8 +3695,10 @@ static JSString* ReplaceAll(JSContext* cx, JSLinearString* string, } // Taintfox: extend the taint flow - result.taint().extend( - TaintOperationFromContextJSString(cx, "replaceAll", true, searchString, replaceString)); + if(result.taint().hasTaint()) { + result.taint().extend( + TaintOperationFromContextJSString(cx, "replaceAll", true, searchString, replaceString)); + } return resultString; } @@ -3948,9 +3960,11 @@ static ArrayObject* SplitHelper(JSContext* cx, Handle str, return nullptr; } - // TaintFox: extend taint flow - sub->taint().extend(TaintOperation("split", true, TaintLocationFromContext(cx), - { taintarg(cx, sep), taintarg(cx, count++) })); + if(sub->isTainted()) { + // TaintFox: extend taint flow + sub->taint().extend(TaintOperation("split", true, TaintLocationFromContext(cx), + { taintarg(cx, sep), taintarg(cx, count++) })); + } // Step 14.c.ii.5. if (splits.length() == limit) { @@ -3974,7 +3988,9 @@ static ArrayObject* SplitHelper(JSContext* cx, Handle str, } // TaintFox: extend taint flow - sub->taint().extend(TaintOperation("split", true, TaintLocationFromContext(cx), { taintarg(cx, sep), taintarg(cx, count++) })); + if(sub->isTainted()) { + sub->taint().extend(TaintOperation("split", true, TaintLocationFromContext(cx), { taintarg(cx, sep), taintarg(cx, count++) })); + } // Step 18. return NewDenseCopiedArray(cx, splits.length(), splits.begin()); @@ -4014,7 +4030,9 @@ static ArrayObject* CharSplitHelper(JSContext* cx, Handle str, return nullptr; } // TaintFox: extend taint flow - sub->taint().extend(TaintOperation("split", true, TaintLocationFromContext(cx), { taintarg(cx, u""), taintarg(cx, count++) })); + if(sub->isTainted()) { + sub->taint().extend(TaintOperation("split", true, TaintLocationFromContext(cx), { taintarg(cx, u""), taintarg(cx, count++) })); + } splits->initDenseElement(i, StringValue(sub)); } @@ -4059,7 +4077,9 @@ static MOZ_ALWAYS_INLINE ArrayObject* SplitSingleCharHelper( return nullptr; } // TaintFox: extend taint flow - sub->taint().extend(op); + if(sub->isTainted()) { + sub->taint().extend(op); + } splits->initDenseElement(splitsIndex++, StringValue(sub)); @@ -4074,7 +4094,9 @@ static MOZ_ALWAYS_INLINE ArrayObject* SplitSingleCharHelper( return nullptr; } // TaintFox: extend taint flow - sub->taint().extend(op); + if(sub->isTainted()) { + sub->taint().extend(op); + } splits->initDenseElement(splitsIndex++, StringValue(sub)); @@ -4791,10 +4813,12 @@ static MOZ_ALWAYS_INLINE bool Encode(JSContext* cx, Handle str, // TaintFox: Add encode operation to output taint. SafeStringTaint taint = sb.empty() ? str->taint() : sb.taint(); - if (unescapedSet == js_isUriReservedPlusPound) { - taint.extend(TaintOperationFromContext(cx, "encodeURI", true, str)); - } else { - taint.extend(TaintOperationFromContext(cx, "encodeURIComponent", true, str)); + if(taint.hasTaint()) { + if (unescapedSet == js_isUriReservedPlusPound) { + taint.extend(TaintOperationFromContext(cx, "encodeURI", true, str)); + } else { + taint.extend(TaintOperationFromContext(cx, "encodeURIComponent", true, str)); + } } MOZ_ASSERT(res == Encode_Success); @@ -4955,10 +4979,12 @@ static bool Decode(JSContext* cx, Handle str, // TaintFox: Add decode operation to output taint. SafeStringTaint taint = sb.empty() ? str->taint() : sb.taint(); - if(reservedSet == js_isUriReservedPlusPound) { - taint.extend(TaintOperationFromContext(cx, "decodeURI", true, str)); - } else { - taint.extend(TaintOperationFromContext(cx, "decodeURIComponent", true, str)); + if(taint.hasTaint()) { + if(reservedSet == js_isUriReservedPlusPound) { + taint.extend(TaintOperationFromContext(cx, "decodeURI", true, str)); + } else { + taint.extend(TaintOperationFromContext(cx, "decodeURIComponent", true, str)); + } } MOZ_ASSERT(res == Decode_Success); diff --git a/js/src/vm/SelfHosting.cpp b/js/src/vm/SelfHosting.cpp index 379bd02f7ddc0..f2ad576e3be3d 100644 --- a/js/src/vm/SelfHosting.cpp +++ b/js/src/vm/SelfHosting.cpp @@ -1723,8 +1723,13 @@ taint_addTaintOperation(JSContext* cx, unsigned argc, Value* vp) MOZ_ASSERT(args.length() >= 2 && args[0].isString() && args[1].isString()); RootedString str(cx, args[0].toString()); - if (!str) + if (!str) { return false; + } + + if(!str->isTainted()) { + return true; + } RootedString opName(cx, args[1].toString()); if (!opName) @@ -1751,9 +1756,7 @@ taint_addTaintOperation(JSContext* cx, unsigned argc, Value* vp) } } - if(str->isTainted()) { - str->taint().extend(TaintOperation(op_chars.get(), TaintLocationFromContext(cx), taint_args)); - } + str->taint().extend(TaintOperation(op_chars.get(), TaintLocationFromContext(cx), taint_args)); return true; } @@ -1768,8 +1771,13 @@ taint_addTaintOperation_native_full(JSContext* cx, unsigned argc, Value* vp) MOZ_ASSERT(args.length() >= 2 && args[0].isString() && args[1].isString()); RootedString str(cx, args[0].toString()); - if (!str) + if (!str) { return false; + } + + if(!str->isTainted()) { + return true; + } RootedString opName(cx, args[1].toString()); if (!opName) @@ -1796,12 +1804,11 @@ taint_addTaintOperation_native_full(JSContext* cx, unsigned argc, Value* vp) } } - if(str->isTainted()) { - str->taint().extend(TaintOperation(op_chars.get(), true, TaintLocationFromContext(cx), taint_args)); - } + str->taint().extend(TaintOperation(op_chars.get(), true, TaintLocationFromContext(cx), taint_args)); return true; } + static bool taint_addTaintOperation_native(JSContext* cx, unsigned argc, Value* vp) { @@ -1812,8 +1819,13 @@ taint_addTaintOperation_native(JSContext* cx, unsigned argc, Value* vp) MOZ_ASSERT(args.length() >= 2 && args[0].isString() && args[1].isString()); RootedString str(cx, args[0].toString()); - if (!str) + if (!str) { return false; + } + + if(!str->isTainted()) { + return true; + } RootedString opName(cx, args[1].toString()); if (!opName) @@ -1840,9 +1852,7 @@ taint_addTaintOperation_native(JSContext* cx, unsigned argc, Value* vp) } } - if(str->isTainted()) { - str->taint().extend(TaintOperation(op_chars.get(), true, TaintLocationFromContext(cx), taint_args)); - } + str->taint().extend(TaintOperation(op_chars.get(), true, TaintLocationFromContext(cx), taint_args)); return true; } diff --git a/taint/Taint.cpp b/taint/Taint.cpp index 48105414ad2a3..41a2005a8dc01 100644 --- a/taint/Taint.cpp +++ b/taint/Taint.cpp @@ -150,8 +150,8 @@ TaintNode::TaintNode(TaintNode* parent, const TaintOperation& operation) } } -TaintNode::TaintNode(TaintNode* parent, TaintOperation&& operation) - : parent_(parent), refcount_(1), operation_(operation) +TaintNode::TaintNode(TaintNode* parent, TaintOperation&& operation) noexcept + : parent_(parent), refcount_(1), operation_(std::move(operation)) { MOZ_COUNT_CTOR(TaintNode); if (parent_) { @@ -166,7 +166,7 @@ TaintNode::TaintNode(const TaintOperation& operation) } TaintNode::TaintNode(TaintOperation&& operation) noexcept - : parent_(nullptr), refcount_(1), operation_(operation) + : parent_(nullptr), refcount_(1), operation_(std::move(operation)) { MOZ_COUNT_CTOR(TaintNode); } @@ -331,7 +331,7 @@ TaintFlow& TaintFlow::extend(const TaintOperation& operation) const TaintFlow& TaintFlow::extend(TaintOperation&& operation) { - TaintNode* newhead = new TaintNode(head_, operation); + TaintNode* newhead = new TaintNode(head_, std::move(operation)); head_->release(); head_ = newhead; return *this; diff --git a/taint/Taint.h b/taint/Taint.h index 11de077302cd3..5a430a0f092a7 100644 --- a/taint/Taint.h +++ b/taint/Taint.h @@ -189,7 +189,7 @@ class TaintNode // Constructing a taint node sets the initial reference count to 1. // Constructs an intermediate node. - TaintNode(TaintNode* parent, TaintOperation&& operation); + TaintNode(TaintNode* parent, TaintOperation&& operation) noexcept; // Constructs a root node. TaintNode(TaintOperation&& operation) noexcept; From 23d8be73c0448ddc4017f79012f05e8191f9e3ae Mon Sep 17 00:00:00 2001 From: David Klein Date: Tue, 30 Jan 2024 21:35:34 +0100 Subject: [PATCH 2/2] Unbroke two tests These tests used to be broken in Foxhound and were fixed in #151. This fix was intended to mask the fact that these tests were picking up and failing due to foxhound induced side effects on untainted strings. As we call toString() on objects during TaintOperation creation, the usage of Foxhound can have visible side effects. This is shown here as these tests count how often toString() is called on an object. Now, in this branch we do not create any TaintOperation objects for these specific cases of untainted strings anymore. Consequently, the tests are now passing in their original form. --- .../replace/replaceValue-evaluation-order-regexp-object.js | 3 +-- .../String/prototype/replace/replaceValue-evaluation-order.js | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order-regexp-object.js b/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order-regexp-object.js index 2473294ae17ee..1eb0d40aab9e1 100644 --- a/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order-regexp-object.js +++ b/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order-regexp-object.js @@ -23,8 +23,7 @@ replaceValue.toString = () => { let newString = "".replace("a", replaceValue); assert.sameValue(newString, ""); -// Taintfox: We change the semantics by calling toString/valueOf internally, so changed to 2 -assert.sameValue(calls, 2); +assert.sameValue(calls, 1); assert.sameValue("dollar".replace("dollar", /$/), "/$/"); reportCompare(0, 0); diff --git a/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order.js b/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order.js index 653eab30f8ca0..aeb45287bae06 100644 --- a/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order.js +++ b/js/src/tests/test262/built-ins/String/prototype/replace/replaceValue-evaluation-order.js @@ -26,7 +26,6 @@ var replaceValue = { var newString = "".replace("a", replaceValue); assert.sameValue(newString, ""); -// Taintfox: We change the semantics by calling toString/valueOf internally, so changed expected calls to 2 -assert.sameValue(calls, 2); +assert.sameValue(calls, 1); reportCompare(0, 0);