Page MenuHomePhabricator

[BoundV2] ArrayBoundV2 checks if the extent is tainted
Needs ReviewPublic

Authored by gamesh411 on May 13 2022, 1:03 AM.

Details

Summary

Use upperbound instead of offset to check if the extent of memory region
being accessed is tainted or not.

Original author: steakhal

Diff Detail

Event Timeline

gamesh411 created this revision.May 13 2022, 1:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
gamesh411 requested review of this revision.May 13 2022, 1:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 1:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gamesh411 updated this revision to Diff 429159.May 13 2022, 1:10 AM

add analyzer tag

martong added inline comments.May 13 2022, 5:51 AM
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
208

Could you please explain why we change rawOffset to *upperBoundToCheck? And perhaps the same explanation could infiltrate into the checker's code itself as a comment to upperbound.

clang/test/Analysis/taint-diagnostic-visitor.c
46–48

Could we get rid of the seemingly unrelated malloc taint report by using an array on the stack?

steakhal added inline comments.May 13 2022, 6:13 AM
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
208

In the test attached you can see that the extent is tainted, not the offset.
Thus checking the offset for taint won't suffice.
The bug condition should depend on the calculation itself, which is basically what is done here.

clang/test/Analysis/taint-diagnostic-visitor.c
46–48

No, we need the extent to be tainted.

martong added inline comments.May 13 2022, 7:20 AM
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
208

Okay makes sense, but then please update the comment

// If we are under constrained and the index variables are tainted, report.

to mention the extent as well.