Re: [ ruby-Patches-5111 ] Instability around popen due to missing rb_thread_atfork

Hi,

At Wed, 19 Jul 2006 06:36:13 +0900,
<[e-mail]> wrote in [ruby-core:08262]:
> A Ruby program with multiple threads making calls to IO.popen
> (or indirectly via backticks) can get some nasty race
> conditions if existing threads get a chance to run in the
> child.
> 
> This can happen, for example, if the rb_warn("Insecure world
> writable dir...") from file.c:path_check_0() gets invoked in
> the child.
> 
> Solution is to add a call to 'rb_thread_atfork()' in the
> child after fork() in order to kill off those other threads.

Sounds reasonable.

> On a related theme, it seems like the 'rb_proc_exec' in
> process.c:pipe_open() should be protected with rb_protect or
> similar, to prevent any exceptions raised during
> rb_proc_exec() from being propagated to the soon-to-be-dead
> main thread in the child process. I'm not familiar enough
> with use of rb_protect to provide a proper patch for that.

The point would be any exceptions must not be raised after
fork.  It should be done in 1.9 almost, but it's bad time to
backport that change from trunk now.


Index: process.c
===================================================================
RCS file: /cvs/ruby/src/ruby/process.c,v
retrieving revision 1.92.2.30
diff -p -U 2 -r1.92.2.30 process.c
--- process.c	18 Jul 2006 05:53:33 -0000	1.92.2.30
+++ process.c	19 Jul 2006 06:32:44 -0000
@@ -1181,4 +1181,36 @@ proc_spawn(sv)
 #endif
 
+struct rb_exec_arg {
+    int argc;
+    VALUE *argv;
+    VALUE prog;
+};
+
+static VALUE
+proc_exec_args(earg)
+    VALUE earg;
+{
+    struct rb_exec_arg *e = (struct rb_exec_arg *)earg;
+    int argc = e->argc;
+    VALUE *argv = e->argv;
+    VALUE prog = e->prog;
+    int i;
+
+    if (prog) {
+	SafeStringValue(prog);
+	e->prog = prog;
+    }
+    for (i = 0; i < argc; i++) {
+	SafeStringValue(argv[i]);
+    }
+    if (argc == 1 && prog == 0) {
+	i = rb_proc_exec(RSTRING(argv[0])->ptr);
+    }
+    else {
+	i = proc_exec_n(argc, argv, prog);
+    }
+    return (VALUE)i;
+}
+
 /*
  *  call-seq:
@@ -1213,4 +1245,5 @@ rb_f_exec(argc, argv)
     VALUE prog = 0;
     VALUE tmp;
+    struct rb_exec_arg earg;
 
     if (argc == 0) {
@@ -1225,15 +1258,9 @@ rb_f_exec(argc, argv)
 	prog = RARRAY(tmp)->ptr[0];
 	argv[0] = RARRAY(tmp)->ptr[1];
-	SafeStringValue(prog);
-    }
-    if (argc == 1 && prog == 0) {
-	VALUE cmd = argv[0];
-
-	SafeStringValue(cmd);
-	rb_proc_exec(RSTRING(cmd)->ptr);
-    }
-    else {
-	proc_exec_n(argc, argv, prog);
     }
+    earg.argc = argc;
+    earg.argv = argv;
+    earg.prog = prog;
+    proc_exec_args(&earg);
     rb_sys_fail(RSTRING(argv[0])->ptr);
     return Qnil;		/* dummy */
@@ -1492,5 +1519,5 @@ rb_f_system(argc, argv)
     volatile VALUE prog = 0;
     int pid;
-    int i;
+    struct rb_exec_arg earg;
 
     fflush(stdout);
@@ -1508,20 +1535,13 @@ rb_f_system(argc, argv)
 	argv[0] = RARRAY(argv[0])->ptr[1];
     }
+    earg.argc = argc;
+    earg.argv = argv;
+    earg.prog = prog;
 
