[2/3] ptest-runner: close all fds in child process.

Submitted by Sakib Sajal on May 31, 2019, 8:25 p.m. | Patch ID: 161855

Details

Message ID 20190531202552.21421-2-sakib.sajal@windriver.com
State New
Headers show

Commit Message

Sakib Sajal May 31, 2019, 8:25 p.m.
When running 'ptest-runner bash', a test named vredir fails
due to too many open file descriptors.
The test sets the maximum number of open file descriptors
to 6 (ulimit -n 6). The test passes by interactively calling
run-ptest, but not when using ptest-runner.
Security-focused applications will close all unused file descriptors in
the child after forking but before execing.

From the failed ptest log:
   run-vredir
   87,88c87,88
   < ./vredir6.sub: line 10: /dev/null: Too many open files
   < ./vredir6.sub: line 13: /dev/null: Too many open files
   FAIL: run-vredir

Upstream-Status: Submitted [yocto@yoctoproject.org]

Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
Signed-off-by: Randy Macleod <randy.macleod@windriver.com>
---
 ...l-file-descriptors-after-completing-.patch | 69 +++++++++++++++++++
 .../ptest-runner/ptest-runner_2.3.1.bb        |  4 +-
 2 files changed, 72 insertions(+), 1 deletion(-)
 create mode 100644 meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch

Patch hide | download patch | download mbox

