Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 3 additions & 24 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
Original file line number Diff line number Diff line change
Expand Up @@ -770,32 +770,11 @@ class RawIndirectInstruction extends Node, TRawIndirectInstruction {
}
}

private predicate isFullyConvertedArgument(Expr e) {
exists(Call call |
e = call.getAnArgument().getFullyConverted()
or
e = call.getQualifier().getFullyConverted()
)
}

private predicate isFullyConvertedCall(Expr e) { e = any(Call call).getFullyConverted() }

/** Holds if `Node::asExpr` should map an some operand node to `e`. */
private predicate convertedExprMustBeOperand(Expr e) {
isFullyConvertedArgument(e)
or
isFullyConvertedCall(e)
}

/** Holds if `node` is an `OperandNode` that should map `node.asExpr()` to `e`. */
predicate exprNodeShouldBeOperand(OperandNode node, Expr e) {
exists(Operand operand |
node.getOperand() = operand and
e = operand.getDef().getConvertedResultExpression()
|
convertedExprMustBeOperand(e)
or
node.(IndirectOperand).isIRRepresentationOf(_, _)
exists(Instruction def |
unique( | | getAUse(def)) = node.getOperand() and
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for the unique?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want the Expr -> Node direction to be a one-to-many relation. If there happens to be more than one use of the operand, then we'll fall back to picking the InstructionNode with the defining Instruction as the node for the expression.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was implicitly achieved before by the much more restrictive definition of this predicate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly. Previously we'd only map an expression to an operand when it was an argument of (or a result of) a function call. And in both cases (at least the argument case) there should always be a unique use.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think so. When there's a unique use this predicate kicks in and ensures that that the Call expression is mapped to the dataflow node (and the Call expression has a pretty toString like call to getenv). And when there isn't a unique use we get the Instruction's Opcode's toString which is a lot less user-friendly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was on my list to fix anyway, so no need to solve it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ I think this is one of the issues we should tackle as part of the "prettify dataflow paths" issue 😄.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was on my list to fix anyway, so no need to solve it here.

Ah, perfect!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the prettifying issue that was what I was referring to 😄

e = def.getConvertedResultExpression()
)
}

Expand Down
66 changes: 35 additions & 31 deletions cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,17 @@ edges
| test.cpp:91:9:91:16 | fread output argument | test.cpp:93:17:93:24 | Convert indirection |
| test.cpp:93:11:93:14 | strncat output argument | test.cpp:94:45:94:48 | Convert indirection |
| test.cpp:93:17:93:24 | Convert indirection | test.cpp:93:11:93:14 | strncat output argument |
| test.cpp:106:20:106:25 | call to getenv | test.cpp:107:33:107:36 | CopyValue indirection |
| test.cpp:106:20:106:25 | Call | test.cpp:107:33:107:36 | CopyValue indirection |
| test.cpp:106:20:106:38 | call to getenv indirection | test.cpp:107:33:107:36 | CopyValue indirection |
| test.cpp:107:31:107:31 | Call | test.cpp:108:18:108:22 | call to c_str indirection |
| test.cpp:107:33:107:36 | CopyValue indirection | test.cpp:107:31:107:31 | Call |
| test.cpp:113:20:113:25 | call to getenv | test.cpp:114:19:114:22 | CopyValue indirection |
| test.cpp:113:20:113:25 | Call | test.cpp:114:19:114:22 | CopyValue indirection |
| test.cpp:113:20:113:38 | call to getenv indirection | test.cpp:114:19:114:22 | CopyValue indirection |
| test.cpp:114:10:114:23 | Convert | test.cpp:114:25:114:29 | call to c_str indirection |
| test.cpp:114:17:114:17 | call to operator+ | test.cpp:114:25:114:29 | call to c_str indirection |
| test.cpp:114:19:114:22 | CopyValue indirection | test.cpp:114:10:114:23 | Convert |
| test.cpp:114:19:114:22 | CopyValue indirection | test.cpp:114:17:114:17 | call to operator+ |
| test.cpp:119:20:119:25 | call to getenv | test.cpp:120:19:120:22 | CopyValue indirection |
| test.cpp:119:20:119:25 | Call | test.cpp:120:19:120:22 | CopyValue indirection |
| test.cpp:119:20:119:38 | call to getenv indirection | test.cpp:120:19:120:22 | CopyValue indirection |
| test.cpp:120:17:120:17 | call to operator+ | test.cpp:120:10:120:30 | call to data indirection |
| test.cpp:120:19:120:22 | CopyValue indirection | test.cpp:120:17:120:17 | call to operator+ |
Expand Down Expand Up @@ -122,19 +122,19 @@ nodes
| test.cpp:93:11:93:14 | strncat output argument | semmle.label | strncat output argument |
| test.cpp:93:17:93:24 | Convert indirection | semmle.label | Convert indirection |
| test.cpp:94:45:94:48 | Convert indirection | semmle.label | Convert indirection |
| test.cpp:106:20:106:25 | call to getenv | semmle.label | call to getenv |
| test.cpp:106:20:106:25 | Call | semmle.label | Call |
| test.cpp:106:20:106:38 | call to getenv indirection | semmle.label | call to getenv indirection |
| test.cpp:107:31:107:31 | Call | semmle.label | Call |
| test.cpp:107:33:107:36 | CopyValue indirection | semmle.label | CopyValue indirection |
| test.cpp:108:18:108:22 | call to c_str indirection | semmle.label | call to c_str indirection |
| test.cpp:113:20:113:25 | call to getenv | semmle.label | call to getenv |
| test.cpp:113:20:113:25 | Call | semmle.label | Call |
| test.cpp:113:20:113:38 | call to getenv indirection | semmle.label | call to getenv indirection |
| test.cpp:114:10:114:23 | Convert | semmle.label | Convert |
| test.cpp:114:17:114:17 | call to operator+ | semmle.label | call to operator+ |
| test.cpp:114:19:114:22 | CopyValue indirection | semmle.label | CopyValue indirection |
| test.cpp:114:25:114:29 | call to c_str indirection | semmle.label | call to c_str indirection |
| test.cpp:114:25:114:29 | call to c_str indirection | semmle.label | call to c_str indirection |
| test.cpp:119:20:119:25 | call to getenv | semmle.label | call to getenv |
| test.cpp:119:20:119:25 | Call | semmle.label | Call |
| test.cpp:119:20:119:38 | call to getenv indirection | semmle.label | call to getenv indirection |
| test.cpp:120:10:120:30 | call to data indirection | semmle.label | call to data indirection |
| test.cpp:120:17:120:17 | call to operator+ | semmle.label | call to operator+ |
Expand Down Expand Up @@ -217,13 +217,13 @@ subpaths
| test.cpp:65:10:65:16 | command | test.cpp:62:9:62:16 | fread output argument | test.cpp:65:10:65:16 | Convert indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:62:9:62:16 | fread output argument | user input (string read by fread) | test.cpp:64:11:64:17 | strncat output argument | strncat output argument |
| test.cpp:85:32:85:38 | command | test.cpp:82:9:82:16 | fread output argument | test.cpp:85:32:85:38 | Convert indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to execl. | test.cpp:82:9:82:16 | fread output argument | user input (string read by fread) | test.cpp:84:11:84:17 | strncat output argument | strncat output argument |
| test.cpp:94:45:94:48 | path | test.cpp:91:9:91:16 | fread output argument | test.cpp:94:45:94:48 | Convert indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to execl. | test.cpp:91:9:91:16 | fread output argument | user input (string read by fread) | test.cpp:93:11:93:14 | strncat output argument | strncat output argument |
| test.cpp:108:18:108:22 | call to c_str | test.cpp:106:20:106:25 | call to getenv | test.cpp:108:18:108:22 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:106:20:106:25 | call to getenv | user input (an environment variable) | test.cpp:107:31:107:31 | Call | Call |
| test.cpp:108:18:108:22 | call to c_str | test.cpp:106:20:106:25 | Call | test.cpp:108:18:108:22 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:106:20:106:25 | Call | user input (an environment variable) | test.cpp:107:31:107:31 | Call | Call |
| test.cpp:108:18:108:22 | call to c_str | test.cpp:106:20:106:38 | call to getenv indirection | test.cpp:108:18:108:22 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:106:20:106:38 | call to getenv indirection | user input (an environment variable) | test.cpp:107:31:107:31 | Call | Call |
| test.cpp:114:25:114:29 | call to c_str | test.cpp:113:20:113:25 | call to getenv | test.cpp:114:25:114:29 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:113:20:113:25 | call to getenv | user input (an environment variable) | test.cpp:114:10:114:23 | Convert | Convert |
| test.cpp:114:25:114:29 | call to c_str | test.cpp:113:20:113:25 | call to getenv | test.cpp:114:25:114:29 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:113:20:113:25 | call to getenv | user input (an environment variable) | test.cpp:114:17:114:17 | call to operator+ | call to operator+ |
| test.cpp:114:25:114:29 | call to c_str | test.cpp:113:20:113:25 | Call | test.cpp:114:25:114:29 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:113:20:113:25 | Call | user input (an environment variable) | test.cpp:114:10:114:23 | Convert | Convert |
| test.cpp:114:25:114:29 | call to c_str | test.cpp:113:20:113:25 | Call | test.cpp:114:25:114:29 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:113:20:113:25 | Call | user input (an environment variable) | test.cpp:114:17:114:17 | call to operator+ | call to operator+ |
| test.cpp:114:25:114:29 | call to c_str | test.cpp:113:20:113:38 | call to getenv indirection | test.cpp:114:25:114:29 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:113:20:113:38 | call to getenv indirection | user input (an environment variable) | test.cpp:114:10:114:23 | Convert | Convert |
| test.cpp:114:25:114:29 | call to c_str | test.cpp:113:20:113:38 | call to getenv indirection | test.cpp:114:25:114:29 | call to c_str indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:113:20:113:38 | call to getenv indirection | user input (an environment variable) | test.cpp:114:17:114:17 | call to operator+ | call to operator+ |
| test.cpp:120:25:120:28 | call to data | test.cpp:119:20:119:25 | call to getenv | test.cpp:120:10:120:30 | call to data indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:119:20:119:25 | call to getenv | user input (an environment variable) | test.cpp:120:17:120:17 | call to operator+ | call to operator+ |
| test.cpp:120:25:120:28 | call to data | test.cpp:119:20:119:25 | Call | test.cpp:120:10:120:30 | call to data indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:119:20:119:25 | Call | user input (an environment variable) | test.cpp:120:17:120:17 | call to operator+ | call to operator+ |
| test.cpp:120:25:120:28 | call to data | test.cpp:119:20:119:38 | call to getenv indirection | test.cpp:120:10:120:30 | call to data indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:119:20:119:38 | call to getenv indirection | user input (an environment variable) | test.cpp:120:17:120:17 | call to operator+ | call to operator+ |
| test.cpp:143:10:143:16 | command | test.cpp:140:9:140:11 | fread output argument | test.cpp:143:10:143:16 | Convert indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to system(string). | test.cpp:140:9:140:11 | fread output argument | user input (string read by fread) | test.cpp:142:11:142:17 | sprintf output argument | sprintf output argument |
| test.cpp:183:32:183:38 | command | test.cpp:174:9:174:16 | fread output argument | test.cpp:183:32:183:38 | Convert indirection | This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to execl. | test.cpp:174:9:174:16 | fread output argument | user input (string read by fread) | test.cpp:177:13:177:17 | strncat output argument | strncat output argument |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ edges
| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
| test.cpp:43:27:43:30 | argv | test.cpp:43:27:43:33 | access to array |
| test.cpp:43:27:43:30 | argv | test.cpp:43:27:43:33 | access to array |
| test.cpp:43:27:43:30 | argv | test.cpp:43:27:43:33 | access to array |
Expand All @@ -18,6 +20,7 @@ nodes
| test.c:21:18:21:23 | query1 | semmle.label | query1 |
| test.c:21:18:21:23 | query1 | semmle.label | query1 |
| test.c:21:18:21:23 | query1 | semmle.label | query1 |
| test.c:21:18:21:23 | query1 | semmle.label | query1 |
| test.cpp:43:27:43:30 | argv | semmle.label | argv |
| test.cpp:43:27:43:30 | argv | semmle.label | argv |
| test.cpp:43:27:43:33 | access to array | semmle.label | access to array |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ edges
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | data |
| test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | data |
| test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | data |
Expand All @@ -36,6 +38,7 @@ edges
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:62:10:62:15 | buffer |
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:63:10:63:13 | data |
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:63:10:63:13 | data |
| test.cpp:56:12:56:17 | fgets output argument | test.cpp:63:10:63:13 | data |
Expand All @@ -52,6 +55,9 @@ edges
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | buffer |
| test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | buffer |
Expand All @@ -61,15 +67,21 @@ edges
| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | recv output argument | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | recv output argument | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | recv output argument | test.cpp:99:15:99:20 | buffer |
| test.cpp:98:17:98:22 | recv output argument | test.cpp:99:15:99:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
| test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
Expand All @@ -91,6 +103,7 @@ nodes
| test.cpp:62:10:62:15 | buffer | semmle.label | buffer |
| test.cpp:62:10:62:15 | buffer | semmle.label | buffer |
| test.cpp:62:10:62:15 | buffer | semmle.label | buffer |
| test.cpp:62:10:62:15 | buffer | semmle.label | buffer |
| test.cpp:63:10:63:13 | data | semmle.label | data |
| test.cpp:63:10:63:13 | data | semmle.label | data |
| test.cpp:63:10:63:13 | data | semmle.label | data |
Expand All @@ -107,18 +120,21 @@ nodes
| test.cpp:78:10:78:15 | buffer | semmle.label | buffer |
| test.cpp:78:10:78:15 | buffer | semmle.label | buffer |
| test.cpp:78:10:78:15 | buffer | semmle.label | buffer |
| test.cpp:78:10:78:15 | buffer | semmle.label | buffer |
| test.cpp:98:17:98:22 | buffer | semmle.label | buffer |
| test.cpp:98:17:98:22 | buffer | semmle.label | buffer |
| test.cpp:98:17:98:22 | recv output argument | semmle.label | recv output argument |
| test.cpp:99:15:99:20 | buffer | semmle.label | buffer |
| test.cpp:99:15:99:20 | buffer | semmle.label | buffer |
| test.cpp:99:15:99:20 | buffer | semmle.label | buffer |
| test.cpp:99:15:99:20 | buffer | semmle.label | buffer |
| test.cpp:106:17:106:22 | buffer | semmle.label | buffer |
| test.cpp:106:17:106:22 | buffer | semmle.label | buffer |
| test.cpp:106:17:106:22 | recv output argument | semmle.label | recv output argument |
| test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
| test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
| test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
| test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
#select
| test.cpp:26:10:26:16 | command | test.cpp:42:18:42:23 | call to getenv | test.cpp:26:10:26:16 | command | The value of this argument may come from $@ and is being passed to system. | test.cpp:42:18:42:23 | call to getenv | call to getenv |
| test.cpp:31:10:31:16 | command | test.cpp:43:18:43:23 | call to getenv | test.cpp:31:10:31:16 | command | The value of this argument may come from $@ and is being passed to system. | test.cpp:43:18:43:23 | call to getenv | call to getenv |
Expand Down
Loading