C++: Missing return-value check for scanf-like functions #1076 #10033
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
d10c
commented
Aug 12, 2022
- Update .gitignore for .vscode/*.log temporaries
- C++: Add test and placeholder query.
- C++: First working. We now prefer flagging the cases where the variable was initialized, as in real world cases we haven't seen it done safely.
- Add more (false-negative) MissingCheckScanf tests
- Add more MissingCheckScanf test cases
These keep getting added, by the Makefile extension I believe.
…le was initialized, as in real world cases we haven't seen it done safely.
Comment on lines
1
to
19
| /** | ||
| * @name TODO | ||
| * @description TODO | ||
| * @kind problem | ||
| * @problem.severity warning | ||
| * @security-severity TODO | ||
| * @precision TODO | ||
| * @id cpp/missing-check-scanf | ||
| * @tags TODO | ||
| */ | ||
|
|
||
| import cpp | ||
| import semmle.code.cpp.commons.Scanf | ||
|
|
||
| from ScanfFunction scanf, FunctionCall fc | ||
| where | ||
| fc.getTarget() = scanf and | ||
| fc instanceof ExprInVoidContext | ||
| select fc, "This is a call to scanf." |
Check warning
Code scanning / CodeQL
Missing security metadata
d999e0e to
3e71ef2
Compare
geoffw0
reviewed
Aug 15, 2022
Also, include a couple of other test cases for related corner cases, namely, to catch stores-before-uses and another case of aliasing.
geoffw0
reviewed
Aug 16, 2022
Still needs to be tested further. But currently returns results on the `unbit/uwsgi` database: see https://github.com/github/semmle-code/actions/runs/2868369365
The problem with the current query is that instructions can be in a cycle, so the scanf argument itself is viewed as an unguarded use.
The other problem is that this query is slow on a real database, like unbit/uwsgi. Particularly during the step called: ScanfOutput::getASubsequentSameValuedInstruction#dispred#f0820431#ff#antijoin_rhs#2 Perhaps it can be optimized further.
|
TODO: Remove a Cartesian Product! This query can run for hours and run out of space: https://github.com/github/semmle-code/runs/7886119315?check_suite_focus=true#step:6:54 The offender is |
To quiet the QL-on-QL "implicit this" warnings.
Offender: getASameValuedInstruction This reduced its runtime on unbit/uwsgi from 1+min to 10ms. Re-running MRVA top 100, and it still looks like there are a few DBs on which it takes over 30 minutes to run this query. So further optimization is warranted.
The query is now shorter, simpler, more correct, and after some local testing, faster. MRVA top 100 and DCA runs pending.
|
MRVA top 10 run as of this commit: https://github.com/github/semmle-code/actions/runs/2914985782 |
|
Superceded by squashed PR #10163 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.