diff --git a/meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch b/meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch
new file mode 100644
index 0000000000..6cc941b853
--- /dev/null
+++ b/meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch
@@ -0,0 +1,69 @@ 
+From 63c1b0d154a8028084a26dd523efa379420d8a5d Mon Sep 17 00:00:00 2001
+From: Sakib Sajal <sakib.sajal@windriver.com>
+Date: Thu, 30 May 2019 16:02:04 -0400
+Subject: [PATCH] utils.c: close all file descriptors after completing a ptest
+
+vredir ptest fails since too many file descriptors were open.
+
+From the failed ptest log:
+   run-vredir
+   87,88c87,88
+   < ./vredir6.sub: line 10: /dev/null: Too many open files
+   < ./vredir6.sub: line 13: /dev/null: Too many open files
+   FAIL: run-vredir
+
+Added function to close file descriptors before starting a new process.
+
+Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
+Signed-off-by: Randy Macleod <randy.macleod@windriver.com>
+---
+ utils.c | 19 +++++++++++++++++++
+ 1 file changed, 19 insertions(+)
+
+diff --git a/utils.c b/utils.c
+index 504df0b..05c2bfe 100644
+--- a/utils.c
++++ b/utils.c
+@@ -28,6 +28,7 @@
+ #include <fcntl.h>
+ #include <time.h>
+ #include <dirent.h>
++#include <sys/resource.h>
+ #include <sys/types.h>
+ #include <sys/wait.h>
+ #include <sys/stat.h>
+@@ -240,6 +241,23 @@ filter_ptests(struct ptest_list *head, char **ptests, int ptest_num)
+ 	return head_new;
+ }
+ 
++/* Close all fds from 3 up to 'ulimit -n'
++ * i.e. do not close STDIN, STDOUT, STDERR.
++ * Typically called in in a child process after forking
++ * but before exec as a good policy especially for security.
++ */ 
++static void
++close_fds(void)
++{
++	struct rlimit curr_lim;
++	getrlimit(RLIMIT_NOFILE, &curr_lim);
++
++	int fd;
++	for (fd=3; fd < curr_lim.rlim_cur; fd++) {
++		(void) close(fd);
++   	}
++}
++
+ static inline void
+ run_child(char *run_ptest, int fd_stdout, int fd_stderr)
+ {
+@@ -252,6 +270,7 @@ run_child(char *run_ptest, int fd_stdout, int fd_stderr)
+ 	dup2(fd_stdout, STDOUT_FILENO);
+ 	// XXX: Redirect stderr to stdout to avoid buffer ordering problems.
+ 	dup2(fd_stdout, STDERR_FILENO);
++	close_fds();
+ 	execv(run_ptest, argv);
+ 
+ 	exit(1);
+-- 
+2.20.1
+
diff --git a/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb b/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb
index e2eb258d0b..afa407b48b 100644
--- a/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb
+++ b/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb
@@ -13,7 +13,9 @@  PV = "2.3.1+git${SRCPV}"
 SRC_URI = "git://git.yoctoproject.org/ptest-runner2 \
  file://0001-utils-Ensure-stdout-stderr-are-flushed.patch \
  file://0002-use-process-groups-when-spawning.patch \
- file://0003-utils-Ensure-pipes-are-read-after-exit.patch"
+ file://0003-utils-Ensure-pipes-are-read-after-exit.patch
+ file://0004-utils.c-close-all-file-descriptors-after-completing-.patch \
+"
 
 S = "${WORKDIR}/git"
 

Comments

Randy MacLeod May 31, 2019, 8:35 p.m.
Your subject is:
   ptest-runner: close all fds in child process.
but that's a bit misleading. Since there are other
things to change, say:
   ptest-runner: close unused fds in child process.

On 5/31/19 4:25 PM, Sakib Sajal wrote:
> When running 'ptest-runner bash', a test named vredir fails
> due to too many open file descriptors.
> The test sets the maximum number of open file descriptors
> to 6 (ulimit -n 6). The test passes by interactively calling
> run-ptest, but not when using ptest-runner.
> Security-focused applications will close all unused file descriptors in
s/Security-focused/Well-behaved/

It's true that his is a security recommendation but here
it's more important to be 'well-behaved'.

> the child after forking but before execing.
> 
>  From the failed ptest log:
>     run-vredir
>     87,88c87,88
>     < ./vredir6.sub: line 10: /dev/null: Too many open files
>     < ./vredir6.sub: line 13: /dev/null: Too many open files
>     FAIL: run-vredir
> 
> Upstream-Status: Submitted [yocto@yoctoproject.org]

This goes in the patch header not the oe-core commit log.
We do it that way so that we can audit the .patch files to
see what their status is independent of recent commit logs.
Resend with that fixed please.

The rest of this commit seems fine.

../Randy

> 
> Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
> Signed-off-by: Randy Macleod <randy.macleod@windriver.com>
> ---
>   ...l-file-descriptors-after-completing-.patch | 69 +++++++++++++++++++
>   .../ptest-runner/ptest-runner_2.3.1.bb        |  4 +-
>   2 files changed, 72 insertions(+), 1 deletion(-)
>   create mode 100644 meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch
> 
> diff --git a/meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch b/meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch
> new file mode 100644
> index 0000000000..6cc941b853
> --- /dev/null
> +++ b/meta/recipes-support/ptest-runner/ptest-runner/0004-utils.c-close-all-file-descriptors-after-completing-.patch
> @@ -0,0 +1,69 @@
> +From 63c1b0d154a8028084a26dd523efa379420d8a5d Mon Sep 17 00:00:00 2001
> +From: Sakib Sajal <sakib.sajal@windriver.com>
> +Date: Thu, 30 May 2019 16:02:04 -0400
> +Subject: [PATCH] utils.c: close all file descriptors after completing a ptest
> +
> +vredir ptest fails since too many file descriptors were open.
> +
> +From the failed ptest log:
> +   run-vredir
> +   87,88c87,88
> +   < ./vredir6.sub: line 10: /dev/null: Too many open files
> +   < ./vredir6.sub: line 13: /dev/null: Too many open files
> +   FAIL: run-vredir
> +
> +Added function to close file descriptors before starting a new process.
> +
> +Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
> +Signed-off-by: Randy Macleod <randy.macleod@windriver.com>
> +---
> + utils.c | 19 +++++++++++++++++++
> + 1 file changed, 19 insertions(+)
> +
> +diff --git a/utils.c b/utils.c
> +index 504df0b..05c2bfe 100644
> +--- a/utils.c
> ++++ b/utils.c
> +@@ -28,6 +28,7 @@
> + #include <fcntl.h>
> + #include <time.h>
> + #include <dirent.h>
> ++#include <sys/resource.h>
> + #include <sys/types.h>
> + #include <sys/wait.h>
> + #include <sys/stat.h>
> +@@ -240,6 +241,23 @@ filter_ptests(struct ptest_list *head, char **ptests, int ptest_num)
> + 	return head_new;
> + }
> +
> ++/* Close all fds from 3 up to 'ulimit -n'
> ++ * i.e. do not close STDIN, STDOUT, STDERR.
> ++ * Typically called in in a child process after forking
> ++ * but before exec as a good policy especially for security.
> ++ */
> ++static void
> ++close_fds(void)
> ++{
> ++	struct rlimit curr_lim;
> ++	getrlimit(RLIMIT_NOFILE, &curr_lim);
> ++
> ++	int fd;
> ++	for (fd=3; fd < curr_lim.rlim_cur; fd++) {
> ++		(void) close(fd);
> ++   	}
> ++}
> ++
> + static inline void
> + run_child(char *run_ptest, int fd_stdout, int fd_stderr)
> + {
> +@@ -252,6 +270,7 @@ run_child(char *run_ptest, int fd_stdout, int fd_stderr)
> + 	dup2(fd_stdout, STDOUT_FILENO);
> + 	// XXX: Redirect stderr to stdout to avoid buffer ordering problems.
> + 	dup2(fd_stdout, STDERR_FILENO);
> ++	close_fds();
> + 	execv(run_ptest, argv);
> +
> + 	exit(1);
> +--
> +2.20.1
> +
> diff --git a/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb b/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb
> index e2eb258d0b..afa407b48b 100644
> --- a/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb
> +++ b/meta/recipes-support/ptest-runner/ptest-runner_2.3.1.bb
> @@ -13,7 +13,9 @@ PV = "2.3.1+git${SRCPV}"
>   SRC_URI = "git://git.yoctoproject.org/ptest-runner2 \
>    file://0001-utils-Ensure-stdout-stderr-are-flushed.patch \
>    file://0002-use-process-groups-when-spawning.patch \
> - file://0003-utils-Ensure-pipes-are-read-after-exit.patch"
> + file://0003-utils-Ensure-pipes-are-read-after-exit.patch
> + file://0004-utils.c-close-all-file-descriptors-after-completing-.patch \
> +"
>   
>   S = "${WORKDIR}/git"
>   
>
Richard Purdie May 31, 2019, 9:50 p.m.
On Fri, 2019-05-31 at 16:25 -0400, Sakib Sajal wrote:
> When running 'ptest-runner bash', a test named vredir fails
> due to too many open file descriptors.
> The test sets the maximum number of open file descriptors
> to 6 (ulimit -n 6). The test passes by interactively calling
> run-ptest, but not when using ptest-runner.
> Security-focused applications will close all unused file descriptors
> in
> the child after forking but before execing.
> 
> From the failed ptest log:
>    run-vredir
>    87,88c87,88
>    < ./vredir6.sub: line 10: /dev/null: Too many open files
>    < ./vredir6.sub: line 13: /dev/null: Too many open files
>    FAIL: run-vredir
> 
> Upstream-Status: Submitted [yocto@yoctoproject.org]
> 
> Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
> Signed-off-by: Randy Macleod <randy.macleod@windriver.com>

Thanks, this looks like a good fix.

Since I believe Anibal has merged this into the upstream source, could
you send a patch which updates the revision ptest-runner2 is using
instead please? That just then saves adding and then dropping the
patch.

Cheers,

Richard