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=39508#toc
Repository:
rL LLVM
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:
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
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
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.
> > 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.
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
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 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 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
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
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:45
@@ +44,3 @@
+ CheckerContext ) {
+ const Expr *CE = Call.getOriginExpr();
+
ygribov wrote:
> It seems that other checkers do more or
ygribov marked 15 inline comments as done.
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:45
@@ +44,3 @@
+ CheckerContext ) {
+ 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
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
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
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
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.
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
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 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
>
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 -
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
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
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
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
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.
> 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.
> 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
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
30 matches
Mail list logo
