-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Rust: Fix default source and sink in inline flow test #17995
Conversation
@@ -10,15 +10,16 @@ private import codeql.rust.dataflow.internal.DataFlowImpl | |||
private import codeql.rust.dataflow.internal.TaintTrackingImpl | |||
private import internal.InlineExpectationsTestImpl as InlineExpectationsTestImpl | |||
|
|||
// Holds if the target expression of `call` is a path and the string representation of the path is `name`. | |||
private predicate callTargetName(CallExpr call, string name) { | |||
call.getExpr().(PathExpr).getPath().toString() = name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should define the toString
of PathExpr
to be result = this.getPath().toString()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me. I'll update the PR to do that instead :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, we ought to implement a lot of more sensible toString
s, but that is of course not for this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the suggested toString
on PathExpr
. Note that this added two inconsistencies here because the Path
nodes on the last two lines in gen_path_expr.rs doesn't have a string representation. I don't know if this is a bug in the extractor? When I explore the AST for that file I also see that the last two let
statements doesn't have an initializer which seems odd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not an extractor error. The qualifier of a path like <TypeRef as Trait>::foo
does not have a nameRef so the toString()
fails. See also:
codeql/rust/ql/lib/codeql/rust/elements/internal/PathSegmentImpl.qll
Lines 23 to 26 in e081b9a
// TODO: this does not cover everything | |
if this.hasGenericArgList() | |
then result = this.getNameRef().toString() + "::<...>" | |
else result = this.getNameRef().toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's too much hassle to address this in the current PR, then we can also accept the inconsistencies and fix them in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense. I've accepted the inconsistencies for now.
4b4080e
to
a71f730
Compare
a71f730
to
0e025ab
Compare
Before
toString
was called on aPathExpr
and we want to call it on thePath
. This fixes a test failure in the very simplesink(source("taint"))
case.