Rework the clone wrapper to add an intermediate function to resolve a…

… defect.

Previously the clone(2) wrapper unconditionallity restored the system
environment.  It also invokes the checks to see if the user has requested
pseudo to be disabled or unloaded. Due to the semantics of clone, this caused
both the parent and child processes to be disabled or unloaded.

The new code adds an intermediate function, wrap_clone_child, that only
runs within the child context.  This way we can be sure to only disable/unload
pseudo from within the child process.  In addition, we avoid mucking with
the environment if CLONE_VM is set, since this will affect both parent and
child.

Signed-off-by: Mark Hatle <mark.hatle@windriver.com>

ChangeLog.txt

@@ -1,3 +1,7 @@
+2011-10-26:
+	* (mhatle) update clone wrapper to add an intermediate function
+	  to avoid setting environment variables in the parent.
+
 2011-10-20:
 	* (mhatle) change from internal PSEUDO_RELOADED to external
 	  PSEUDO_UNLOAD environment variable.  Enable external programs

ports/linux/newclone/guts/clone.c

@@ -1,5 +1,5 @@
 /* 
- * Copyright (c) 2008-2010 Wind River Systems; see
+ * Copyright (c) 2008-2011 Wind River Systems; see
  * guts/COPYRIGHT for information.
  *
  * static int
@@ -8,18 +8,23 @@
  */
 	/* because clone() doesn't actually continue in this function, we
 	 * can't check the return and fix up environment variables in the
-	 * child.  Instead, we have to temporarily do any fixup, then possibly
-	 * undo it later.  UGH!
+	 * child.  Instead, we have to create an intermediate function,
+	 * wrap_clone_child, which does this fix up.
 	 */
-	pseudo_debug(1, "client resetting for clone(2) call\n");
-	pseudo_setupenv();
-	if (!pseudo_get_value("PSEUDO_UNLOAD")) {
-		pseudo_reinit_libpseudo();
-	} else {
-		pseudo_dropenv();
-	}
+
+	struct clone_args * myargs = malloc(sizeof(struct clone_args));
+
+	myargs->fn = fn;
+	myargs->flags = flags;
+	myargs->arg = arg;
+
 	/* call the real syscall */
