Skip to content

Commit

Permalink
Performance Tweaks
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@leeN
leeN committed Jan 29, 2024
1 parent ea28d9c commit f28bf70
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 48 deletions.
6 changes: 4 additions & 2 deletions js/src/builtin/Array.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
84 changes: 55 additions & 29 deletions js/src/builtin/String.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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)));
}

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -3948,9 +3960,11 @@ static ArrayObject* SplitHelper(JSContext* cx, Handle<JSLinearString*> 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) {
Expand All @@ -3974,7 +3988,9 @@ static ArrayObject* SplitHelper(JSContext* cx, Handle<JSLinearString*> 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());
Expand Down Expand Up @@ -4014,7 +4030,9 @@ static ArrayObject* CharSplitHelper(JSContext* cx, Handle<JSLinearString*> 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));
}
Expand Down Expand Up @@ -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));

Expand All @@ -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));

Expand Down Expand Up @@ -4791,10 +4813,12 @@ static MOZ_ALWAYS_INLINE bool Encode(JSContext* cx, Handle<JSLinearString*> 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);
Expand Down Expand Up @@ -4955,10 +4979,12 @@ static bool Decode(JSContext* cx, Handle<JSLinearString*> 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);
Expand Down
34 changes: 22 additions & 12 deletions js/src/vm/SelfHosting.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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;
}
Expand All @@ -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)
Expand All @@ -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)
{
Expand All @@ -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)
Expand All @@ -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;
}
Expand Down
8 changes: 4 additions & 4 deletions taint/Taint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion taint/Taint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit f28bf70

Please sign in to comment.