Trying to implement piping in C - shell is hanging and not running commands

Question

I'm trying to run this command ps -j | more. I think i've set up the pipes correctly, but for some reason it just hangs:

I'm calling a fork that runs ps -j and a second fork that runs more and connecting them with pipes.

For some reason this is still not working as intended.

Code below:

#define BUF_SIZE 100

int main() {

    pid_t pid1, pid2;
    int save_stdin, save_stdout;
    int fd[2];

    char *argv1[] = { "/bin/ps", "-j", NULL };
    char *argv2[] = { "/bin/more", NULL };

    char prompt[] = "Press enter: ";
    char buffer[BUF_SIZE];

    write(STDOUT_FILENO, prompt, sizeof(prompt) - 1);

    ssize_t readIn = readIn = read(STDIN_FILENO, buffer, BUF_SIZE);
    buffer[readIn - 1] = '\0';

    printf("readIn: %d\n", readIn);

    pipe(fd);

    pid1 = fork();
    if (pid1 == 0) { // child
        close(fd[0]);
        save_stdout = dup(1);
        dup2(fd[1], STDOUT_FILENO);
        execvp(argv1[0], argv1);
        close(fd[1]);
    } else { // parent
        pid2 = fork();
        if(pid2 == 0) {
            save_stdin = dup(0);
            dup2(fd[0], STDIN_FILENO);
            execvp(argv2[0], argv2);
            close(fd[0]);
        } else {

        }
    }

    dup2(save_stdin, 0);
    dup2(save_stdout, 1);
    close(save_stdin);
    close(save_stdout);

    int i = 1;
    do {
        wait(NULL);
    } while (i-- > 0);


    exit(0);

}

Any help greatly appreciated!

EDIT:

I've tried closing the pipes after dup2() but before execvp(), but it's still hanging:

pipe(fd);

pid1 = fork();
if (pid1 == 0) { // child
    dup2(fd[1], STDOUT_FILENO);
    close(fd[1]);
    close(fd[0]);
    int res1 = execvp(argv1[0], argv1);
    printf("exec1: %d\n", res1);
} else { // parent
    pid2 = fork();
    if(pid2 == 0) {
        dup2(fd[0], STDIN_FILENO);
        close(fd[0]);
        close(fd[1]);
        int res2 = execvp(argv2[0], argv2);
        printf("exec2: %d\n", res2);
    } else {

    }
}

close(fd[1]);
close(fd[0]);
printf("finished\n");

int i = 1;
do {
    wait(NULL);
} while (i-- > 0);

Answer 1

You aren't closing enough file descriptors of the pipe in the children, or any of them in the parent.

Rule of thumb: If you dup2() one end of a pipe to standard input or standard output, close both of the original file descriptors returned by pipe() as soon as possible. In particular, you should close them before using any of the exec*() family of functions.

The rule also applies if you duplicate the descriptors with either dup() or fcntl() with F_DUPFD

Here's working code that closes pipes properly. I removed the path component of the commands since (a) they were wrong for my machine, and (b) there's no point in using execvp() if you specify the path to the command. I report most errors (but I don't check the results of the read() and write() calls. I also report on the exit statuses of the children. The children report and exit if they fail to execute the command. Your code with 'save_stdin` etc was further confusing things because the variables were uninitialized in the parent which is where the sequence:

dup2(save_stdin, 0);
dup2(save_stdout, 1);
close(save_stdin);
close(save_stdout);

was executed. Neither you nor I know what those two dup2() calls do — but it was probable that they failed, or that they did something weird with your standard I/O channels.

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

#define BUF_SIZE 100

int main(void)
{
    pid_t pid1, pid2;
    int fd[2];

    char *argv1[] = { "ps", "-j", NULL };   // Remove PATH because using execvp()
    char *argv2[] = { "more", NULL };       // Remove PATH because using execvp()

    char prompt[] = "Press enter: ";
    char buffer[BUF_SIZE];

    write(STDOUT_FILENO, prompt, sizeof(prompt) - 1);

    ssize_t readIn = readIn = read(STDIN_FILENO, buffer, BUF_SIZE);
    buffer[readIn - 1] = '\0';

    printf("readIn: %zd\n", readIn);

    if (pipe(fd) < 0)
    {
        fprintf(stderr, "failed to create pipe\n");
        exit(EXIT_FAILURE);
    }

    if ((pid1 = fork()) < 0)
    {
        fprintf(stderr, "failed to fork - 1\n");
        exit(EXIT_FAILURE);
    }

    if (pid1 == 0)   // child
    {
        dup2(fd[1], STDOUT_FILENO);
        close(fd[0]);
        close(fd[1]);
        execvp(argv1[0], argv1);
        fprintf(stderr, "failed to execute %s\n", argv1[0]);
        exit(EXIT_FAILURE);
    }

    if ((pid2 = fork()) < 0)
    {
        fprintf(stderr, "failed to fork - 2\n");
        exit(EXIT_FAILURE);
    }

    if (pid2 == 0)
    {
        dup2(fd[0], STDIN_FILENO);
        close(fd[0]);
        close(fd[1]);
        execvp(argv2[0], argv2);
        fprintf(stderr, "failed to execute %s\n", argv2[0]);
        exit(EXIT_FAILURE);
    }

    close(fd[0]);
    close(fd[1]);

    int corpse;
    int status;
    while ((corpse = wait(&status)) > 0)
        fprintf(stderr, "%d: child %d exited with status 0x%.4X\n",
                (int)getpid(), corpse, (unsigned)status);

    return 0;
}

The program was pipe41 (with some minor editing on the output to remove that which really isn't necessary for you to know about what I was doing in other windows):

$ ./pipe41
Press enter: 
readIn: 1
USER              PID  PPID  PGID   SESS JOBC STAT   TT       TIME COMMAND
jonathanleffler  6618  6617  6618      0    1 S    s000    0:00.11 -bash
jonathanleffler  6629  6628  6629      0    1 S+   s001    0:00.04 -bash
jonathanleffler  6660  6645  6660      0    1 S+   s002    0:00.04 -bash
jonathanleffler  6716  6695  6716      0    1 S+   s003    0:00.04 -bash
jonathanleffler  6776  6746  6776      0    1 S+   s004    0:00.04 -bash
jonathanleffler  6800  6771  6800      0    1 S+   s005    0:00.04 -bash
jonathanleffler  7487  7486  7487      0    1 S    s006    0:00.04 -bash
jonathanleffler  9558  9557  9558      0    1 S    s007    0:00.06 -bash
jonathanleffler 10375  9558 10375      0    1 S+   s007    0:00.01 ./pipe41
jonathanleffler 10377 10375 10375      0    1 S+   s007    0:00.00 more
(END)10375: child 10376 exited with status 0x0000
10375: child 10377 exited with status 0x0000
$