[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Artem Dergachev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5a11233a2fa5: [analyzer] VforkChecker: allow execve after 
vfork. (authored by dergachev.a).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629

Files:
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/vfork.c


Index: clang/test/Analysis/vfork.c
===
--- clang/test/Analysis/vfork.c
+++ clang/test/Analysis/vfork.c
@@ -6,7 +6,7 @@
 void foo();
 
 // Ensure that child process is properly checked.
-int f1(int x) {
+int f1(int x, int y) {
   pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is 
insecure}}
   if (pid != 0)
 return 0;
@@ -16,7 +16,29 @@
 // Ensure that modifying pid is ok.
 pid = 1; // no-warning
 // Ensure that calling whitelisted routines is ok.
-execl("", "", 0); // no-warning
+switch (y) {
+case 0:
+  execl("", "", 0); // no-warning
+  break;
+case 1:
+  execle("", "", 0); // no-warning
+  break;
+case 2:
+  execlp("", "", 0); // no-warning
+  break;
+case 3:
+  execv("", NULL); // no-warning
+  break;
+case 4:
+  execve("", NULL, NULL); // no-warning
+  break;
+case 5:
+  execvp("", NULL); // no-warning
+  break;
+case 6:
+  execvpe("", NULL, NULL); // no-warning
+  break;
+}
 _exit(1); // no-warning
 break;
   case 1:
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ clang/test/Analysis/Inputs/system-header-simulator.h
@@ -98,6 +98,12 @@
 pid_t fork(void);
 pid_t vfork(void);
 int execl(const char *path, const char *arg, ...);
+int execle(const char *path, const char *arg, ...);
+int execlp(const char *file, const char *arg, ...);
+int execv(const char *path, char *const argv[]);
+int execve(const char *path, char *const argv[], char *const envp[]);
+int execvp(const char *file, char *const argv[]);
+int execvpe(const char *file, char *const argv[], char *const envp[]);
 
 void exit(int status) __attribute__ ((__noreturn__));
 void _exit(int status) __attribute__ ((__noreturn__));
Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -98,12 +98,13 @@
   if (VforkWhitelist.empty()) {
 // According to manpage.
 const char *ids[] = {
-  "_exit",
   "_Exit",
+  "_exit",
   "execl",
-  "execlp",
   "execle",
+  "execlp",
   "execv",
+  "execve",
   "execvp",
   "execvpe",
   nullptr


Index: clang/test/Analysis/vfork.c
===
--- clang/test/Analysis/vfork.c
+++ clang/test/Analysis/vfork.c
@@ -6,7 +6,7 @@
 void foo();
 
 // Ensure that child process is properly checked.
-int f1(int x) {
+int f1(int x, int y) {
   pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is insecure}}
   if (pid != 0)
 return 0;
@@ -16,7 +16,29 @@
 // Ensure that modifying pid is ok.
 pid = 1; // no-warning
 // Ensure that calling whitelisted routines is ok.
-execl("", "", 0); // no-warning
+switch (y) {
+case 0:
+  execl("", "", 0); // no-warning
+  break;
+case 1:
+  execle("", "", 0); // no-warning
+  break;
+case 2:
+  execlp("", "", 0); // no-warning
+  break;
+case 3:
+  execv("", NULL); // no-warning
+  break;
+case 4:
+  execve("", NULL, NULL); // no-warning
+  break;
+case 5:
+  execvp("", NULL); // no-warning
+  break;
+case 6:
+  execvpe("", NULL, NULL); // no-warning
+  break;
+}
 _exit(1); // no-warning
 break;
   case 1:
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ clang/test/Analysis/Inputs/system-header-simulator.h
@@ -98,6 +98,12 @@
 pid_t fork(void);
 pid_t vfork(void);
 int execl(const char *path, const char *arg, ...);
+int execle(const char *path, const char *arg, ...);
+int execlp(const char *file, const char *arg, ...);
+int execv(const char *path, char *const argv[]);
+int execve(const char *path, char *const argv[], char *const envp[]);
+int execvp(const char *file, char *const argv[]);
+int execvpe(const char *file, char *const argv[], char *const envp[]);
 
 void exit(int status) __attribute__ ((__noreturn__));
 void _exit(int status) __attribute__ ((__noreturn__));
Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Thank you!! I'll commit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Jan Včelák via Phabricator via cfe-commits]

janvcelak updated this revision to Diff 245025.
janvcelak added a comment.

I'm attaching updated version with tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629

Files:
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/vfork.c


Index: clang/test/Analysis/vfork.c
===
--- clang/test/Analysis/vfork.c
+++ clang/test/Analysis/vfork.c
@@ -6,7 +6,7 @@
 void foo();
 
 // Ensure that child process is properly checked.
-int f1(int x) {
+int f1(int x, int y) {
   pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is 
insecure}}
   if (pid != 0)
 return 0;
@@ -16,7 +16,29 @@
 // Ensure that modifying pid is ok.
 pid = 1; // no-warning
 // Ensure that calling whitelisted routines is ok.
-execl("", "", 0); // no-warning
+switch (y) {
+case 0:
+  execl("", "", 0); // no-warning
+  break;
+case 1:
+  execle("", "", 0); // no-warning
+  break;
+case 2:
+  execlp("", "", 0); // no-warning
+  break;
+case 3:
+  execv("", NULL); // no-warning
+  break;
+case 4:
+  execve("", NULL, NULL); // no-warning
+  break;
+case 5:
+  execvp("", NULL); // no-warning
+  break;
+case 6:
+  execvpe("", NULL, NULL); // no-warning
+  break;
+}
 _exit(1); // no-warning
 break;
   case 1:
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ clang/test/Analysis/Inputs/system-header-simulator.h
@@ -98,6 +98,12 @@
 pid_t fork(void);
 pid_t vfork(void);
 int execl(const char *path, const char *arg, ...);
+int execle(const char *path, const char *arg, ...);
+int execlp(const char *file, const char *arg, ...);
+int execv(const char *path, char *const argv[]);
+int execve(const char *path, char *const argv[], char *const envp[]);
+int execvp(const char *file, char *const argv[]);
+int execvpe(const char *file, char *const argv[], char *const envp[]);
 
 void exit(int status) __attribute__ ((__noreturn__));
 void _exit(int status) __attribute__ ((__noreturn__));
Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -98,12 +98,13 @@
   if (VforkWhitelist.empty()) {
 // According to manpage.
 const char *ids[] = {
-  "_exit",
   "_Exit",
+  "_exit",
   "execl",
-  "execlp",
   "execle",
+  "execlp",
   "execv",
+  "execve",
   "execvp",
   "execvpe",
   nullptr


Index: clang/test/Analysis/vfork.c
===
--- clang/test/Analysis/vfork.c
+++ clang/test/Analysis/vfork.c
@@ -6,7 +6,7 @@
 void foo();
 
 // Ensure that child process is properly checked.
-int f1(int x) {
+int f1(int x, int y) {
   pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is insecure}}
   if (pid != 0)
 return 0;
@@ -16,7 +16,29 @@
 // Ensure that modifying pid is ok.
 pid = 1; // no-warning
 // Ensure that calling whitelisted routines is ok.
-execl("", "", 0); // no-warning
+switch (y) {
+case 0:
+  execl("", "", 0); // no-warning
+  break;
+case 1:
+  execle("", "", 0); // no-warning
+  break;
+case 2:
+  execlp("", "", 0); // no-warning
+  break;
+case 3:
+  execv("", NULL); // no-warning
+  break;
+case 4:
+  execve("", NULL, NULL); // no-warning
+  break;
+case 5:
+  execvp("", NULL); // no-warning
+  break;
+case 6:
+  execvpe("", NULL, NULL); // no-warning
+  break;
+}
 _exit(1); // no-warning
 break;
   case 1:
Index: clang/test/Analysis/Inputs/system-header-simulator.h
===
--- clang/test/Analysis/Inputs/system-header-simulator.h
+++ clang/test/Analysis/Inputs/system-header-simulator.h
@@ -98,6 +98,12 @@
 pid_t fork(void);
 pid_t vfork(void);
 int execl(const char *path, const char *arg, ...);
+int execle(const char *path, const char *arg, ...);
+int execlp(const char *file, const char *arg, ...);
+int execv(const char *path, char *const argv[]);
+int execve(const char *path, char *const argv[], char *const envp[]);
+int execvp(const char *file, char *const argv[]);
+int execvpe(const char *file, char *const argv[], char *const envp[]);
 
 void exit(int status) __attribute__ ((__noreturn__));
 void _exit(int status) __attribute__ ((__noreturn__));
Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- 

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

LGTM granted the test is supplied, nice catch!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Let's still add the test. It'll make sure that you understand exactly what 
you're doing and that there aren't more aspects to this problem. More tests 
never hurt and it's a local culture to have at least some tests for every 
commit that changes the behavior. Our tests are also very cheap to add.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Jan Včelák via Phabricator via cfe-commits]

janvcelak added a comment.
Herald added a subscriber: martong.

Please, can the reviewers respond to my comments? I would like to know if this 
needs additional revision or if it can be accepted as is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Jan Včelák via Phabricator via cfe-commits]

janvcelak added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp:109
   "execvpe",
+  "execve",
   nullptr

xazax.hun wrote:
> Well, this is not the case now, but I wonder if it would also make sense to 
> sort this list alphabetically. 
I had this idea as well. The list is currently not sorted but seems to match 
the order of the functions as they are listed in the `exec(3)` manual page so I 
kept it.

Let me know what is preferred and I can update the diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Jan Včelák via Phabricator via cfe-commits]

janvcelak added a comment.

In D73629#1847750 , @NoQ wrote:

> We should add a test for this (cf. `test/Analysis/vfork.c`).


There is already a test to check if the whitelist works by making sure that 
call to `execl` doesn't generate the warning. I think there is no point adding 
`execve` into the tests unless we want to add a test for each of the functions 
in the list. However, let me know if you want it added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp:109
   "execvpe",
+  "execve",
   nullptr

Well, this is not the case now, but I wonder if it would also make sense to 
sort this list alphabetically. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Jan Včelák via Phabricator via cfe-commits]

janvcelak updated this revision to Diff 241245.
janvcelak added a comment.

Sorry. I missed that step in contribution guidelines. I'm attaching updated 
version with larger context.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629

Files:
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -106,6 +106,7 @@
   "execv",
   "execvp",
   "execvpe",
+  "execve",
   nullptr
 };
 


Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -106,6 +106,7 @@
   "execv",
   "execvp",
   "execvpe",
