Shared: Improvements to content-sensitive model generation #20915
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hvitved
force-pushed
the
content-flow-ap-limit
branch
from
6fb68ee to
8b5dbe2
Compare
hvitved
changed the title
accessPathLimit in content flow
| FlowFeature getAFeature() { result = ContentConfig::getAFeature() } | ||
|
|
||
| predicate accessPathLimit = ContentConfig::accessPathLimit/0; | ||
| predicate accessPathLimit = ContentConfig::accessPathLimitInternal/0; |
Check warning
Code scanning / CodeQL
Dead code Warning
hvitved
force-pushed
the
content-flow-ap-limit
branch
from
8b5dbe2 to
666855d
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves content-sensitive model generation by distinguishing between the access path limit used internally and the limit used for generating models, enabling better filtering of invalid access paths.
- Introduced separate
accessPathLimit()andaccessPathLimitInternal()to allow different limits for internal data flow analysis versus model generation - Enhanced
validateAccessPath()to exclude models with unsupported content types by validating all content in access paths - Refactored validation to occur earlier in the pipeline by moving
validateAccessPathchecks into theapiFlowpredicate before counting models per parameter
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| shared/mad/codeql/mad/modelgenerator/internal/ModelGeneratorImpl.qll | Enhanced validateAccessPath documentation and logic; refactored validation to occur in apiFlow predicate before model counting |
| shared/dataflow/codeql/dataflow/internal/ContentDataFlowImpl.qll | Added accessPathLimitInternal() method and length() method for access paths to support internal vs external limit distinction |
| rust/ql/test/utils-tests/modelgenerator/option.rs | Updated test expectations to reflect newly generated summaries that were previously missing due to access path limits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Perhaps we should also run DCA (at least for C# model generation) to check the impact on performance and number of models generated?
| 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() } |
There was a problem hiding this comment.
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.
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Looks good to me (if DCA is happy)!
|
The go language test failures are unrelated. Feel free to ignore them. They come from some changes I made recently to the go consistency checks. They should be resolved now, so re-running CI and/or rebasing should fix them. |
3 participants
Contents.validateAccessPathbefore checking whether at most 3 models exist for a given parameter.