[PATCH] Fix for tottaly broken background operations support - part 1

[Pavel Tsekov <ptsekov gmx net>]

Hello,

Currently the background operation is totaly screwed. It was broken
by this change (I guess) made on 2003-10-25:

  * background.c (do_background): Use pipe() instead of less
  portable socketpair().  Close comm[0] in the child.

Still, it may have been broken even in its previous incarnation - that I
cannot tell. Even with my patch there are still problems - those will be
addressed in the next set of patches. The current patch fixes the
communication between the parent and the child - the major problem.

In short - a new method of communication (pipe() vs socketpair()) was
employed but the semantics remained as if socket pair was used.

Unlike a socket, pipe() is inherently unidirectional  - it has a read end
and a write end. One cannot write to the read end or read from the write end
as is done in the current code. See the examples below:

In parent_call_string() the child does this:

[...]
    for (i = 0; i < argc; i++){
        int  len;
        void *value;

        len   = va_arg (ap, int);
        value = va_arg (ap, void *);
        write (parent_fd, &len, sizeof (int));
        write (parent_fd, value, len);
    }
    read (parent_fd, &i, sizeof (int));
[...]

In background_attention() the parent does this:

[...]
    for (i = 0; i < argc; i++){
        int size;

        read (fd, &size, sizeof (size));
        data [i] = g_malloc (size+1);
        read (fd, data [i], size);

        data [i][size] = 0;     /* NULL terminate the blocks (they could be strings) */
    }
[...]
        if (resstr){
            len = strlen (resstr);
            write (fd, &len, sizeof (len));
            if (len){
                write (fd, resstr, len);
                g_free (resstr);
            }
        } else {
            len = 0;
            write (fd, &len, sizeof (len));
        }
[...]

Both are wrong.

The suggested patch addresses the problem by creating two pipes one used
to send data from the parent to the child and the other in the
opposite direction. All calls a adjusted accordingly.

Due to this bug and lack of proper error checking in the background
operations code a very interesting behaviour can be observed. To try
and reproduce it follow these steps:

1) Start a background copy operation and arrange things so that
   the operation will fail while in progress . For example
   copy a large number of files so that you will run out of disk space
   or play with permissions.

2) Watch MC crashing while running out of stack space

This one is particulary funny and is the result of the reported bug and
bad coding .

