Skip to content

JS: only calculate getStringValue for concatenation roots #3864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 3, 2020
Merged

JS: only calculate getStringValue for concatenation roots #3864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f54bb1
only calculate getStringValue for concatenation roots
erik-krogh Jul 1, 2020
226e066
use strictconcat instead of concat
erik-krogh Jul 2, 2020
bbdeca3
use `getUnderlyingValue()` to find leafs of a string-concat
erik-krogh Jul 2, 2020
f60a748
ignore parents that doesn't have all constant roots when deciding whi…
erik-krogh Jul 2, 2020
ceb1929
autoformat
erik-krogh Jul 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 63 additions & 9 deletions javascript/ql/src/semmle/javascript/Expr.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class Expr extends @expr, ExprOrStmt, ExprOrType, AST::ValueNode {

/** Gets the constant string value this expression evaluates to, if any. */
cached
string getStringValue() { none() }
string getStringValue() { result = getStringValue(this) }

/** Holds if this expression is impure, that is, its evaluation could have side effects. */
predicate isImpure() { any() }
Expand Down Expand Up @@ -423,8 +423,6 @@ class BigIntLiteral extends @bigintliteral, Literal {
* ```
*/
class StringLiteral extends @stringliteral, Literal {
override string getStringValue() { result = getValue() }

/**
* Gets the value of this string literal parsed as a regular expression, if possible.
*
Expand Down Expand Up @@ -839,8 +837,6 @@ class SeqExpr extends @seqexpr, Expr {

override predicate isImpure() { getAnOperand().isImpure() }

override string getStringValue() { result = getLastOperand().getStringValue() }

override Expr getUnderlyingValue() { result = getLastOperand().getUnderlyingValue() }
}

Expand Down Expand Up @@ -1517,13 +1513,71 @@ class URShiftExpr extends @urshiftexpr, BinaryExpr {
*/
class AddExpr extends @addexpr, BinaryExpr {
override string getOperator() { result = "+" }
}

override string getStringValue() {
result = getLeftOperand().getStringValue() + getRightOperand().getStringValue() and
result.length() < 1000 * 1000
}
/**
* Gets the string value for the expression `e`.
* This string-value is either a constant-string, or the result from a simple string-concatenation.
*/
private string getStringValue(Expr e) {
result = getConstantString(e)
or
result = getConcatenatedString(e)
}

/**
* Gets the constant string value for the expression `e`.
*/
private string getConstantString(Expr e) {
result = getConstantString(e.getUnderlyingValue())
or
result = e.(StringLiteral).getValue()
or
exists(TemplateLiteral lit | lit = e |
// fold singletons
lit.getNumChildExpr() = 0 and
result = ""
or
e.getNumChildExpr() = 1 and
result = getConstantString(lit.getElement(0))
)
or
result = e.(TemplateElement).getValue()
}

/**
* Holds if `add` is a string-concatenation where all the transitive leafs have a constant string value.
*/
private predicate hasAllConstantLeafs(AddExpr add) {
forex(Expr leaf | leaf = getAnAddOperand*(add) and not exists(getAnAddOperand(leaf)) |
exists(getConstantString(leaf))
)
}

/**
* Gets the concatenated string for a string-concatenation `add`.
* Only has a result if `add` is not itself an operand in another string-concatenation with all constant leafs.
*/
private string getConcatenatedString(Expr add) {
result = getConcatenatedString(add.getUnderlyingValue())
or
not add = getAnAddOperand(any(AddExpr parent | hasAllConstantLeafs(parent))) and
hasAllConstantLeafs(add) and
result =
strictconcat(Expr leaf |
leaf = getAnAddOperand*(add)
|
getConstantString(leaf) order by leaf.getFirstToken().getIndex()
) and
result.length() < 1000 * 1000
}

/**
* Gets an operand from `add`.
* Is specialized to `AddExpr` such that `getAnAddOperand*(add)` can be used to get a leaf from a string-concatenation transitively.
*/
private Expr getAnAddOperand(AddExpr add) { result = add.getAnOperand().getUnderlyingValue() }

/**
* A subtraction expression.
*
Expand Down
11 changes: 0 additions & 11 deletions javascript/ql/src/semmle/javascript/Templates.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,6 @@ class TemplateLiteral extends Expr, @templateliteral {
int getNumElement() { result = count(getAnElement()) }

override predicate isImpure() { getAnElement().isImpure() }

override string getStringValue() {
// fold singletons
getNumChildExpr() = 0 and
result = ""
or
getNumChildExpr() = 1 and
result = getElement(0).getStringValue()
}
}

/**
Expand Down Expand Up @@ -103,6 +94,4 @@ class TemplateElement extends Expr, @templateelement {
string getRawValue() { literals(_, result, this) }

override predicate isImpure() { none() }

override string getStringValue() { result = getValue() }
}
72 changes: 72 additions & 0 deletions javascript/ql/test/library-tests/StringConcatenation/StringOps.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ concatenation
| tst.js:89:3:89:14 | x |
| tst.js:89:3:89:14 | x += 'three' |
| tst.js:95:7:95:30 | x.conca ... three') |
| tst.js:104:11:104:23 | "foo" + "bar" |
| tst.js:104:11:104:31 | "foo" + ... + value |
| tst.js:105:11:105:23 | value + "foo" |
| tst.js:105:11:105:31 | value + ... + "bar" |
| tst.js:106:11:106:33 | "foo" + ... "baz") |
| tst.js:106:20:106:32 | "bar" + "baz" |
concatenationOperand
| closure.js:5:1:5:37 | build(' ... 'four') |
| closure.js:5:7:5:11 | 'one' |
Expand Down Expand Up @@ -127,6 +133,18 @@ concatenationOperand
| tst.js:95:7:95:7 | x |
| tst.js:95:16:95:20 | 'two' |
| tst.js:95:23:95:29 | 'three' |
| tst.js:104:11:104:15 | "foo" |
| tst.js:104:11:104:23 | "foo" + "bar" |
| tst.js:104:19:104:23 | "bar" |
| tst.js:104:27:104:31 | value |
| tst.js:105:11:105:15 | value |
| tst.js:105:11:105:23 | value + "foo" |
| tst.js:105:19:105:23 | "foo" |
| tst.js:105:27:105:31 | "bar" |
| tst.js:106:11:106:15 | "foo" |
| tst.js:106:19:106:33 | ("bar" + "baz") |
| tst.js:106:20:106:24 | "bar" |
| tst.js:106:28:106:32 | "baz" |
concatenationLeaf
| closure.js:5:7:5:11 | 'one' |
| closure.js:5:14:5:18 | 'two' |
Expand Down Expand Up @@ -199,6 +217,16 @@ concatenationLeaf
| tst.js:95:7:95:7 | x |
| tst.js:95:16:95:20 | 'two' |
| tst.js:95:23:95:29 | 'three' |
| tst.js:104:11:104:15 | "foo" |
| tst.js:104:19:104:23 | "bar" |
| tst.js:104:27:104:31 | value |
| tst.js:105:11:105:15 | value |
| tst.js:105:19:105:23 | "foo" |
| tst.js:105:27:105:31 | "bar" |
| tst.js:106:11:106:15 | "foo" |
| tst.js:106:19:106:33 | ("bar" + "baz") |
| tst.js:106:20:106:24 | "bar" |
| tst.js:106:28:106:32 | "baz" |
concatenationNode
| closure.js:5:1:5:37 | build(' ... 'four') |
| closure.js:5:1:5:46 | build(' ... 'five' |
Expand Down Expand Up @@ -318,6 +346,22 @@ concatenationNode
| tst.js:95:7:95:30 | x.conca ... three') |
| tst.js:95:16:95:20 | 'two' |
| tst.js:95:23:95:29 | 'three' |
| tst.js:104:11:104:15 | "foo" |
| tst.js:104:11:104:23 | "foo" + "bar" |
| tst.js:104:11:104:31 | "foo" + ... + value |
| tst.js:104:19:104:23 | "bar" |
| tst.js:104:27:104:31 | value |
| tst.js:105:11:105:15 | value |
| tst.js:105:11:105:23 | value + "foo" |
| tst.js:105:11:105:31 | value + ... + "bar" |
| tst.js:105:19:105:23 | "foo" |
| tst.js:105:27:105:31 | "bar" |
| tst.js:106:11:106:15 | "foo" |
| tst.js:106:11:106:33 | "foo" + ... "baz") |
| tst.js:106:19:106:33 | ("bar" + "baz") |
| tst.js:106:20:106:24 | "bar" |
| tst.js:106:20:106:32 | "bar" + "baz" |
| tst.js:106:28:106:32 | "baz" |
operand
| closure.js:5:1:5:37 | build(' ... 'four') | 0 | closure.js:5:7:5:11 | 'one' |
| closure.js:5:1:5:37 | build(' ... 'four') | 1 | closure.js:5:14:5:28 | 'two' + 'three' |
Expand Down Expand Up @@ -421,6 +465,18 @@ operand
| tst.js:95:7:95:30 | x.conca ... three') | 0 | tst.js:95:7:95:7 | x |
| tst.js:95:7:95:30 | x.conca ... three') | 1 | tst.js:95:16:95:20 | 'two' |
| tst.js:95:7:95:30 | x.conca ... three') | 2 | tst.js:95:23:95:29 | 'three' |
| tst.js:104:11:104:23 | "foo" + "bar" | 0 | tst.js:104:11:104:15 | "foo" |
| tst.js:104:11:104:23 | "foo" + "bar" | 1 | tst.js:104:19:104:23 | "bar" |
| tst.js:104:11:104:31 | "foo" + ... + value | 0 | tst.js:104:11:104:23 | "foo" + "bar" |
| tst.js:104:11:104:31 | "foo" + ... + value | 1 | tst.js:104:27:104:31 | value |
| tst.js:105:11:105:23 | value + "foo" | 0 | tst.js:105:11:105:15 | value |
| tst.js:105:11:105:23 | value + "foo" | 1 | tst.js:105:19:105:23 | "foo" |
| tst.js:105:11:105:31 | value + ... + "bar" | 0 | tst.js:105:11:105:23 | value + "foo" |
| tst.js:105:11:105:31 | value + ... + "bar" | 1 | tst.js:105:27:105:31 | "bar" |
| tst.js:106:11:106:33 | "foo" + ... "baz") | 0 | tst.js:106:11:106:15 | "foo" |
| tst.js:106:11:106:33 | "foo" + ... "baz") | 1 | tst.js:106:19:106:33 | ("bar" + "baz") |
| tst.js:106:20:106:32 | "bar" + "baz" | 0 | tst.js:106:20:106:24 | "bar" |
| tst.js:106:20:106:32 | "bar" + "baz" | 1 | tst.js:106:28:106:32 | "baz" |
nextLeaf
| closure.js:5:7:5:11 | 'one' | closure.js:5:14:5:18 | 'two' |
| closure.js:5:14:5:18 | 'two' | closure.js:5:22:5:28 | 'three' |
Expand Down Expand Up @@ -466,6 +522,12 @@ nextLeaf
| tst.js:89:3:89:3 | x | tst.js:89:8:89:14 | 'three' |
| tst.js:95:7:95:7 | x | tst.js:95:16:95:20 | 'two' |
| tst.js:95:16:95:20 | 'two' | tst.js:95:23:95:29 | 'three' |
| tst.js:104:11:104:15 | "foo" | tst.js:104:19:104:23 | "bar" |
| tst.js:104:19:104:23 | "bar" | tst.js:104:27:104:31 | value |
| tst.js:105:11:105:15 | value | tst.js:105:19:105:23 | "foo" |
| tst.js:105:19:105:23 | "foo" | tst.js:105:27:105:31 | "bar" |
| tst.js:106:11:106:15 | "foo" | tst.js:106:19:106:33 | ("bar" + "baz") |
| tst.js:106:20:106:24 | "bar" | tst.js:106:28:106:32 | "baz" |
htmlRoot
| html-concat.js:2:14:2:26 | `<b>${x}</b>` |
| html-concat.js:3:14:3:26 | `<B>${x}</B>` |
Expand All @@ -488,3 +550,13 @@ htmlLeaf
| html-concat.js:8:15:10:23 | .\\n \\n ... um!</i> |
| html-concat.js:13:3:13:8 | buffer |
| html-concat.js:13:13:13:18 | '<li>' |
getStringValue
| tst.js:104:11:104:15 | "foo" | foo |
| tst.js:104:11:104:23 | "foo" + "bar" | foobar |
| tst.js:104:19:104:23 | "bar" | bar |
| tst.js:105:19:105:23 | "foo" | foo |
| tst.js:105:27:105:31 | "bar" | bar |
| tst.js:106:11:106:15 | "foo" | foo |
| tst.js:106:11:106:33 | "foo" + ... "baz") | foobarbaz |
| tst.js:106:20:106:24 | "bar" | bar |
| tst.js:106:28:106:32 | "baz" | baz |
5 changes: 5 additions & 0 deletions javascript/ql/test/library-tests/StringConcatenation/StringOps.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,8 @@ query predicate nextLeaf(StringOps::ConcatenationNode node, DataFlow::Node next)
query StringOps::HtmlConcatenationRoot htmlRoot() { any() }

query StringOps::HtmlConcatenationLeaf htmlLeaf() { any() }

query string getStringValue(Expr e) {
result = e.getStringValue() and
e.getEnclosingFunction().getName() = "stringValue"
}
6 changes: 6 additions & 0 deletions javascript/ql/test/library-tests/StringConcatenation/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,9 @@ function concatCall() {
function arrayConcat(a, b) {
return [].concat(a, b);
}

function stringValue() {
var a = "foo" + "bar" + value;
var b = value + "foo" + "bar";
var c = "foo" + ("bar" + "baz")
}