Does this multiple pipes code in C makes sense?

Question

I've created a question about this a few days. My solution is something in the lines of what was suggested in the accepted answer. However, a friend of mine came up with the following solution:

Please note that the code has been updated a few times (check the edit revisions) to reflect the suggestions in the answers below. If you intend to give a new answer, please do so with this new code in mind and not the old one which had lots of problems.

#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>

int main(int argc, char *argv[]){
    int fd[2], i, aux, std0, std1;

    do {
     std0 = dup(0); // backup stdin
     std1 = dup(1); // backup stdout

     // let's pretend I'm reading commands here in a shell prompt
     READ_COMMAND_FROM_PROMPT();

     for(i=1; i<argc; i++) {
      // do we have a previous command?
      if(i > 1) {
       dup2(aux, 0);
       close(aux);
      }

      // do we have a next command?
      if(i < argc-1) {
       pipe(fd);

       aux = fd[0];
       dup2(fd[1], 1);
       close(fd[1]);
      }

      // last command? restore stdout...
      if(i == argc-1) {
       dup2(std1, 1);
       close(std1);
      }

      if(!fork()) {
       // if not last command, close all pipe ends
       // (the child doesn't use them)
       if(i < argc-1) {
        close(std0);
        close(std1);
        close(fd[0]);
       }

       execlp(argv[i], argv[i], NULL);
       exit(0);
      }
     }

     // restore stdin to be able to keep using the shell
     dup2(std0, 0);
     close(std0);
    }

    return 0;
}

This simulates a series of commands through pipes like in bash, for instance: cmd1 | cmd2 | ... | cmd_n. I say "simulate", because, as you can see, the commands are actually read from the arguments. Just to spare time coding a simple shell prompt...

Of course there are some issues to fix and to add like error handling but that's not the point here. I think I kinda get the code but it still makes me a lot of confusing how this whole thing works.

Am I missing something or this really works and it's a nice and clean solution to solve the problem? If not, can anyone point me the crucial problems this code has?

Answer 1

The key problem is that you create a bunch of pipes and don't make sure that all the ends are closed properly. If you create a pipe, you get two file descriptors; if you fork, then you have four file descriptors. If you dup() or dup2() one end of the pipe to a standard descriptor, you need to close both ends of the pipe - at least one of the closes must be after the dup() or dup2() operation.

---

Consider the file descriptors available to the first command (assuming there are at least two - something that should be handled in general (no pipe() or I/O redirection needed with just one command), but I recognize that the error handling is eliminated to keep the code suitable for SO):

    std=dup(1);    // Likely: std = 3
    pipe(fd);      // Likely: fd[0] = 4, fd[1] = 5
    aux = fd[0];
    dup2(fd[1], 1);
    close(fd[1]);  // Closes 5

    if (fork() == 0) {
         // Need to close: fd[0] aka aux = 4
         // Need to close: std = 3
         close(fd[0]);
         close(std);
         execlp(argv[i], argv[i], NULL);
         exit(1);
    }

Note that because fd[0] is not closed in the child, the child will never get EOF on its standard input; this is usually problematic. The non-closure of std is less critical.

---

Revisiting amended code (as of 2009-06-03T20:52-07:00)...

Assume that process starts with file descriptors 0, 1, 2 (standard input, output, error) open only. Also assume we have exactly 3 commands to process. As before, this code writes out the loop with annotations.

std0 = dup(0); // backup stdin - 3
std1 = dup(1); // backup stdout - 4

// Iteration 1 (i == 1)
// We have another command
pipe(fd);   // fd[0] = 5; fd[1] = 6
aux = fd[0]; // aux = 5
dup2(fd[1], 1);
close(fd[1]);       // 6 closed
// Not last command
if (fork() == 0) {
    // Not last command
    close(std1);    // 4 closed
    close(fd[0]);   // 5 closed
    // Minor problemette: 3 still open
    execlp(argv[i], argv[i], NULL);
    }
// Parent has open 3, 4, 5 - no problem

// Iteration 2 (i == 2)
// There was a previous command
dup2(aux, 0);      // stdin now on read end of pipe
close(aux);        // 5 closed
// We have another command
pipe(fd);          // fd[0] = 5; fd[1] = 6
aux = fd[0];
dup2(fd[1], 1);
close(fd[1]);      // 6 closed
// Not last command
if (fork() == 0) {
    // Not last command
    close(std1);   // 4 closed
    close(fd[0]);  // 5 closed
    // As before, 3 is still open - not a major problem
    execlp(argv[i], argv[i], NULL);
    }
