Page MenuHomePhabricator

[analyzer] ApiModeling: Add buffer size arg constraint with multiplier involved
ClosedPublic

Authored by martong on Mar 31 2020, 8:44 AM.

Details

Summary

Further develop the buffer size argumentum constraint so it can handle sizes
that we can get by multiplying two variables.

Diff Detail

Event Timeline

martong created this revision.Mar 31 2020, 8:44 AM
balazske added inline comments.
clang/test/Analysis/std-c-library-functions-arg-constraints.c
163

s <= 3 is better here? We know that the buffer has space for 3 short's. But it is better to assume nothing about sizeof(short).

martong marked 2 inline comments as done.Apr 1 2020, 7:16 AM
martong added inline comments.
clang/test/Analysis/std-c-library-functions-arg-constraints.c
163

That would have been better if the range based constraint solver was able to solve the in-equation to s. So, even though the analyzer knows that s * 2 <= 6 it cannot reason about s itself. This is the limitation of the solver. I believe @baloghadamsoftware has a few patches that directs this deficiency: D49074 and D50256

Szelethus added inline comments.Apr 15 2020, 3:52 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
231–239

I don't know, these constructors don't look overly talkative.

986

So this is supposed to mean that the 1st argument has the size of the argument of 2nd argument times the 3rd argument? Unlike the other summaries, this feels a bit cryptic. Something like this might look better:

.ArgConstraint(0, BufferSize(/*BufSize*/1, /*BufSizeMultiplier*/))
.ArgConstraint(BufferSize(/*Buffer*/0, /*BufSize*/1, /*BufSizeMultiplier*/2))
.ArgConstraint(0, BufferSizeMul(1, 2))

WDYT?

martong updated this revision to Diff 262098.May 5 2020, 6:50 AM
martong marked 2 inline comments as done.
  • Rebase to latest parent patch
  • Create BufSize constraints in a more descriptive way
martong marked 3 inline comments as done.May 5 2020, 6:51 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
231–239

Yeah, renamed the params a bit.

986

Ok, I updated with the 2nd option. :)

Szelethus accepted this revision.May 5 2020, 9:38 AM

LGTM!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
225

fread-like

986

Very well!

This revision is now accepted and ready to land.May 5 2020, 9:38 AM

Overall looks good to me, I have one question inline.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1008

Why do we need these test functions? Above I saw fread as an example that requires this capability. Wouldn't it be better to make its summary utilize the new feature and use fread in tests? Do I miss something?

martong marked 3 inline comments as done.May 26 2020, 9:09 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1008

Yeah, we could test that with fread. However, in fread there are multiple argument constraints for the different args. I wanted a test function which has this arg constraint in an isolation. In my opinion it is good to have small isolated unit tests that test only one functionality. Also if we decide to remove or modify freads summary for any reason, then we should modify this test too, on the other hand, with this test function it is not a problem.

xazax.hun added inline comments.May 26 2020, 9:36 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1008

I see. The plan in the future is to split this checker up a bit. In this case I'd like to have these test functions in a separate test checker. Until then I'm fine with a FIXME/TODO.

martong marked 3 inline comments as done.May 26 2020, 9:47 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1008

They are already enabled only if a test checker is enabled, check out the enclosing if of this block:

if (ChecksEnabled[CK_StdCLibraryFunctionsTesterChecker]) {
xazax.hun added inline comments.May 26 2020, 9:51 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1008

Oh, sorry, my bad :)

martong updated this revision to Diff 267141.May 29 2020, 1:31 AM
martong marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.