[PATCH] D97699: [analyzer] Add InvalidPtrChecker

This revision was automatically updated to reflect the committed changes. zukatsinadze marked an inline comment as done. Closed by commit rG811b1736d91b: [analyzer] Add InvalidPtrChecker (authored by zukatsinadze). Changed prior to commit: https://reviews.llvm.org/D97699?vs=372237=376902#toc

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

martong added a comment. Gentle ping, I think we can land this, please do so. Let me know if there is an issue with, .e.g missing commit rights, etc... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:42 DynamicTypeChecker.cpp + cert/InvalidPtrChecker.cpp EnumCastOutOfRangeChecker.cpp Please, insert this in its sorted place. CHANGES SINCE LAST ACTION

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Excellent! Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699 ___ cfe-commits mailing

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze marked 4 inline comments as done. zukatsinadze added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:163 +// memory region returned by previous call of this function +REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap,

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze updated this revision to Diff 372237. zukatsinadze added a comment. Thanks for the review @martong I've fixed all the suggestions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699 Files: clang/docs/analyzer/checkers.rst

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:159 +// Note: This pointer has type 'const MemRegion *' +REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const void *) + martong wrote: > Why is it `const

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

martong added a comment. Nice work! Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:159 +// Note: This pointer has type 'const MemRegion *' +REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const void *) + Why is it `const void *`? Why

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

martong added a comment. > And about `checkDeadSymbols`, I get your point, but I am interested why > checker has different behavior on these two examples: > > a1 int main(int argc, char **argv, char *envp[]) { > a2 putenv((char*) "NAME=VALUE"); // envp invalidated > a3 envp[0]; //

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

steakhal resigned from this revision. steakhal added a comment. I think it looks good, I don't have much objection to this. I've also participated in the offline-review of this patch, so the current shape of this reflects my intentions, thus I resign. At the same time, I'm requesting others to

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

Charusso resigned from this revision. Charusso added a comment. @NoQ, what do you think? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze marked an inline comment as done and an inline comment as not done. zukatsinadze added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:947 + +} // end "alpha.cert.env" + balazske wrote: > I have multiple issues

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze updated this revision to Diff 350656. zukatsinadze marked 2 inline comments as done. zukatsinadze added a comment. @balazske Thanks for the comments! Updated diff after suggested changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:180 +// Note: This pointer has type 'const MemRegion *' +REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const void *) + balazske wrote: > Is it not possible

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

balazske added a comment. I have not too deep knowledge about checker development but still found (hopefully valid) issues, even if only a part of them. Comment at: clang/docs/analyzer/checkers.rst:2117 +// envp may no longer point to the current environment +

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

steakhal added a comment. Overall I think it's a useful checker not only for checking the `getenv()` but a bunch of other functions as well, which might return a pointer to a statically allocated buffer. The implementation could be polished a bit but it's ok I think. About the produced

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze added a comment. Herald added a subscriber: manas. @NoQ can you please have another look at this? I think it will be a useful checker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze updated this revision to Diff 335334. zukatsinadze edited the summary of this revision. zukatsinadze added a comment. Gentle ping - Diff with context - Added some more tests - Updated documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze added inline comments. Comment at: clang/docs/analyzer/checkers.rst:2103-2104 +puts(envp[i]); +// envp may no longer point to the current environment +// this program has unanticipated behavior. + } zukatsinadze wrote: >

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze added inline comments. Comment at: clang/docs/analyzer/checkers.rst:2103-2104 +puts(envp[i]); +// envp may no longer point to the current environment +// this program has unanticipated behavior. + } Charusso wrote: > NoQ

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

Charusso added a comment. In D97699#2601804 , @NoQ wrote: > Why are you even tracking `reg_$0`? It's obvious from the structure of > the symbol that it's an environment pointer, there's no need to keep it in > maps. `envp` is confusable with `argv`:

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze added a comment. @NoQ, thanks for the comments. In D97699#2601804 , @NoQ wrote: > ... and whether flagged code is expected to be always invalid. C standard says "may be overwritten", so I guess it's undefined behavior. In D97699#2601804

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

NoQ added a comment. CERT rules are typically very vague and don't map nicely into specific static analysis algorithms. A lot of CERT rules flag valid code as well as bugs. I want you to explain what *exactly* does the checker checks (say, in the form of a state machine, i.e. what //sequences

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze updated this revision to Diff 327173. zukatsinadze added a comment. Removed code repetition from the tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699 Files:

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze added a comment. Attaching results of CodeChecker run on some projects.F15697529: cc_results.zip Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze updated this revision to Diff 327150. zukatsinadze added a comment. Fixed docs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97699/new/ https://reviews.llvm.org/D97699 Files: clang/docs/analyzer/checkers.rst

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze added a comment. Please suggest which package to use for the checker. CERT rules are ENV, however, it deals with non-ENV functions as well. Also, I am having a problem with `checkDeadSymbols`, it is similar to one xazax.hun faced here: http://reviews.llvm.org/D14203 (many many

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

zukatsinadze created this revision. zukatsinadze added reviewers: NoQ, vsavchenko, Charusso, Szelethus, martong. zukatsinadze added a project: clang. Herald added subscribers: steakhal, ASDenysPetrov, ormris, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware,