This revision was automatically updated to reflect the committed changes.
Closed by commit rL252285: [analyzer] Add VforkChecker to find unsafe code in
vforked process. (authored by ygribov).
Changed prior to commit:
http://reviews.llvm.org/D14014?vs=39345&id=39508#toc
Repository:
rL LLVM
h
zaks.anna added a comment.
Sounds good!
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zaks.anna added a comment.
Ok. Thanks!
I'll commit shortly.
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ygribov added a comment.
> I now also test for collaboration with security.InsecureAPI.vfork.
Should probably clarify: I've added checks to testcase to verify that new
checker properly interacts with (existing) InsecureAPI.vfork checker,
http://reviews.llvm.org/D14014
_
ygribov updated this revision to Diff 39345.
ygribov added a comment.
Moved to unix package (and thus enabled by default). I now also test for
collaboration with security.InsecureAPI.vfork.
http://reviews.llvm.org/D14014
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
zaks.anna added a comment.
> > What is needed to turn this into a non-alpha checker?
>
>
> I guess that's question for maintainers) We have to consider that vfork is
> used rarely enough.
Please, make this into a non-alpha, turned on by default.
Thank you!
Anna.
http://reviews.llvm.org/
ygribov marked 2 inline comments as done.
ygribov added a comment.
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ygribov updated this revision to Diff 39040.
ygribov added a comment.
Updated warning messages.
http://reviews.llvm.org/D14014
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/S
zaks.anna added a comment.
Thanks for addressing all the comments. A couple of additional ones below.
Comment at: test/Analysis/vfork.c:60
@@ +59,3 @@
+// Ensure that returning from function is prohibited.
+return 0;
+ }
Shouldn't the warning appear her
ygribov added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:118
@@ +117,3 @@
+std::tie(VD, Init) = parseAssignment(S);
+if (VD && Init)
+ S = Init;
Semantics is slightly changed for assignment case here: originally S
ygribov marked 5 inline comments as done.
ygribov added a comment.
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ygribov updated this revision to Diff 38926.
ygribov marked 2 inline comments as done.
ygribov added a comment.
Updated after review.
http://reviews.llvm.org/D14014
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/Stati
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:45
@@ +44,3 @@
+ CheckerContext &C) {
+ const Expr *CE = Call.getOriginExpr();
+
ygribov wrote:
> It seems that other checkers do more or l
ygribov marked 15 inline comments as done.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:45
@@ +44,3 @@
+ CheckerContext &C) {
+ const Expr *CE = Call.getOriginExpr();
+
It seems that other checkers do more or less the
ygribov updated this revision to Diff 38740.
ygribov added a comment.
Updated after Anna's review.
http://reviews.llvm.org/D14014
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/VforkChecker.cpp
test/Analysis/Inputs/s
ygribov added a comment.
> What happens when this checker and the security.insecureAPI.vfork are enabled
> at the same time?
Both checkers will emit warnings independently (which I think is ok).
> Did you run this checker on a large body of code? Did it find any issues?
Yes, I've ran it on A
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 c
ygribov added a comment.
Done!
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dcoughlin added a comment.
LGTM. Can you update the summary to a commit message? Then I will commit.
Thanks for upstreaming!
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailma
ygribov added a comment.
> > This is a valid concern because it greatly complicates enablement of
> > VforkChecker for a casual user.
>
>
> I think at the very least I can check that InsecureAPI is enable and issue a
> warning to user.
Actually I think that's not a huge problem. InsecureA
ygribov marked 43 inline comments as done.
ygribov added a comment.
http://reviews.llvm.org/D14014
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ygribov removed rL LLVM as the repository for this revision.
ygribov updated this revision to Diff 38423.
ygribov added a comment.
Updated based on review.
http://reviews.llvm.org/D14014
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAn
a.sidorin added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:47
@@ +46,3 @@
+
+ bool isChildProcess(const ProgramStateRef State) const;
+
ygribov wrote:
> a.sidorin wrote:
> > I think it's a good idea to make some functions static and
ygribov added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:149
@@ +148,3 @@
+
+ // see if it's an ordinary assignment
+ do {
ygribov wrote:
> a.sidorin wrote:
> > You can use early return to escape do{}.
> In this particular case - y
ygribov added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:47
@@ +46,3 @@
+
+ bool isChildProcess(const ProgramStateRef State) const;
+
a.sidorin wrote:
> I think it's a good idea to make some functions static and/or move them out
>
a.sidorin added a comment.
I put some suggestions in inline comments.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:47
@@ +46,3 @@
+
+ bool isChildProcess(const ProgramStateRef State) const;
+
I think it's a good idea to make some functions static an
ygribov added a comment.
> This is a valid concern because it greatly complicates enablement of
> VforkChecker for a casual user.
I think at the very least I can check that InsecureAPI is enable and issue a
warning to user.
Repository:
rL LLVM
http://reviews.llvm.org/D14014
___
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
dcoughlin added a comment.
> For example, doing 'x = malloc(4); *x = 0;' in the child process seems fine
> to me.
I don't think this is necessarily safe. For example, malloc() could end up both
modifying memory shared between the child and parent process but only modifying
process state for t
ygribov added a comment.
> I didn't know vfork() is in regular use.
Neither did I, until I had to debug the damned code. Actually Android has some
5 or 6 uses of it.
> I'm also not convinced that flagging all stores in the vfork child process is
> the right thing to do,
> since the FP rate
dcoughlin added a comment.
Hi Yury,
Thanks for the patch. This is definitely interesting for upstream!
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. This check is on
vsk added a subscriber: vsk.
vsk added a comment.
Thanks for the patch.
I didn't know vfork() is in regular use. Afaict copy-on-write fork() and
threading both solve a similar (the same?) problem. I'm also not convinced that
flagging all stores in the vfork child process is the right thing to d
ygribov created this revision.
ygribov added reviewers: zaks.anna, dcoughlin, jordan_rose, krememek.
ygribov added a subscriber: cfe-commits.
ygribov set the repository for this revision to rL LLVM.
Hi all,
This checker verifies that vfork is used safely. Vforked process shared stack
with parent
33 matches
Mail list logo