Index: src/background.c
===================================================================
RCS file: /cvsroot/mc/mc/src/background.c,v
retrieving revision 1.41
diff -u -p -r1.41 background.c
--- src/background.c	27 May 2005 03:35:15 -0000	1.41
+++ src/background.c	13 Jul 2005 17:28:19 -0000
@@ -51,8 +51,8 @@ enum ReturnType {
 /* If true, this is a background process */
 int we_are_background = 0;
 
-/* File descriptor for talking to our parent */
-static int parent_fd;
+/* Used by the child to talk to the parent. */
+static int parent_comm_pipe[2];
 
 #define MAXCALLARGS 4		/* Number of arguments supported */
 
@@ -61,19 +61,23 @@ struct TaskList *task_list = NULL;
 static int background_attention (int fd, void *closure);
     
 static void
-register_task_running (FileOpContext *ctx, pid_t pid, int fd, char *info)
+register_task_running (FileOpContext *ctx, pid_t pid, int fd[2], char *info)
 {
-    TaskList *new;
+    TaskList *new_task;
 
-    new = g_new (TaskList, 1);
-    new->pid   = pid;
-    new->info  = info;
-    new->state = Task_Running;
-    new->next  = task_list;
-    new->fd    = fd;
-    task_list  = new;
+    ctx->pid   = pid;
 
-    add_select_channel (fd, background_attention, ctx);
+    new_task = g_new (TaskList, 1);
+    new_task->pid   = pid;
+    new_task->info  = info;
+    new_task->state = Task_Running;
+    new_task->next  = task_list;
+    new_task->fd[0]    = fd[0];
+    new_task->fd[1]    = fd[1];
+    new_task->file_op_context = ctx; 
+    task_list  = new_task;
+
+    add_select_channel (fd[0], background_attention, new_task);
 }
 
 void
@@ -109,16 +113,25 @@ unregister_task_running (pid_t pid, int 
 int
 do_background (struct FileOpContext *ctx, char *info)
 {
-    int comm[2];		/* control connection stream */
     pid_t pid;
+    int parent_to_child_pipe[2];
+    int child_to_parent_pipe[2];
+
+    if (pipe (parent_to_child_pipe) == -1)
+	return -1;
 
-    if (pipe (comm) == -1)
+    if (pipe (child_to_parent_pipe) == -1) {
+	close (parent_to_child_pipe[0]);
+	close (parent_to_child_pipe[1]);
 	return -1;
+    }
 
     if ((pid = fork ()) == -1) {
     	int saved_errno = errno;
-    	(void) close (comm[0]);
-    	(void) close (comm[1]);
+	close (child_to_parent_pipe[0]);
+	close (child_to_parent_pipe[1]);
+	close (parent_to_child_pipe[0]);
+	close (parent_to_child_pipe[1]);
     	errno = saved_errno;
 	return -1;
     }
@@ -126,27 +139,34 @@ do_background (struct FileOpContext *ctx
     if (pid == 0) {
 	int nullfd;
 
-	close (comm[0]);
-	parent_fd = comm[1];
 	we_are_background = 1;
 	current_dlg = NULL;
 
-	/* Make stdin/stdout/stderr point somewhere */
-	close (0);
-	close (1);
-	close (2);
-
 	if ((nullfd = open ("/dev/null", O_RDWR)) != -1) {
-	    while (dup2 (nullfd, 0) == -1 && errno == EINTR);
-	    while (dup2 (nullfd, 1) == -1 && errno == EINTR);
-	    while (dup2 (nullfd, 2) == -1 && errno == EINTR);
+	    while (dup2 (nullfd, STDIN_FILENO) == -1 && errno == EINTR);
+	    while (dup2 (nullfd, STDOUT_FILENO) == -1 && errno == EINTR);
+	    while (dup2 (nullfd, STDERR_FILENO) == -1 && errno == EINTR);
 	}
+	/* TODO: else ... */
+
+	close (child_to_parent_pipe[0]);
+	close (parent_to_child_pipe[1]);
+
+	parent_comm_pipe[0] = parent_to_child_pipe[0];
+	parent_comm_pipe[1] = child_to_parent_pipe[1];
 
 	return 0;
     } else {
-	close (comm[1]);
-	ctx->pid = pid;
-	register_task_running (ctx, pid, comm[0], info);
+	int comm[2];
+
+	close (child_to_parent_pipe[1]);
+	close (parent_to_child_pipe[0]);
+
+	comm[0] = child_to_parent_pipe[0];
+	comm[1] = parent_to_child_pipe[1];
+
+	register_task_running (ctx, pid, comm, info);
+
 	return 1;
     }
 }
@@ -194,6 +214,7 @@ do_background (struct FileOpContext *ctx
 static int
 background_attention (int fd, void *closure)
 {
+    TaskList *task_info;
     FileOpContext *ctx;
     int have_ctx;
     void *routine;
@@ -202,7 +223,8 @@ background_attention (int fd, void *clos
     ssize_t bytes;
     enum ReturnType type;
 
-    ctx = closure;
+    task_info = closure;
+    ctx = task_info->file_op_context;
 
     bytes = read (fd, &routine, sizeof (routine));
     if (bytes == -1 || (size_t) bytes < (sizeof (routine))) {
@@ -246,6 +268,8 @@ background_attention (int fd, void *clos
 	data [i][size] = 0;	/* NULL terminate the blocks (they could be strings) */
     }
 
+    channels_down ();
+
     /* Handle the call */
     if (type == Return_Integer){
 	if (!have_ctx)
@@ -287,9 +311,9 @@ background_attention (int fd, void *clos
 	    }
 
 	/* Send the result code and the value for shared variables */
-	write (fd, &result, sizeof (int));
+	write (task_info->fd[1], &result, sizeof (int));
 	if (have_ctx)
-	    write (fd, ctx, sizeof (FileOpContext));
+	    write (task_info->fd[1], ctx, sizeof (FileOpContext));
     } else if (type == Return_String) {
 	int len;
 	char *resstr = NULL;
@@ -317,14 +341,14 @@ background_attention (int fd, void *clos
 	}
 	if (resstr){
 	    len = strlen (resstr);
-	    write (fd, &len, sizeof (len));
+	    write (task_info->fd[1], &len, sizeof (len));
 	    if (len){
-		write (fd, resstr, len);
+		write (task_info->fd[1], resstr, len);
 		g_free (resstr);
 	    }
 	} else {
 	    len = 0;
-	    write (fd, &len, sizeof (len));
+	    write (task_info->fd[1], &len, sizeof (len));
 	}
     }
     for (i = 0; i < argc; i++)
@@ -333,6 +357,9 @@ background_attention (int fd, void *clos
     do_refresh ();
     mc_refresh ();
     doupdate ();
+
+    channels_up ();
+
     return 0;
 }
 
@@ -352,13 +379,13 @@ parent_call_header (void *routine, int a
 
     have_ctx = (ctx != NULL);
 
-    write (parent_fd, &routine, sizeof (routine));
-    write (parent_fd, &argc, sizeof (int));
-    write (parent_fd, &type, sizeof (type));
-    write (parent_fd, &have_ctx, sizeof (have_ctx));
+    write (parent_comm_pipe[1], &routine, sizeof (routine));
+    write (parent_comm_pipe[1], &argc, sizeof (int));
+    write (parent_comm_pipe[1], &type, sizeof (type));
+    write (parent_comm_pipe[1], &have_ctx, sizeof (have_ctx));
 
     if (have_ctx)
-	write (parent_fd, ctx, sizeof (FileOpContext));
+	write (parent_comm_pipe[1], ctx, sizeof (FileOpContext));
 }
 
 int
@@ -375,12 +402,12 @@ parent_call (void *routine, struct FileO
 
 	len   = va_arg (ap, int);
 	value = va_arg (ap, void *);
-	write (parent_fd, &len, sizeof (int));
-	write (parent_fd, value, len);
+	write (parent_comm_pipe[1], &len, sizeof (int));
+	write (parent_comm_pipe[1], value, len);
     }
-    read (parent_fd, &i, sizeof (int));
+    read (parent_comm_pipe[0], &i, sizeof (int));
     if (ctx)
-	read (parent_fd, ctx, sizeof (FileOpContext));
+	read (parent_comm_pipe[0], ctx, sizeof (FileOpContext));
 
     return i;
 }
@@ -400,14 +427,14 @@ parent_call_string (void *routine, int a
 
 	len   = va_arg (ap, int);
 	value = va_arg (ap, void *);
-	write (parent_fd, &len, sizeof (int));
-	write (parent_fd, value, len);
+	write (parent_comm_pipe[1], &len, sizeof (int));
+	write (parent_comm_pipe[1], value, len);
     }
-    read (parent_fd, &i, sizeof (int));
+    read (parent_comm_pipe[0], &i, sizeof (int));
     if (!i)
 	return NULL;
     str = g_malloc (i + 1);
-    read (parent_fd, str, i);
+    read (parent_comm_pipe[0], str, i);
     str [i] = 0;
     return str;
 }
Index: src/boxes.c
===================================================================
RCS file: /cvsroot/mc/mc/src/boxes.c,v
retrieving revision 1.91
diff -u -p -r1.91 boxes.c
--- src/boxes.c	14 Jun 2005 13:02:31 -0000	1.91
+++ src/boxes.c	13 Jul 2005 17:28:20 -0000
@@ -926,7 +926,7 @@ task_cb (int action)
     }
     
     if (sig == SIGINT)
-	unregister_task_running (tl->pid, tl->fd);
+	unregister_task_running (tl->pid, tl->fd[0]);
 
     kill (tl->pid, sig);
     listbox_remove_list (bg_list);