Skip to content

Shared: Improvements to content-sensitive model generation #20915

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
hvitved merged 2 commits into from
Dec 3, 2025
Merged

Shared: Improvements to content-sensitive model generation #20915

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
666855d
Shared: Improvements to content-sensitive model generation
hvitved Nov 26, 2025
3ba256a
C#/Java: Go back to access path limit 2
hvitved Dec 3, 2025
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
8 changes: 3 additions & 5 deletions rust/ql/test/utils-tests/modelgenerator/option.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,7 @@ impl<T> MyOption<T> {
}

// summary=<test::option::MyOption>::insert;Argument[0];Argument[self].Reference.Field[test::option::MyOption::MySome(0)];value;dfc-generated
// This summary is currently missing because of access path limit
// summary-MISSING=<test::option::MyOption>::insert;Argument[0];ReturnValue.Reference;value;dfc-generated
// summary=<test::option::MyOption>::insert;Argument[0];ReturnValue.Reference;value;dfc-generated
// The content of `self` is overwritten so it does not flow to the return value.
// SPURIOUS-summary=<test::option::MyOption>::insert;Argument[self].Reference.Field[test::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated
pub fn insert(&mut self, value: T) -> &mut T {
Expand All @@ -311,8 +310,7 @@ impl<T> MyOption<T> {
}

// summary=<test::option::MyOption>::get_or_insert;Argument[0];Argument[self].Reference.Field[test::option::MyOption::MySome(0)];value;dfc-generated
// This summary is currently missing because of access path limit
// summary-MISSING=<test::option::MyOption>::get_or_insert;Argument[0];ReturnValue.Reference;value;dfc-generated
// summary=<test::option::MyOption>::get_or_insert;Argument[0];ReturnValue.Reference;value;dfc-generated
// summary=<test::option::MyOption>::get_or_insert;Argument[self].Reference.Field[test::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated
pub fn get_or_insert(&mut self, value: T) -> &mut T {
self.get_or_insert_with(|| value)
Expand All @@ -328,7 +326,7 @@ impl<T> MyOption<T> {

// summary=<test::option::MyOption>::get_or_insert_with;Argument[self].Reference.Field[test::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated
// summary=<test::option::MyOption>::get_or_insert_with;Argument[0].ReturnValue;Argument[self].Reference.Field[test::option::MyOption::MySome(0)];value;dfc-generated
// SPURIOUS-summary=<test::option::MyOption>::get_or_insert_with;Argument[0];Argument[self].Reference.Field[test::option::MyOption::MySome(0)];value;dfc-generated
// summary=<test::option::MyOption>::get_or_insert_with;Argument[0].ReturnValue;ReturnValue.Reference;value;dfc-generated
pub fn get_or_insert_with<F>(&mut self, f: F) -> &mut T
where
F: FnOnce() -> T,
Expand Down
15 changes: 14 additions & 1 deletion shared/dataflow/codeql/dataflow/internal/ContentDataFlowImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@
/** Gets a limit on the number of reads out of sources and number of stores into sinks. */
default int accessPathLimit() { result = Lang::accessPathLimit() }

/** Gets the access path limit used in the internal invocation of the standard data flow library. */
default int accessPathLimitInternal() { result = Lang::accessPathLimit() }
Copy link
Contributor

@michaelnebel michaelnebel Dec 3, 2025

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

@hvitved hvitved Dec 3, 2025

Choose a reason for hiding this comment

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

It is not currently used, but I thought I'd add it in case e.g. C# model generation blows up, in which case we can revert it back to 2.


/** Holds if `c` is relevant for reads out of sources or stores into sinks. */
default predicate isRelevantContent(ContentSet c) { any() }
}
Expand Down Expand Up @@ -110,7 +113,7 @@

FlowFeature getAFeature() { result = ContentConfig::getAFeature() }

predicate accessPathLimit = ContentConfig::accessPathLimit/0;
predicate accessPathLimit = ContentConfig::accessPathLimitInternal/0;

Check warning

Code scanning / CodeQL

Dead code Warning

This code is never used, and it's not publicly exported.

// needed to record reads/stores inside summarized callables
predicate includeHiddenNodes() { any() }
Expand Down Expand Up @@ -274,6 +277,16 @@
)
}

/**
* Gets the length of this access path.
*/
int length() {
this = TAccessPathNil() and
result = 0
or
result = this.getTail().length() + 1
}

/**
* Gets the content set at index `i` in this access path, if any.
*/
Expand Down
23 changes: 12 additions & 11 deletions shared/mad/codeql/mad/modelgenerator/internal/ModelGeneratorImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,14 +703,17 @@ module MakeModelGeneratorFactory<
}

/**
* Holds if the access path `ap` is not a parameter or returnvalue of a callback
* stored in a field.
* Holds if `ap` is valid for generating summary models.
*
* That is, we currently don't include summaries that rely on parameters or return values
* of callbacks stored in fields.
* We currently don't include summaries that rely on parameters or return values
* of callbacks stored in fields, as those are not supported by the data flow
* library.
*
* We also exclude access paths with contents not supported by `printContent`.
*/
private predicate validateAccessPath(PropagateContentFlow::AccessPath ap) {
not (mentionsField(ap) and mentionsCallback(ap))
not (mentionsField(ap) and mentionsCallback(ap)) and
forall(int i | i in [0 .. ap.length() - 1] | exists(getContent(ap, i)))
}

private predicate apiFlow(
Expand All @@ -720,7 +723,9 @@ module MakeModelGeneratorFactory<
) {
PropagateContentFlow::flow(p, reads, returnNodeExt, stores, preservesValue) and
getEnclosingCallable(returnNodeExt) = api and
getEnclosingCallable(p) = api
getEnclosingCallable(p) = api and
validateAccessPath(reads) and
validateAccessPath(stores)
}

/**
Expand Down Expand Up @@ -763,9 +768,7 @@ module MakeModelGeneratorFactory<
PropagateContentFlow::AccessPath reads, ReturnNodeExt returnNodeExt,
PropagateContentFlow::AccessPath stores, boolean preservesValue
) {
PropagateContentFlow::flow(p, reads, returnNodeExt, stores, preservesValue) and
getEnclosingCallable(returnNodeExt) = api and
getEnclosingCallable(p) = api and
apiFlow(api, p, reads, returnNodeExt, stores, preservesValue) and
p = api.getARelevantParameterNode()
}

Expand Down Expand Up @@ -956,8 +959,6 @@ module MakeModelGeneratorFactory<
input = parameterNodeAsExactInput(p) + printReadAccessPath(reads) and
output = getExactOutput(returnNodeExt) + printStoreAccessPath(stores) and
input != output and
validateAccessPath(reads) and
validateAccessPath(stores) and
(
if mentionsField(reads) or mentionsField(stores)
then lift = false and api.isRelevant()
Expand Down
Loading