Paths
| Differential D15227
[analyzer] Valist checkers. ClosedPublic Authored by xazax.hun on Dec 4 2015, 5:14 AM.
Details
Summary This patch introduces two new checks for incorrect usage of va_lists. They finds usages of uninitialized va_lists and leaked/unterminated va_lists. They can also find va_lists that are copied into itself. This check was developed by Donat Nagy (m1nagdon@gmail.com) and myself.
Diff Detail Event Timelinexazax.hun updated this object. Comment Actions Looks good overall; comments below. Please, provide more information on real world code evaluation. Which codebases this has been tested on? What was the false positive rate? How many real bugs were found/fixed? What is the criteria for taking it out of alpha? Please, write the commit message that will be used.
Comment Actions It was tested on gcc and rAthena (https://github.com/rathena/rathena). It did not find any issues in those projects, but I was able to find some issues that I artificially put into those projects.
xazax.hun edited edge metadata. xazax.hun marked 5 inline comments as done. Comment Actions
Comment Actions
Comment Actions The checker's in great shape! I see a few minor things, but that's it. The checks are split into two sections ("uninitialized" and "unterminated"), but there seem to be more auxiliary checks provided (eg. "copies into itself") that are on for both checkers, do you think you need to add more flags to make the checker more flexible, or maybe just merge everything into a single checker? Because plist tests are so heavy: if all you wanted to test was how your bug reporter visitor works, did you consider using -analyzer-output=text instead? (this is not the default output mode; it has note: diagnostics displaying the path, which you can catch with expected-note{{}}). Also, you could probably write special tests for the visitor, instead of testing the visitors on all tests, regardless of text/plist. But i'm merely sharing this hint, i don't think it's worth rewriting once we already have plist tests.
xazax.hun marked 5 inline comments as done. Comment Actions
This revision is now accepted and ready to land.Aug 17 2016, 8:19 AM Closed by commit rL279041: [analyzer] Added valist related checkers. (authored by xazax). · Explain WhyAug 18 2016, 1:51 AM This revision was automatically updated to reflect the committed changes. Comment Actions It looks like it broke some of the build bots. Error from the windows build bots: error: 'note' diagnostics expected but not seen: File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 96: va_list 'va' is copied onto itself File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 102: va_list 'va' is copied onto itself File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 108: Initialized va_list 'va' is overwritten by an uninitialized one error: 'note' diagnostics seen but not expected: Line 96: va_list va' is copied onto itself Line 102: va_list va' is copied onto itself Line 108: Initialized va_list va' is overwritten by an uninitialized one 12 errors generated. Somehow the beginning single quote is missing from the generated message. It is very strange. Error from other architectures: error: 'warning' diagnostics expected but not seen: File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 27: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 39: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array[3]' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem[0]' is leaked error: 'warning' diagnostics seen but not expected: File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 33: Initialized va_list 'fst' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem' is leaked error: 'note' diagnostics expected but not seen: File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 26: Initialized va_list File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 27: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 36: Initialized va_list File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 39: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array[3]' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem[0]' is leaked error: 'note' diagnostics seen but not expected: Line 31: Initialized va_list File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 33: Initialized va_list 'fst' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem' is leaked 17 errors generated. I suspect that slightly different AST is generated for those architectures that cause the different behavior. I will further investigate those problems. This revision is now accepted and ready to land.Aug 18 2016, 4:08 AM Comment Actions
Seems so, because on my machine when i append -triple hexagon-unknown-linux to the test run line, a bunch of things start failing. You may also consider hardcoding the runline in the current test and commit (which is so often a good idea), and fix other stuff in later commits. A proper investigation would be great, of course :) P.S. Just curious - do you plan to eventually model va_arg(), as we discussed with Michael Tandy in http://lists.llvm.org/pipermail/cfe-dev/2016-August/050202.html ? Closed by commit rL279427: Reapply "[analyzer] Added valist related checkers." (authored by xazax). · Explain WhyAug 22 2016, 4:32 AM This revision was automatically updated to reflect the committed changes. Comment Actions
I committed with the hardcoded triple in the runline for now.
It is not on my TODO list at the moment, but I might look into it in the distant future. Comment Actions Can we move this out of alpha? Have this checkers been tested on a large codebase? What are false positive rates? Thanks! Comment Actions
I have tested it on a few ~200k LOC C codebase and I did not see any major problem. There is one problem left though, when I committed this check some of the build bots broke, I suspect that slightly different AST is generated on some of the platforms. I temporarily fixed the issue by hardcoding the triple into the tests and I did not have time yet to investigate the source of the problems. But as far as I remember, this produced false negatives in the tests not false positives. Comment Actions
Could you double check that? Maybe you still have some notes in your mail box or just by looking at the code. Did none of the checks work or just some of them? Also, which platforms did this not work on? It would be great to move all of the useful checks out of alpha since alpha essentially means "unsupported" and we do not recommend turning those checkers on. Thanks! Comment Actions
I think I managed to solve the problems due to the different AST variants. In the future this should be abstracted away by the analyzer core. You can see the patch here: https://reviews.llvm.org/D30157 Early next week I will rerun this check on a larger open source project. If it does not give me too much false positives or crash, I think it is a good candidate to move out from alpha.
Revision Contents
Diff 53700 lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
test/Analysis/Inputs/system-header-simulator-for-valist.h
test/Analysis/valist-uninitialized.c
test/Analysis/valist-unterminated.c
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This looks like a general purpose utility that could be used by other checkers.
(Though, this code might not support all languages as is.)