// Parent has open 3, 4, 5 - no problem

// Iteration 3 (i == 3)
// We have a previous command
dup2(aux, 0);      // stdin is now read end of pipe 
close(aux);        // 5 closed
// No more commands

// Last command - restore stdout...
dup2(std1, 1);     // stdin is back where it started
close(std1);       // 4 closed

if (fork() == 0) {
    // Last command
    // 3 still open
    execlp(argv[i], argv[i], NULL);
}
// Parent has closed 4 when it should not have done so!!!
// End of loop
// restore stdin to be able to keep using the shell
dup2(std0, 0);
// 3 still open - as desired

So, all the children have the original standard input connected as file descriptor 3. This is not ideal, though it is not dreadfully traumatic; I'm hard pressed to find a circumstance where this would matter.

Closing file descriptor 4 in the parent is a mistake - the next iteration of 'read a command and process it won't work because std1 is not initialized inside the loop.

Generally, this is close to correct - but not quite correct.

Answer 2

It will give results, some that are not expected. It is far from a nice solution: It messes with the parent process' standard descriptors, does not recover the standard input, descriptors leak to children, etc.

If you think recursively, it may be easier to understand. Below is a correct solution, without error checking. Consider a linked-list type command, with it's next pointer and a argv array.

void run_pipeline(command *cmd, int input) {
  int pfds[2] = { -1, -1 };

  if (cmd->next != NULL) {
    pipe(pfds);
  }
  if (fork() == 0) { /* child */
    if (input != -1) {
      dup2(input, STDIN_FILENO);
      close(input);
    }
    if (pfds[1] != -1) {
      dup2(pfds[1], STDOUT_FILENO);
      close(pfds[1]);
    }
    if (pfds[0] != -1) {
      close(pfds[0]);
    }
    execvp(cmd->argv[0], cmd->argv);
    exit(1);
  }
  else { /* parent */
    if (input != -1) {
      close(input);
    }
    if (pfds[1] != -1) {
      close(pfds[1]);
    }
    if (cmd->next != NULL) {
      run_pipeline(cmd->next, pfds[0]);
    }
  }
}

Call it with the first command in the linked-list, and input = -1. It does the rest.

Answer 3

Both in this question and in another (as linked in the first post), ephemient suggested me a solution to the problem without messing with the parents file descriptors as demonstrated by a possible solution in this question.

I didn't get his solution, I tried and tried to understand but I can't seem to get it. I also tried to code it without understanding but it didn't work. Probably because I've failed to understand it correctly and wasn't able to code it the it should have been coded.

Anyway, I tried to come up with my own solution using some of the things I understood from the pseudo code and came up with this:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <wait.h>
#include <string.h>
#include <readline/readline.h>
#include <readline/history.h>

#define NUMPIPES 5
#define NUMARGS 10

int main(int argc, char *argv[]) {
    char *bBuffer, *sPtr, *aPtr = NULL, *pipeComms[NUMPIPES], *cmdArgs[NUMARGS];
    int aPipe[2], bPipe[2], pCount, aCount, i, status;
    pid_t pid;

    using_history();

    while(1) {
     bBuffer = readline("\e[1;31mShell \e[1;32m# \e[0m");

     if(!strcasecmp(bBuffer, "exit")) {
      return 0;
     }

     if(strlen(bBuffer) > 0) {
      add_history(bBuffer);
     }

     sPtr = bBuffer;
     pCount =0;

     do {
      aPtr = strsep(&sPtr, "|");

      if(aPtr != NULL) {
       if(strlen(aPtr) > 0) {
        pipeComms[pCount++] = aPtr;
       }
      }
     } while(aPtr);

     cmdArgs[pCount] = NULL;

     for(i = 0; i < pCount; i++) {
      aCount = 0;

      do {
       aPtr = strsep(&pipeComms[i], " ");

       if(aPtr != NULL) {
        if(strlen(aPtr) > 0) {
         cmdArgs[aCount++] = aPtr;
        }
       }
      } while(aPtr);

      cmdArgs[aCount] = NULL;

      // Do we have a next command?
      if(i < pCount-1) {
       // Is this the first, third, fifth, etc... command?
       if(i%2 == 0) {
        pipe(aPipe);
       }

       // Is this the second, fourth, sixth, etc... command?
       if(i%2 == 1) {
        pipe(bPipe);
       }
      }

      pid = fork();

      if(pid == 0) {
       // Is this the first, third, fifth, etc... command?
       if(i%2 == 0) {
        // Do we have a previous command?
        if(i > 0) {
         close(bPipe[1]);
         dup2(bPipe[0], STDIN_FILENO);
         close(bPipe[0]);
        }

        // Do we have a next command?
        if(i < pCount-1) {
         close(aPipe[0]);
         dup2(aPipe[1], STDOUT_FILENO);
         close(aPipe[1]);
        }
       }

       // Is this the second, fourth, sixth, etc... command?
       if(i%2 == 1) {
        // Do we have a previous command?
        if(i > 0) {
         close(aPipe[1]);
         dup2(aPipe[0], STDIN_FILENO);
         close(aPipe[0]);
        }

        // Do we have a next command?
        if(i < pCount-1) {
         close(bPipe[0]);
         dup2(bPipe[1], STDOUT_FILENO);
         close(bPipe[1]);
        }
       }

       execvp(cmdArgs[0], cmdArgs);
       exit(1);
      } else {
       // Do we have a previous command?
       if(i > 0) {
        // Is this the first, third, fifth, etc... command?
        if(i%2 == 0) {
         close(bPipe[0]);
         close(bPipe[1]);
        }

        // Is this the second, fourth, sixth, etc... command?
        if(i%2 == 1) {
         close(aPipe[0]);
         close(aPipe[1]);
        }
       }

       // wait for the last command? all others will run in the background
       if(i == pCount-1) {
        waitpid(pid, &status, 0);
       }

       // I know they will be left as zombies in the table
       // Not relevant for this...
      }
     }
    }

    return 0;
}

This may not be the best and cleanest solution but it was something I could come up with and, most importantly, something I can understand. What good is to have something working that I don't understand and then I'm evaluated by my teacher and I can't explain to him what the code is doing?

Anyway, what do you think about this one?

Answer 4

This is my "final" code with ephemient suggestions:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <wait.h>
#include <string.h>
#include <readline/readline.h>
#include <readline/history.h>

#define NUMPIPES 5
#define NUMARGS 10

int main(int argc, char *argv[]) {
    char *bBuffer, *sPtr, *aPtr = NULL, *pipeComms[NUMPIPES], *cmdArgs[NUMARGS];
    int newPipe[2], oldPipe[2], pCount, aCount, i, status;
    pid_t pid;

    using_history();

    while(1) {
     bBuffer = readline("\e[1;31mShell \e[1;32m# \e[0m");

     if(!strcasecmp(bBuffer, "exit")) {
      return 0;
     }

     if(strlen(bBuffer) > 0) {
      add_history(bBuffer);
     }

     sPtr = bBuffer;
     pCount = -1;

     do {
      aPtr = strsep(&sPtr, "|");

      if(aPtr != NULL) {
       if(strlen(aPtr) > 0) {
        pipeComms[++pCount] = aPtr;
       }
      }
     } while(aPtr);

     cmdArgs[++pCount] = NULL;

     for(i = 0; i < pCount; i++) {
      aCount = -1;

      do {
       aPtr = strsep(&pipeComms[i], " ");

       if(aPtr != NULL) {
        if(strlen(aPtr) > 0) {
         cmdArgs[++aCount] = aPtr;
        }
       }
      } while(aPtr);

      cmdArgs[++aCount] = NULL;

      // do we have a next command?
      if(i < pCount-1) {
       pipe(newPipe);
      }

      pid = fork();

      if(pid == 0) {
       // do we have a previous command?
       if(i > 0) {
        close(oldPipe[1]);
        dup2(oldPipe[0], 0);
        close(oldPipe[0]);
       }

       // do we have a next command?
       if(i < pCount-1) {
        close(newPipe[0]);
        dup2(newPipe[1], 1);
        close(newPipe[1]);
       }

       // execute command...
       execvp(cmdArgs[0], cmdArgs);
       exit(1);
      } else {
       // do we have a previous command?
       if(i > 0) {
        close(oldPipe[0]);
        close(oldPipe[1]);
       }

       // do we have a next command?
       if(i < pCount-1) {
        oldPipe[0] = newPipe[0];
        oldPipe[1] = newPipe[1];
       }

       // wait for last command process?
       if(i == pCount-1) {
        waitpid(pid, &status, 0);
       }
      }
     }
    }

    return 0;
}

Is it ok now?