Page MenuHomePhabricator

[analyzer][NFC][MallocChecker] Convert many parameters into CallEvent
ClosedPublic

Authored by Szelethus on Mar 1 2020, 5:14 PM.

Details

Summary

Exactly what it says on the tin! This is clearly not the end of the road in this direction, the parameters could be merged far more with the use of CallEvent or a better value type in the CallDescriptionMap, but this was shockingly difficult enough on its own. I expect that simplifying the file further will be far easier moving forward.

The end goal is to research how we could create a more mature checker interaction infrastructure for more complicated C++ modeling, and I'm pretty sure that being able successfully split up our giants is the first step in this direction.

Also... as to why I added so much LLVM_UNREACHABLE annotations, I'll provide the following image:

One could explain why this restroom looks like this -- but you could guess what the horrifying story must have been :^)

Diff Detail

Event Timeline

Szelethus created this revision.Mar 1 2020, 5:14 PM
Charusso accepted this revision.Mar 3 2020, 3:53 AM

Also... as to why I added so much LLVM_UNREACHABLE annotations

I believe it is not necessary to add LLVM_NODISCARD, but as it perfectly works here, I like it.

I would mention the mailing list here as well: http://lists.llvm.org/pipermail/cfe-dev/2020-February/064754.html

This revision is now accepted and ready to land.Mar 3 2020, 3:53 AM
Charusso added a comment.EditedMar 3 2020, 4:00 AM

I have added green markers to all of your patches as well. I really appreciate the simplification of the MallocChecker. May you would commit it as soon as possible, given that you have nailed what Artem has suggested. Cool^2.

Some nits inline.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1017–1022

What is the main reason of getting rid of the state parameter? I think this could make using these functions more error prone overall as it would be easier to introduce splits in the exploded graph this way (if we somehow wanted to model a function as successive calls of some other functions).

1273–1275

Use auto here to avoid repeating the type.

1676

In functions where we only use ArgExpr to retrieve the corresponding SVal we could pass the SVal in the first place.

Szelethus marked 2 inline comments as done.Mar 3 2020, 5:38 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1017–1022

Refer to @NoQ's earlier inline.

I think this could make using these functions more error prone overall as it would be easier to introduce splits in the exploded graph this way (if we somehow wanted to model a function as successive calls of some other functions).

Why would the removal of a ProgramStateRef parameter cause that?

1676

Long term, I plan to find a better value type for the CallDescriptionMap where information such as this could be retrieved easier, which is why I focused on nothing but the task at hand.

While I would like to see MallocChecker in a healthier state, at times I find the development effort I'm putting into it questionable, so I'll probably stop at the point where I can comfortably split it.

NoQ accepted this revision.Mar 15 2020, 10:50 PM

Must have been annoying, thanks a lot!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
441–444

Excellent, we should totally do this more often wherever necessary.

631

Is the state parameter still necessary here?

970

CallDescriptionMap was supposed to verify this for us.

1017–1022

if we somehow wanted to model a function as successive calls of some other functions

If we ever want to compose our evaluations, we should do it as a sequence of composable operations on a state, i.e. by not only accepting the state but also returning the state.

2810–2811

Maybe iterate over CallEvent's argument SVals directly? We probably don't have a method for this but it doesn't sound like a too uncommon of a task.

This revision was automatically updated to reflect the committed changes.
balazske added inline comments.Wed, May 20, 6:00 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

This should be added to avoid later crash (probably not needed for every check kind?):

const FunctionDecl *FD = C.getCalleeDecl(CE);
if (!FD)
  return;
Szelethus marked 3 inline comments as done.Wed, May 20, 6:39 AM

I though I addressed the inlines months ago, but seems like I did not. I'll get this done post-commit. Oops.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

Not all CallEvents have a corresponding FunctionDecl or a CallExpr, for instance, CXXAllocatorCall corresponds with CXXNewExpr, which is not a CallExpr, but it is handled by this checker. For this reason, I decided to move this check to the individual modeling functions.

