zaks.anna added a comment.
Thanks for the patch!
Some minor comments inline.
What happens when this checker and the security.insecureAPI.vfork are enabled
at the same time?
Did you run this checker on a large body of code? Did it find any issues? What
is needed to turn this into a non-alpha checker?
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:10
@@ +9,3 @@
+//
+// This file defines vfork checker which checks for dangerous uses of vfork.
+//
----------------
Let's move the more detailed comment that describes what the checker does here.
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:44
@@ +43,3 @@
+// Pattern matches to extract lhs in `lhs = vfork()' statement.
+static const VarDecl *GetAssignedVariable(const CallEvent &Call,
+ CheckerContext &C) {
----------------
This should be added as a utility function. Does this exist elsewhere?
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:63
@@ +62,3 @@
+ return D;
+ } while (0);
+
----------------
Can this be rewritten so that it is more clear why this terminates?
Also, I'd prefer to use "while(true)".
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:155
@@ +154,3 @@
+ if (!BT)
+ BT.reset(new BuiltinBug(this, "Dangerous construct in vforked process"));
+
----------------
"a vforked process"?
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:160
@@ +159,3 @@
+
+ os << What << " is prohibited after successful vfork";
+
----------------
"after successful vfork" -> "after a successful vfork"?
Devin, are we missing an article here and in the other error messages?
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:163
@@ +162,3 @@
+ auto Report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+ C.emitReport(std::move(Report));
+ }
----------------
Ideally, we would point out where the vfork has occurred along the path with a
BugReportVisitor. (But it's not a blocker.)
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:208
@@ +207,3 @@
+ && !isCallWhitelisted(Call.getCalleeIdentifier(), C))
+ reportBug("Calling functions (except exec(3) or _exit(2))", C);
+}
----------------
We are not listing the full whitelist here. Should we drop the "(except ..)"
from the message?
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:226
@@ +225,3 @@
+
+ reportBug("Assigning variables (except return value of vfork)", C);
+}
----------------
"except return value" -> "except the return value"? Or maybe we should drop the
"(except ...)" here as well to make the message shorter.
================
Comment at: test/Analysis/vfork.c:24
@@ +23,3 @@
+ // Ensure that writing variables is prohibited.
+ x = 0; // expected-warning{{}}
+ break;
----------------
We should check for the exact expected warning in regression tests.
http://reviews.llvm.org/D14014
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
