Make FunctionOutput.isResult(0) and CallNode.getResult(0) match single results#157
Conversation
owen-mc
requested review from
a team and
max-schaefer
and removed request for
max-schaefer
|
The commit "Make FunctionOutput.isResult(0) match single results" is making tests for ReflectedXss fail. I'm looking into why. |
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
LGTM modulo minor suggestions and the test failure.
|
|
||
| override predicate isResult(int i) { i = index and i >= 0 } | ||
| override predicate isResult(int i) { | ||
| index = -1 and i = 0 |
There was a problem hiding this comment.
Perhaps do i = 0 and isResult() instead? Doesn't make a difference here, but nicely parallels the other changes.
|
|
||
| override predicate isResult(int i) { i = index and i >= 0 } | ||
| override predicate isResult(int i) { | ||
| index = -1 and i = 0 |
| /** Gets the data-flow node corresponding to the `i`th result of this call. */ | ||
| Node getResult(int i) { result = extractTupleElement(this, i) } | ||
| Node getResult(int i) { | ||
| not getType() instanceof TupleType and i = 0 and result = this |
There was a problem hiding this comment.
| not getType() instanceof TupleType and i = 0 and result = this | |
| i = 0 and result = getResult() |
| @@ -328,7 +328,11 @@ class CallNode extends ExprNode { | |||
| FunctionNode getCallback(int i) { result.getASuccessor*() = this.getArgument(i) } | |||
|
|
|||
| /** Gets the data-flow node corresponding to the `i`th result of this call. */ | |||
There was a problem hiding this comment.
Perhaps mention here that if there is a single result, that is considered to be the 0th result.
|
I think it would be a bit cleaner to just remove the codeql-go/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll Lines 16 to 22 in f0e1147 I also think it would make sense to have a |
I think it's worth having both, for completeness and analogy with the two
Seconded. |
|
@sauyon If we don't have a separate model for "the one and only result" then we can't make it that |
owen-mc
force-pushed
the
isresult-consistency
branch
from
8b208f8 to
bbce7d1
Compare
| if nr = 1 then kind = TSingleReturn() else kind = TMultiReturn(i) | ||
| ) | ||
| } | ||
| ReturnNode() { exists(int nr | nr = fd.getType().getNumResult() | kind = MkReturnKind(i)) } |
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
One more suggestion, otherwise LGTM. I'll start an evaluation.
| override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) { | ||
| inp.isParameter(_) and | ||
| (outp.isResult() or outp.isResult(_)) | ||
| outp.isResult(_) |
There was a problem hiding this comment.
Here is another bit of code that we could simplify: https://github.com/github/codeql-go/blob/master/ql/src/semmle/go/frameworks/Stdlib.qll#L688
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
Evaluation showed no regressions.
|
Thank you! 👏 |
4 participants
Resolves #146
Note that
FunctionOutput.isResult()andCallNode.getResult()do not match anything when there is more than one result. I think this is less confusing and error-prone than the alternative.