-	rc = (*real_clone)(fn, child_stack, flags, arg, pid, tls, ctid);
+	rc = (*real_clone)(wrap_clone_child, child_stack, flags, myargs, pid, tls, ctid);
+
+	/* If we're not sharing memory, we need to free myargs in the parent */
+	if (!(flags & CLONE_VM))
+		free(myargs);
+
 /*	...
  *	return rc;
  * }

ports/linux/newclone/pseudo_wrappers.c

@@ -10,6 +10,34 @@ ap) {
 	return 0;
 }
 
+struct clone_args {
+	int (*fn)(void *);
+	int flags;
+	void *arg;
+};
+
+int wrap_clone_child(void *args) {
+	struct clone_args *clargs = args;
+
+	int (*fn)(void *) = clargs->fn;
+	int flags = clargs->flags;
+	void *arg = clargs->arg;
+
+	/* We always free in the client */
+	free(clargs);
+
+	if (!(flags & CLONE_VM)) {
+		pseudo_setupenv();
+		if (!pseudo_get_value("PSEUDO_UNLOAD")) {
+			pseudo_reinit_libpseudo();
+		} else {
+			pseudo_dropenv();
+		}
+	}
+
+	return fn(arg);
+}
+
 int
 clone(int (*fn)(void *), void *child_stack, int flags, void *arg, ...) {
 	sigset_t saved;
@@ -42,11 +70,6 @@ clone(int (*fn)(void *), void *child_stack, int flags, void *arg, ...) {
 
 	int save_errno;
 	int save_disabled = pseudo_disabled;
-	/* because clone() doesn't actually continue in this function, we
-	 * can't check the return and fix up environment variables in the
-	 * child.  Instead, we have to temporarily do any fixup, then possibly
-	 * undo it later.  UGH!
-	 */
 
 #include "guts/clone.c"
 

ports/linux/oldclone/guts/clone.c

@@ -1,5 +1,5 @@
 /* 
- * Copyright (c) 2008-2010 Wind River Systems; see
+ * Copyright (c) 2008-2011 Wind River Systems; see
  * guts/COPYRIGHT for information.
  *
  * static int
@@ -8,18 +8,23 @@
  */
 	/* because clone() doesn't actually continue in this function, we
 	 * can't check the return and fix up environment variables in the
-	 * child.  Instead, we have to temporarily do any fixup, then possibly
-	 * undo it later.  UGH!
+	 * child.  Instead, we have to create an intermediate function,
+	 * wrap_clone_child, which does this fix up.
 	 */
-	pseudo_debug(1, "client resetting for clone(2) call\n");
-	pseudo_setupenv();
-	if (!pseudo_get_value("PSEUDO_UNLOAD")) {
-		pseudo_reinit_libpseudo();
-	} else {
-		pseudo_dropenv();
-	}
+
+	struct clone_args * myargs = malloc(sizeof(struct clone_args));
+
+	myargs->fn = fn;
+	myargs->flags = flags;
+	myargs->arg = arg;
+
 	/* call the real syscall */
-	rc = (*real_clone)(fn, child_stack, flags, arg);
+	rc = (*real_clone)(wrap_clone_child, child_stack, flags, myargs, pid);
+
+	/* If we're not sharing memory, we need to free myargs in the parent */
+	if (!(flags & CLONE_VM))
+		free(myargs);
+
 /*	...
  *	return rc;
  * }

ports/linux/oldclone/pseudo_wrappers.c

@@ -1,13 +1,40 @@
-int
+static int
 wrap_clone(int (*fn)(void *), void *child_stack, int flags, void *arg) {
 	/* unused */
 	return 0;
 }
 
+struct clone_args {
+	int (*fn)(void *);
+	int flags;
+	void *arg;
+};
+
+int wrap_clone_child(void *args) {
+	struct clone_args *clargs = args;
+
+	int (*fn)(void *) = clargs->fn;
+	int flags = clargs->flags;
+	void *arg = clargs->arg;
+
+	/* We always free in the client */
+	free(clargs);
+
+	if (!(flags & CLONE_VM)) {
+		pseudo_setupenv();
+		if (!pseudo_get_value("PSEUDO_UNLOAD")) {
+			pseudo_reinit_libpseudo();
+		} else {
+			pseudo_dropenv();
+		}
+	}
+
+	return fn(arg);
+}
+
 int
 clone(int (*fn)(void *), void *child_stack, int flags, void *arg) {
 	sigset_t saved;
-	va_list ap;
 	pid_t *pid;
 	struct user_desc *tls;
 	pid_t *ctid;
@@ -30,11 +57,6 @@ clone(int (*fn)(void *), void *child_stack, int flags, void *arg) {
 
 	int save_errno;
 	int save_disabled = pseudo_disabled;
-	/* because clone() doesn't actually continue in this function, we
-	 * can't check the return and fix up environment variables in the
-	 * child.  Instead, we have to temporarily do any fixup, then possibly
-	 * undo it later.  UGH!
-	 */
 
 #include "guts/clone.c"
 

pseudo.1

@@ -576,13 +576,6 @@ than it is otherwise.  This is probably because nearly every operation
 communication with the server, and probably some kind of database
 activity.
 
-The 
-.IR clone(2)
-wrapper unconditionallity restores the system environment.  It also invokes
-the checks to see if the user has requested pseudo to be disabled or unloaded.
-Due to the semantics of clone, this causes both the parent and child processes
-to be disabled or unloaded.
-
 .SH SEE ALSO
 fakeroot(1), ld.so(8), pseudolog(1), sqlite3(1)
 .SH FURTHER READING