[Ace-users] [ace-bugs] ACE_Process::spawn() results in undefined behavior

[J.T. Conklin]

    ACE VERSION: 5.6.1 (subversion version 79840)

    HOST MACHINE and OPERATING SYSTEM:

NetBSD orac.acorntoolworks.com 3.0_STABLE NetBSD 3.0_STABLE (ORAC) #0: Tue Feb 21 20:05:51 PST 2006  jtc at orac.acorntoolworks.com:/home/jtc/netbsd/NetBSD-3/src/sys/arch/i386/compile/ORAC i386

    TARGET MACHINE and OPERATING SYSTEM, if different from HOST:

NetBSD orac.acorntoolworks.com 3.0_STABLE NetBSD 3.0_STABLE (ORAC) #0: Tue Feb 21 20:05:51 PST 2006  jtc at orac.acorntoolworks.com:/home/jtc/netbsd/NetBSD-3/src/sys/arch/i386/compile/ORAC i386

    COMPILER NAME AND VERSION (AND PATCHLEVEL):

gcc version 3.3.3 (NetBSD nb3 20040520)

    THE $ACE_ROOT/ace/config.h FILE [if you use a link to a platform-
    specific file, simply state which one]:

config-netbsd.h

    THE $ACE_ROOT/include/makeinclude/platform_macros.GNU FILE [if you
    use a link to a platform-specific file, simply state which one
    (unless this isn't used in this case, e.g., with Microsoft Visual
    C++)]:

platform_netbsd.GNU

    CONTENTS OF $ACE_ROOT/bin/MakeProjectCreator/config/default.features
    (used by MPC when you generate your own makefiles):

N/A

    AREA/CLASS/EXAMPLE AFFECTED:
    [What example failed?  What module failed to compile?]

ACE_Process

    DOES THE PROBLEM AFFECT:

EXECUTION

    SYNOPSIS:

ACE_Process::spawn() results in undefined behavior 

    DESCRIPTION:

Similar to the PRF I sent this morning, ACE_Process::spawn() invokes
async-signal-unsafe functions between fork() and exec().

This PRF is for the simplest of those cases.  In the child process,
if setpgid(), setregid(), or setreuid() fails, it invokes ACE_ERROR
to note the failure(s).  This will result in an async-signal-unsafe
call to printf() at the very least (I wouldn't be surprised if tens
of unsafe functions are called within a single ACE_ERROR invocation).

This can result in deadlock.

    REPEAT BY:

Discovered by code inspection.

    SAMPLE FIX/WORKAROUND:

The enclosed patch Adds a #if 0 around each ACE_ERROR call.  A case
could be made for using #if !ACE_HAS_THREADS instead.  I'm interested
in opinions over which would be preferable for the master ACE sources.

Index: Process.cpp
===================================================================
--- Process.cpp	(revision 79869)
+++ Process.cpp	(working copy)
@@ -353,9 +353,15 @@
       if (options.getgroup () != ACE_INVALID_PID
           && ACE_OS::setpgid (0,
                               options.getgroup ()) < 0)
-        ACE_ERROR ((LM_ERROR,
-                    ACE_TEXT ("%p.\n"),
-                    ACE_TEXT ("ACE_Process::spawn: setpgid failed.")));
+	{
+#if 0
+	  // We can't emit this log message because ACE_ERROR(), etc.
+	  // will invoke async signal unsafe functions.
+	  ACE_ERROR ((LM_ERROR,
+                      ACE_TEXT ("%p.\n"),
+                      ACE_TEXT ("ACE_Process::spawn: setpgid failed.")));
+#endif
+	}
 # endif /* ACE_LACKS_SETPGID */
 
 # if !defined (ACE_LACKS_SETREGID)
@@ -363,9 +369,15 @@
           || options.getegid () != (uid_t) -1)
         if (ACE_OS::setregid (options.getrgid (),
                               options.getegid ()) == -1)
-          ACE_ERROR ((LM_ERROR,
-                      ACE_TEXT ("%p.\n"),
-                      ACE_TEXT ("ACE_Process::spawn: setregid failed.")));
+	  {
+#if 0
+	    // We can't emit this log message because ACE_ERROR(), etc.
+	    // will invoke async signal unsafe functions.
+	    ACE_ERROR ((LM_ERROR,
+			ACE_TEXT ("%p.\n"),
+			ACE_TEXT ("ACE_Process::spawn: setregid failed.")));
+#endif
+	  }
 # endif /* ACE_LACKS_SETREGID */
 
 # if !defined (ACE_LACKS_SETREUID)
@@ -374,9 +386,15 @@
           || options.geteuid () != (uid_t) -1)
         if (ACE_OS::setreuid (options.getruid (),
                               options.geteuid ()) == -1)
-          ACE_ERROR ((LM_ERROR,
-                      ACE_TEXT ("%p.\n"),
-                      ACE_TEXT ("ACE_Process::spawn: setreuid failed.")));
+	  {
+#if 0
+	    // We can't emit this log message because ACE_ERROR(), etc.
+	    // will invoke async signal unsafe functions.
+	    ACE_ERROR ((LM_ERROR,
+			ACE_TEXT ("%p.\n"),
+			ACE_TEXT ("ACE_Process::spawn: setreuid failed.")));
+#endif
+	  }
 # endif /* ACE_LACKS_SETREUID */
 
       this->child (ACE_OS::getppid ());

-- 
J.T. Conklin