ygribov added a comment. > One thing to note (which I assume you are already aware of) is that we > already have the "security.insecureAPI.vfork" checker,
> an AST check that warns on *every* use of vfork. Yes, I was aware of this one. Unfortunately as I mentioned above vfork is probably to stay with us for quite a while. > This check is on by default (which I think makes sense, given the variety of > ways that vfork is problematic) -- > so users of your more permissive check would have to disable the default > check. This is a valid concern because it greatly complicates enablement of VforkChecker for a casual user. Do we have some mechanism so that one checker (here VforkChecker) could disable/modify behavior of it's less precise companion (insecureAPI)? This sounds like a generally useful feature (although it damages the modularity of checkers). ================ Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:360 @@ +359,3 @@ +def VforkChecker : Checker<"Vfork">, + HelpText<"Check for proper usage of vfork.">, + DescFile<"VforkChecker.cpp">; ---------------- dcoughlin wrote: > The convention here is to not have a '.' after the help text. Got it. ================ Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:61 @@ +60,3 @@ + + // these are used to track vfork state + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; ---------------- dcoughlin wrote: > LLVM convention is to use capitalization and punctuation for comments. (See > http://llvm.org/docs/CodingStandards.html#commenting). Thanks, I should probably re-read the std once again. ================ Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:74 @@ +73,3 @@ +// region child is allowed to write) +REGISTER_TRAIT_WITH_PROGRAMSTATE(VforkLhs, const void *) +#define VFORK_LHS_NONE ((void *)(uintptr_t)1) ---------------- dcoughlin wrote: > Is it possible to use a name that implies a more semantic notion? For > example, "VforkResultRegion"? That would be the fourth iteration of the name) But yes, "Result region" sounds more descriptive. ================ Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:102 @@ +101,3 @@ + + for (SmallVectorImpl<const IdentifierInfo *>::iterator + I = VforkWhitelist.begin(), E = VforkWhitelist.end(); ---------------- dcoughlin wrote: > I think it is better to use SmallSet for VforkWhitelist rather than duplicate > that functionality here. Ok. ================ Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:115 @@ +114,3 @@ + BT_BadChildCode.reset( + new BuiltinBug(this, "Dangerous construct in vforked process", + "Prohibited construct after successful " ---------------- dcoughlin wrote: > What do you think about making the warnings more descriptive here? For > example, you could one warning saying that "Child can only modify variable > used to store result of vfork()", one that says "Child can only call _exit() > or `exec()` family of functions", "Child must not return from function > calling vfork()", etc. > > These descriptions would help the user not have to guess what they are doing > wrong. Yes, given the public ignorance about vfork's idiosyncrasies that makes good sense. ================ Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:118 @@ +117,3 @@ + "vfork")); + ExplodedNode *N = C.generateSink(C.getState(), C.getPredecessor()); + auto Report = llvm::make_unique<BugReport>(*BT_BadChildCode, ---------------- dcoughlin wrote: > You should use `C.generateErrorNode()` here, which expresses the intent to > create a node for the purposes of error reporting. Will do. ================ Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:135 @@ +134,3 @@ + do { + const DeclStmt *PD = dyn_cast_or_null<DeclStmt>(P); + if (!PD) ---------------- dcoughlin wrote: > For dyn_cast and friends and you use `auto` to avoid repeating the type > twice. For example: > > ``` > const auto *PD = dyn_cast_or_null<DeclStmt>(P); > ``` True. This was forward-ported from our ancient version so lacks C++11 beauties. ================ Comment at: test/Analysis/vfork-1.c:1 @@ +1,2 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core.builtin.NoReturnFunctions,alpha.unix.Vfork %s -verify + ---------------- dcoughlin wrote: > Tests generally need to have all of core turned on (e.g., > `-analyzer-checker=core,alpha.unix.Vfork`) because the analyzer implements > key functionality in the core checkers. (It is not safe to run a > path-sensitive checker without core turned on). Ok, good to know. ================ Comment at: test/Analysis/vfork-1.c:68 @@ +67,3 @@ + if (vfork() == 0) + _exit(1); + x = 0; ---------------- dcoughlin wrote: > I think it would be good to add `// no-warning` on the lines that shouldn't > warn. This will make it easier for people tracking down test failures to know > that there really shouldn't be a warning on that line. Sounds like a good maintenance practice, I'll definitely add it. ================ Comment at: test/Analysis/vfork-2.c:1 @@ +1,2 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.unix.Vfork %s -verify + ---------------- dcoughlin wrote: > Is it possible to combine this with the other test file? If not, can you > rename the files to be more descriptive than "-1" or "-2". This will help > people adding future tests decide which file they should go in. Vfork-2.c is actually dedicated to model the bug in libxtables. I can surely merge both tests. Repository: rL LLVM http://reviews.llvm.org/D14014 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