+  "execve",
   nullptr
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yup, thanks, nice catch!

We should add a test for this (cf. `test/Analysis/vfork.c`).

Also do you have any immediate opinions on 
https://bugs.llvm.org/show_bug.cgi?id=43871? 'Cause i'm super confused. Like, 
it's trivial to fix but i'm not sure what the correct behavior is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: NoQ.
Charusso added a comment.

Hey, thanks! The patch looks great, but please note that we do the reviews with 
context using `git diff -U99` or uploading with `arc` 
(https://secure.phabricator.com/book/phabricator/article/arcanist/).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Jan Včelák via Phabricator via cfe-commits]

janvcelak created this revision.
janvcelak added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.

`execve` is missing in the list of functions that are allowed after `vfork()`. 
As a result, clang analyzer reports the following false positive:

  #include 
  
  int main(int argc, char *argv[])
  {
char *a[] = {"true", NULL};
char *e[] = {NULL};
if (vfork() == 0) {
execve("/bin/true", a, e);
_exit(1);
}
return 0;
  }



  $ scan-build clang -Wall -c repro.c  
  scan-build: Using '/usr/bin/clang-9' for static analysis
  repro.c:7:6: warning: Call to function 'vfork' is insecure as it can lead to 
denial of service situations in the parent process. Replace calls to vfork with 
calls to the safer 'posix_spawn' function
  if (vfork() == 0) {
  ^
  repro.c:8:3: warning: This function call is prohibited after a successful 
vfork
  execve("/bin/true", a, e);
  ^
  2 warnings generated.
  scan-build: 2 bugs found.
  scan-build: Run 'scan-view /tmp/scan-build-2020-01-29-162705-3770808-1' to 
examine bug reports.

The list of exec functions in the code is take from the `exec(3)` man page 
which are just a fronted for `execve(2)`. Quoting the manual page:

> The  exec() family of functions replaces the current process image with a new 
> process image.  The functions escribed in this manual page are front-ends for 
> execve(2).  (See the manual page for execve(2) for further details about the 
> replacement of the current process image.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73629

Files:
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -106,6 +106,7 @@
   "execv",
   "execvp",
   "execvpe",
+  "execve",
   nullptr
 };
 


Index: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -106,6 +106,7 @@
   "execv",
   "execvp",
   "execvpe",
+  "execve",
   nullptr
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits