Skip to content
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

Ruby: Summarized type-tracking stores should target post-update nodes #14628

Merged
merged 3 commits into from
Nov 1, 2023
Merged

Ruby: Summarized type-tracking stores should target post-update nodes #14628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14cfb82
Ruby: Summarized type-tracking stores should target post-update nodes
hvitved Oct 30, 2023
bf3fb68
Python: Sync in changes to `SummaryTypeTracker.qll`
hvitved Oct 30, 2023
0c5b528
Address review comments
hvitved Nov 1, 2023
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
71 changes: 46 additions & 25 deletions python/ql/lib/semmle/python/dataflow/new/internal/SummaryTypeTracker.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,12 @@ signature module Input {
}

// Relating nodes to summaries
/** Gets a dataflow node respresenting the argument of `call` indicated by `arg`. */
Node argumentOf(Node call, SummaryComponent arg);
/**
* Gets a dataflow node respresenting the argument of `call` indicated by `arg`.
*
* Returns the post-update node of the argument when `isPostUpdate` is true.
*/
Node argumentOf(Node call, SummaryComponent arg, boolean isPostUpdate);

/** Gets a dataflow node respresenting the parameter of `callable` indicated by `param`. */
Node parameterOf(Node callable, SummaryComponent param);
Expand Down Expand Up @@ -221,14 +225,18 @@ module SummaryFlow<Input I> implements Output<I> {

/**
* Gets a data flow `I::Node` corresponding an argument or return value of `call`,
* as specified by `component`.
* as specified by `component`. `isOutput` indicates whether the node represents
* an output node or an input node.
*/
bindingset[call, component]
private I::Node evaluateSummaryComponentLocal(I::Node call, I::SummaryComponent component) {
result = I::argumentOf(call, component)
private I::Node evaluateSummaryComponentLocal(
I::Node call, I::SummaryComponent component, boolean isOutput
) {
result = I::argumentOf(call, component, isOutput)
or
component = I::return() and
result = call
result = call and
isOutput = true
}

/**
Expand Down Expand Up @@ -280,27 +288,40 @@ module SummaryFlow<Input I> implements Output<I> {
*/
pragma[nomagic]
private I::Node evaluateSummaryComponentStackLocal(
I::SummarizedCallable callable, I::Node call, I::SummaryComponentStack stack
I::SummarizedCallable callable, I::Node call, I::SummaryComponentStack stack, boolean isOutput
) {
exists(I::SummaryComponent component |
dependsOnSummaryComponentStackLeaf(callable, component) and
stack = I::singleton(component) and
call = I::callTo(callable) and
result = evaluateSummaryComponentLocal(call, component)
result = evaluateSummaryComponentLocal(call, component, isOutput)
)
or
exists(I::Node prev, I::SummaryComponent head, I::SummaryComponentStack tail |
prev = evaluateSummaryComponentStackLocal(callable, call, tail) and
exists(
I::Node prev, I::SummaryComponent head, I::SummaryComponentStack tail, boolean isOutput0
|
prev = evaluateSummaryComponentStackLocal(callable, call, tail, isOutput0) and
dependsOnSummaryComponentStackConsLocal(callable, pragma[only_bind_into](head),
pragma[only_bind_out](tail)) and
stack = I::push(pragma[only_bind_out](head), pragma[only_bind_out](tail))
|
result = I::parameterOf(prev, head)
// `Parameter[X]` is only allowed in the output of flow summaries (hence `isOutput = true`),
// however the target of the parameter (e.g. `Argument[Y].Parameter[X]`) should be fetched
// not from a post-update argument node (hence `isOutput0 = false`)
result = I::parameterOf(prev, head) and
isOutput0 = false and
isOutput = true
or
result = I::returnOf(prev, head)
// `ReturnValue` is only allowed in the input of flow summaries (hence `isOutput = false`),
// and the target of the return value (e.g. `Argument[X].ReturnValue`) should be fetched not
// from a post-update argument node (hence `isOutput0 = false`)
result = I::returnOf(prev, head) and
isOutput0 = false and
isOutput = false
or
componentLevelStep(head) and
result = prev
result = prev and
isOutput = isOutput0
)
}

Expand All @@ -312,8 +333,8 @@ module SummaryFlow<Input I> implements Output<I> {
|
callable.propagatesFlow(input, output, true) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This predicate is not a store step, should it have the same treatment? (The commit title indicates maybe not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should have the same treatment, good question though.


Expand All @@ -325,8 +346,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasLoadSummary(callable, content, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -338,8 +359,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasStoreSummary(callable, content, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -354,8 +375,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasLoadStoreSummary(callable, loadContent, storeContent, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -369,8 +390,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasWithoutContentSummary(callable, filter, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -384,8 +405,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasWithContentSummary(callable, filter, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}
}
5 changes: 3 additions & 2 deletions python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackerSpecific.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,11 @@ private module SummaryTypeTrackerInput implements SummaryTypeTracker::Input {
predicate return = FlowSummary::SummaryComponent::return/0;

// Relating nodes to summaries
Node argumentOf(Node call, SummaryComponent arg) {
Node argumentOf(Node call, SummaryComponent arg, boolean isPostUpdate) {
exists(DataFlowDispatch::ParameterPosition pos |
arg = FlowSummary::SummaryComponent::argument(pos) and
argumentPositionMatch(call, result, pos)
argumentPositionMatch(call, result, pos) and
isPostUpdate = [false, true] // todo: implement when/if Python uses post-update nodes in type tracking
)
}

Expand Down
6 changes: 3 additions & 3 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,11 +596,11 @@ private module Cached {
entrySsaDefinition(n) and
not LocalFlow::localFlowSsaParamInput(_, n)
or
TypeTrackerSpecific::storeStepIntoSourceNode(_, n, _)
TypeTrackerSpecific::basicStoreStep(_, n, _)
or
TypeTrackerSpecific::readStepIntoSourceNode(_, n, _)
TypeTrackerSpecific::basicLoadStep(_, n, _)
or
TypeTrackerSpecific::readStoreStepIntoSourceNode(_, n, _, _)
TypeTrackerSpecific::basicLoadStoreStep(_, n, _, _)
}

cached
Expand Down
16 changes: 10 additions & 6 deletions ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ predicate basicStoreStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet conten
* Holds if a store step `nodeFrom -> nodeTo` with `contents` exists, where the destination node
* is a post-update node that should be treated as a local source node.
*/
predicate storeStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
private predicate storeStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
// TODO: support SetterMethodCall inside TuplePattern
exists(ExprNodes::MethodCallCfgNode call |
contents
Expand Down Expand Up @@ -311,7 +311,7 @@ predicate basicLoadStep(Node nodeFrom, Node nodeTo, DataFlow::ContentSet content
* Holds if a read step `nodeFrom -> nodeTo` with `contents` exists, where the destination node
* should be treated as a local source node.
*/
predicate readStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
private predicate readStepIntoSourceNode(Node nodeFrom, Node nodeTo, DataFlow::ContentSet contents) {
DataFlowPrivate::readStepCommon(nodeFrom, contents, nodeTo)
}

Expand All @@ -330,7 +330,7 @@ predicate basicLoadStoreStep(
* Holds if a read+store step `nodeFrom -> nodeTo` exists, where the destination node
* should be treated as a local source node.
*/
predicate readStoreStepIntoSourceNode(
private predicate readStoreStepIntoSourceNode(
Node nodeFrom, Node nodeTo, DataFlow::ContentSet loadContent, DataFlow::ContentSet storeContent
) {
exists(DataFlowPrivate::SynthSplatParameterShiftNode shift |
Expand Down Expand Up @@ -441,10 +441,14 @@ private module SummaryTypeTrackerInput implements SummaryTypeTracker::Input {
class SummarizedCallable = FlowSummary::SummarizedCallable;

// Relating nodes to summaries
Node argumentOf(Node call, SummaryComponent arg) {
exists(DataFlowDispatch::ParameterPosition pos |
Node argumentOf(Node call, SummaryComponent arg, boolean isPostUpdate) {
exists(DataFlowDispatch::ParameterPosition pos, DataFlowPrivate::ArgumentNode n |
arg = SummaryComponent::argument(pos) and
argumentPositionMatch(call.asExpr(), result, pos)
argumentPositionMatch(call.asExpr(), n, pos)
|
isPostUpdate = false and result = n
or
isPostUpdate = true and result.(DataFlowPublic::PostUpdateNode).getPreUpdateNode() = n
)
}

Expand Down
71 changes: 46 additions & 25 deletions ruby/ql/lib/codeql/ruby/typetracking/internal/SummaryTypeTracker.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,12 @@ signature module Input {
}

// Relating nodes to summaries
/** Gets a dataflow node respresenting the argument of `call` indicated by `arg`. */
Node argumentOf(Node call, SummaryComponent arg);
/**
* Gets a dataflow node respresenting the argument of `call` indicated by `arg`.
*
* Returns the post-update node of the argument when `isPostUpdate` is true.
*/
Node argumentOf(Node call, SummaryComponent arg, boolean isPostUpdate);

/** Gets a dataflow node respresenting the parameter of `callable` indicated by `param`. */
Node parameterOf(Node callable, SummaryComponent param);
Expand Down Expand Up @@ -221,14 +225,18 @@ module SummaryFlow<Input I> implements Output<I> {

/**
* Gets a data flow `I::Node` corresponding an argument or return value of `call`,
* as specified by `component`.
* as specified by `component`. `isOutput` indicates whether the node represents
* an output node or an input node.
*/
bindingset[call, component]
private I::Node evaluateSummaryComponentLocal(I::Node call, I::SummaryComponent component) {
result = I::argumentOf(call, component)
private I::Node evaluateSummaryComponentLocal(
I::Node call, I::SummaryComponent component, boolean isOutput
) {
result = I::argumentOf(call, component, isOutput)
or
component = I::return() and
result = call
result = call and
isOutput = true
}

/**
Expand Down Expand Up @@ -280,27 +288,40 @@ module SummaryFlow<Input I> implements Output<I> {
*/
pragma[nomagic]
private I::Node evaluateSummaryComponentStackLocal(
I::SummarizedCallable callable, I::Node call, I::SummaryComponentStack stack
I::SummarizedCallable callable, I::Node call, I::SummaryComponentStack stack, boolean isOutput
) {
exists(I::SummaryComponent component |
dependsOnSummaryComponentStackLeaf(callable, component) and
stack = I::singleton(component) and
call = I::callTo(callable) and
result = evaluateSummaryComponentLocal(call, component)
result = evaluateSummaryComponentLocal(call, component, isOutput)
)
or
exists(I::Node prev, I::SummaryComponent head, I::SummaryComponentStack tail |
prev = evaluateSummaryComponentStackLocal(callable, call, tail) and
exists(
I::Node prev, I::SummaryComponent head, I::SummaryComponentStack tail, boolean isOutput0
|
prev = evaluateSummaryComponentStackLocal(callable, call, tail, isOutput0) and
dependsOnSummaryComponentStackConsLocal(callable, pragma[only_bind_into](head),
pragma[only_bind_out](tail)) and
stack = I::push(pragma[only_bind_out](head), pragma[only_bind_out](tail))
|
result = I::parameterOf(prev, head)
// `Parameter[X]` is only allowed in the output of flow summaries (hence `isOutput = true`),
// however the target of the parameter (e.g. `Argument[Y].Parameter[X]`) should be fetched
// not from a post-update argument node (hence `isOutput0 = false`)
result = I::parameterOf(prev, head) and
isOutput0 = false and
isOutput = true
or
result = I::returnOf(prev, head)
// `ReturnValue` is only allowed in the input of flow summaries (hence `isOutput = false`),
// and the target of the return value (e.g. `Argument[X].ReturnValue`) should be fetched not
// from a post-update argument node (hence `isOutput0 = false`)
result = I::returnOf(prev, head) and
isOutput0 = false and
isOutput = false
or
componentLevelStep(head) and
result = prev
result = prev and
isOutput = isOutput0
)
}

Expand All @@ -312,8 +333,8 @@ module SummaryFlow<Input I> implements Output<I> {
|
callable.propagatesFlow(input, output, true) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -325,8 +346,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasLoadSummary(callable, content, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -338,8 +359,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasStoreSummary(callable, content, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -354,8 +375,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasLoadStoreSummary(callable, loadContent, storeContent, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -369,8 +390,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasWithoutContentSummary(callable, filter, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}

Expand All @@ -384,8 +405,8 @@ module SummaryFlow<Input I> implements Output<I> {
hasWithContentSummary(callable, filter, pragma[only_bind_into](input),
pragma[only_bind_into](output)) and
call = I::callTo(callable) and
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output)
nodeFrom = evaluateSummaryComponentStackLocal(callable, call, input, false) and
nodeTo = evaluateSummaryComponentStackLocal(callable, call, output, true)
)
}
}
Loading