Re: [AD] xmessage broken pipe

[Chris <chris.kcat@xxxxxxxxxx>]

Elias Pschernig wrote:
> Maybe using system() instead of fork/pipes/execlp/other weird stuff :)
> is better after all? I don't think it poses a high security risk and
> would be much simpler, but not sure it is actually the cause of the
> problem.

The problem seems to be the crazy multi-function if()'s, leaving itself 
wide open for improper file closings. To properly close a pipe, you 
*have* to close *all* writing handles before the last one that reads, or 
else you get a SIGPIPE signal. I put together this patch that should fix 
the problem.
Index: src/x/xsystem.c
===================================================================
RCS file: /cvsroot/alleg/allegro/src/x/xsystem.c,v
retrieving revision 1.31
diff -u -r1.31 xsystem.c
--- src/x/xsystem.c	29 Nov 2004 08:09:06 -0000	1.31
+++ src/x/xsystem.c	10 Dec 2004 12:05:41 -0000
@@ -207,7 +207,7 @@
    get_executable_name(tmp, sizeof(tmp));
    set_window_title(get_filename(tmp));
 
-   /* Open the display, create a window, and background-process 
+   /* Open the display, create a window, and background-process
     * events for it all. */
    if (_xwin_open_display(0) || _xwin_create_window()
        || _unix_bg_man->register_func (_xwin_bg_handler)) {
@@ -280,7 +280,6 @@
 static int _xwin_sysdrv_set_close_button_callback(void (*proc)(void))
 {
    _xwin.close_button_callback = proc;
-   
    return 0;
 }
 
@@ -312,32 +311,36 @@
    switch (pid) {
 
       case -1: /* fork error */
-	 close(fd[0]);
 	 close(fd[1]);
+	 close(fd[0]);
 	 fputs(msg2, stdout);
 	 break;
 
       case 0: /* child process */
-	 if ((close(STDIN_FILENO) != -1)
-	     && (dup2(fd[0], STDIN_FILENO) != -1)
-	     && (close(fd[1]) != -1))
-	 {
-	    execlp("xmessage", "xmessage", "-buttons", "OK", "-default", "OK", "-center", "-file", "-", NULL);
+	 if (close(STDIN_FILENO) != -1) {
+	    if (dup2(fd[0], STDIN_FILENO) != -1) {
+	       execlp("xmessage", "xmessage", "-buttons", "OK", "-default", "OK", "-center", "-file", "-", NULL);
+	    }
 	 }
 	 /* if execution reaches here, it means execlp failed */
 	 _exit(EXIT_FAILURE);
 	 break;
 
       default: /* parent process */
-	 if ((close(fd[0]) == -1)
-	     || (write(fd[1], msg2, strlen(msg2)) == -1)
-	     || (close(fd[1]) == -1)
-	     || (waitpid(pid, &status, 0) != pid)
-	     || (!WIFEXITED(status))
+	 /* Even if writing fails, we should wait for the child to close */
+	 write(fd[1], msg2, strlen(msg2));
+	 while ((waitpid(pid, &status, 0) != pid)
+	    ;
+	 if ((!WIFEXITED(status))
 	     || (WEXITSTATUS(status) != 101)) /* ok button */
 	 {
 	    fputs(msg2, stdout);
 	 }
+
+	 /* Close these in proper order to avoid SIGPIPE: writer first, then
+	  * reader */
+	 close(fd[1]);
+	 close(fd[0]);
 	 break;
    }
 }