Szelethus marked 4 inline comments as done.Wed, May 20, 6:48 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
631

Not as per its current usages in the code, but it does make sense, if we do some prechecks and modify the state before calling this functions.

1194

Oh I'm sorry, do we have an actual crash resulting from this?

balazske added inline comments.Wed, May 20, 7:13 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

I did not look into it by detail but the problem is in MallocChecker::checkOwnershipAttr with a null FD. Probably it is enough to insert a return at that point (makes the crash gone on that analyzed project).

Szelethus marked 2 inline comments as done.Wed, May 20, 7:41 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

I'm sorry but I don't have enough to follow up on this. On the master branch, everything seems to work smoothly for me (all tests pass) and I didn't get any buildbot failures. Can you post a code snippet?

Szelethus marked an inline comment as done.Wed, May 20, 7:44 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

If you have a project name, I can analyze that myself to save you the trouble.

Hi, @Szelethus, I don't know exactly which of the changes (this one, https://reviews.llvm.org/D75430, or https://reviews.llvm.org/D75431) causes a crash on SQLite, but it's definitely one of these.

Steps to reproduce

clang -cc1 -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -Werror=implicit-function-declaration -analyze -disable-free -main-file-name sqlite3.c -analyzer-store=region -analyzer-opt-analyze-nested-blocks -analyzer-checker=core -analyzer-checker=apiModeling -analyzer-checker=unix -analyzer-checker=osx -analyzer-checker=security.insecureAPI.decodeValueOfObjCType -analyzer-checker=deadcode -analyzer-checker=security.insecureAPI.UncheckedReturn -analyzer-checker=security.insecureAPI.getpw -analyzer-checker=security.insecureAPI.gets -analyzer-checker=security.insecureAPI.mktemp -analyzer-checker=security.insecureAPI.mkstemp -analyzer-checker=security.insecureAPI.vfork -analyzer-checker=nullability.NullPassedToNonnull -analyzer-checker=nullability.NullReturnedFromNonnull -analyzer-output plist -w -setup-static-analyzer -analyzer-config-compatibility-mode=true -mrelocation-model pic -pic-level 2 -mthread-model posix -mframe-pointer=all -fno-strict-return -fno-rounding-math -munwind-tables -faligned-alloc-unavailable -target-cpu core2 -dwarf-column-info -target-linker-version 556.6 -Wno-reorder-init-list -Wno-implicit-int-float-conversion -Wno-c99-designator -Wno-final-dtor-non-final-class -Wno-extra-semi-stmt -Wno-misleading-indentation -Wno-quoted-include-in-framework-header -Wno-implicit-fallthrough -Wno-enum-enum-conversion -Wno-enum-float-conversion -ferror-limit 19 -stack-protector 1 -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fgnuc-version=4.2.1 -fmax-type-align=16 -analyzer-checker=alpha.unix.SimpleStream,alpha.security.taint,cplusplus.NewDeleteLeaks,core,cplusplus,deadcode,security,unix,osx,nullability -analyzer-config serialize-stats=true,stable-report-filename=true -x c sqlite3-258aa5.c

Output

Assertion failed: (FromPtr && ToPtr && "By this point, FreeMemAux and MallocMemAux should have checked " "whether the argument or the return value is symbolic!"), function ReallocMemAux, file /Users/vsavchenko/source/llvm-project/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp, line 2409.

Attached file is the exact version of SQLite source code to reproduce the issue:

Thanks, I'll get right to it.

balazske added inline comments.Wed, May 20, 8:17 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

Could not debug the problem because debug info was not there for unknown reason (no exact line locations were printed). But the project was emacs 26.3 configure --with-xpm=no --with-gif=no --with-tiff=no --with-gnutls=no, failed at one of the the first files with CodeChecker analyze.

Szelethus marked an inline comment as done.Wed, May 20, 8:47 AM

This will take a while for me to fix (couple hours, wanna wait for creduce to finish running, and I needed to compile llvm on the server as well), but I'll get it done probably today.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1194

No worries, I'll attend to it myself. Thank you for the quick response though!