-    if (prog) {
-	SafeStringValue(prog);
-    }
-    for (i = 0; i < argc; i++) {
-	SafeStringValue(argv[i]);
-    }
   retry:
     switch (pid = fork()) {
       case 0:
-	if (argc == 1 && prog == 0) {
-	    rb_proc_exec(RSTRING(argv[0])->ptr);
-	}
-	else {
-	    proc_exec_n(argc, argv, prog);
-	}
+	rb_thread_atfork();
+	rb_protect(proc_exec_args, (VALUE)&earg, NULL);
 	_exit(127);
 	break;			/* not reached */

Index: io.c
===================================================================
RCS file: /cvs/ruby/src/ruby/io.c,v
retrieving revision 1.412
diff -p -u -2 -r1.412 io.c
--- io.c	3 Jul 2006 01:44:05 -0000	1.412
+++ io.c	19 Jul 2006 07:11:34 -0000
@@ -2949,4 +2949,5 @@ popen_exec(void *pp)
     int fd;
 
+    rb_thread_atfork();
     popen_redirect(p);
     for (fd = 3; fd < NOFILE; fd++) {
Index: process.c
===================================================================
RCS file: /cvs/ruby/src/ruby/process.c,v
retrieving revision 1.149
diff -p -u -2 -r1.149 process.c
--- process.c	18 Jul 2006 14:55:28 -0000	1.149
+++ process.c	19 Jul 2006 07:22:40 -0000
@@ -155,5 +155,5 @@ static VALUE
 get_ppid(void)
 {
-  rb_secure(2);
+    rb_secure(2);
 #ifdef _WIN32
     return INT2FIX(0);
@@ -921,5 +921,4 @@ proc_exec_v(char **argv, const char *pro
     if (!prog)
 	prog = argv[0];
-    security(prog);
     prog = dln_find_exe(prog, 0);
     if (!prog) {
@@ -1043,6 +1042,6 @@ rb_proc_exec(const char *str)
     memcpy(ss, str, s-str);
     ss[s-str] = '\0';
-    if (*a++ = strtok(ss, " \t")) {
-	while (t = strtok(NULL, " \t")) {
+    if ((*a++ = strtok(ss, " \t")) != 0) {
+	while ((t = strtok(NULL, " \t")) != 0) {
 	    *a++ = t;
 	}
@@ -1066,7 +1065,5 @@ rb_proc_exec(const char *str)
 #else
 static int
-proc_spawn_v(argv, prog)
-    char **argv;
-    char *prog;
+proc_spawn_v(char **argv, char *prog)
 {
     char *extension;
@@ -1167,4 +1164,5 @@ rb_check_argv(int argc, VALUE *argv)
     VALUE tmp, prog;
     int i;
+    const char *name = 0;
 
     if (argc == 0) {
@@ -1181,9 +1179,12 @@ rb_check_argv(int argc, VALUE *argv)
 	argv[0] = RARRAY(tmp)->ptr[1];
 	SafeStringValue(prog);
+	prog = rb_str_new4(prog);
+	name = RSTRING(prog)->ptr;
     }
     for (i = 0; i < argc; i++) {
 	SafeStringValue(argv[i]);
+	argv[i] = rb_str_new4(argv[i]);
     }
-    security(RSTRING(prog ? prog : argv[0])->ptr);
+    security(name ? name : RSTRING(argv[0])->ptr);
     return prog;
 }
@@ -1261,4 +1262,11 @@ rb_exec(const struct rb_exec_arg *e)
 }
 
+static int
+rb_exec_atfork(void* arg)
+{
+    rb_thread_atfork();
+    return rb_exec(arg);
+}
+
 #ifdef HAVE_FORK
 #ifdef FD_CLOEXEC
@@ -1543,5 +1551,5 @@ rb_spawn(int argc, VALUE *argv)
     earg.argv = argv;
     earg.prog = prog ? RSTRING(prog)->ptr : 0;
-    status = rb_fork(&status, (int (*)(void*))rb_exec, &earg);
+    status = rb_fork(&status, rb_exec_atfork, &earg);
     if (prog && argc) argv[0] = prog;
 #elif defined HAVE_SPAWNV


-- 
Nobu Nakada