C Programming pipe only half working

Question

I'm working on a mini shell for a college assignment. We have to read in the command, find the binary to execute from the path var, and execute command, both with and without pipes. I have everything working (I think) except for the pipe. Through web searches I've been able to build a test program that use two hard coded commands and pipes one to the other, with the expected results. Now when I copy and paste that code into my actual program, the first command outputs fine (actually outputs the command as if there were no pipe), while the second I don't think actually does anything (the output from the first is not piped through to the second).

Here is the entire code:

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

#define BUFFSIZE 1024
#define MAXWORDS 17
#define MAXCHAR  64

static char *path;
extern char **environ;

//split cmd "string" on pipe (|) symbol
void split(char **pipe, char **left, char **right, int n)
{
    int i, x;

    for(i = 0; i < n; i++)
    {
        if (strchr(&pipe[i][0], '|') != 0)
        {
            for(x = 0; x < i; x++)
                strcpy(left[x], pipe[x]);
            left[x++] = 0;

            break;
        }
    }

    i++;
    for(x = 0; i < n; x++)
        strcpy(right[x], pipe[i++]);
    right[x++] = 0;
}

//Find directory where cmd can be executed from (PATH or direct access)
char *finddir(char *s)
{
    char *pp;
    char *pf;
    int   ok;
    strcpy(path, getenv("PATH"));
    pp = strtok(path, ":");
    while (pp != NULL)
    {
        pf = (char *)malloc(strlen(pp) + strlen(s) + 2);
        if (pf == NULL)
        {
            fprintf(stderr, "Out of memory in finddir\n");
            return NULL;
        }

        strcpy(pf,pp);
        strcat(pf,"/");
        strcat(pf,s);
        ok = !access(pf, X_OK);
        free(pf);
        if (ok)
            return pp;

        pp = strtok(NULL, ":");
    }
    return NULL;
}

int cmdcheck(char *cmd, char *p)
{
    char *dir;

    if (strchr(p, '/') != NULL)
        sprintf(cmd, "%s\0", p);
    else
    {
        dir = finddir(p);
        if (dir == NULL)
            return 1;
        else 
            sprintf(cmd, "%s/%s\0", dir, p);
    }

    return 0;
} 

void runpipe(int pfd[], char *cmd1, char *p1[], char *cmd2, char *p2[])
{
    int pid;
    int status;

    switch (pid = fork())
    {
        case 0:     //Child
            dup(pfd[0]);
            close(pfd[1]);          //the child does not need this end of the pipe 
            execve(cmd2, p2, environ);
            perror(cmd2);

        default:    //Parent
            dup(pfd[1]);
            close(pfd[0]);          //the parent does not need this end of the pipe 
            execve(cmd1, p1, environ);
            perror(cmd1);

        case -1:    //ERROR
            perror("fork-RP");
            exit(1);
    }
}   

int main(void)
    {
        int status;         //read status when reading cmd in
        char ch;            //character currently reading
        int n, i, x;            //(n) count of chars read; (i) cmd args iter; (x) cmd arg iter in cmd array
        char buffer[BUFFSIZE];      //read buffer
        char *token;            //token var when splitting buffer
        int pid0, pid1, pid2;       //return ID from fork call
        int which;          //return value from wait (child pID that just ended)
        char msg[100];          //messages to print out
        char *cmd1, *cmd2;      //cmds when piping
        char *params[MAXWORDS];     //cmd parameters to send to execve
        int fd[2];          //pipe file descriptors
        char *pparam1[MAXWORDS];    //cmd "string" on left side of pipe
        char *pparam2[MAXWORDS];    //cmd on right side of pipe

        for(;;)
        {
            for (i = 0; i < MAXWORDS; i++)
                params[i] = malloc(MAXCHAR);

            n = 0;
            write(1, "# ", 2);

            for(;;)
            {
                status = read(0, &ch, 1);
                if (status == 0)
                    return 0;   //End of file
                if (status == -1)
                    return 1;   //Error

                if(n == BUFFSIZE)
                {
                    write(1, "Line too long\n", 14);
                    return 1;
                }

                buffer[n++] = ch;

                if(ch == '\n')
                    break;
            }

            buffer[n] = '\0';

            x = 0;
            token = strtok(buffer, " \t\n\0");
            while(token != NULL)
            {
                strcpy(params[x++], token);
                token = strtok(NULL, " \t\n\0");
            }
            params[x] = 0;

            path = getenv("PATH");
            if (path == NULL)
            {
                fprintf(stderr, "PATH environment variable not found.\n");
                return 1;
            }

            n = strlen(path);
            path = (char *)malloc(n+1);
            if (path == NULL)
            {
                fprintf(stderr, "Unable to allocate space for copy of PATH.\n");
                return 1;
            }

            cmd1    = malloc(MAXCHAR);
            cmd2    = malloc(MAXCHAR);
            for (i = 0; i < MAXWORDS; i++)
                pparam1[i] = malloc(MAXCHAR);
            for (i = 0; i < MAXWORDS; i++)
                pparam2[i] = malloc(MAXCHAR);

            split(params, pparam1, pparam2, x);

            //Check first cmd
            if(cmdcheck(cmd1, pparam1[0]))
            {
                sprintf(msg, "cmd '%s' is not executable\n", pparam1[0]);
                write(1, msg, strlen(msg));
                break;
            }

            //Check second cmd
            if(cmdcheck(cmd2, pparam2[0]))
            {
                sprintf(msg, "cmd '%s' is not executable\n", pparam2[0]);
                write(1, msg, strlen(msg));
                break;
            }

            pipe(fd);

            switch (pid0 = fork())
            {
                case 0:     //Child
                    switch (pid1 = fork())
                    {
                        case 0:     //Child
                            runpipe(fd, cmd1, pparam1, cmd2, pparam2);
                            exit(0);

                        default:
                            exit(0);
                            //break;

                        case -1:    //ERROR
                            perror("fork-2");
                            exit(1);
                    }

                default:    //Parent
                    which = wait(&status);
                    if (which == -1)
                    {
                        write(1, "wait failed\n", 12);
                        exit(1);
                    }

                    if (status & 0xff)
                        sprintf(msg, "process %d terminated abnormally for reason %d\n", which, status & 0xff);
                    else
                        sprintf(msg, "process %d terminated normally with status %d\n", which, (status >> 8) & 0xff);

                    write(1, msg, strlen(msg));
                    break;

                case -1:    //ERROR
                    perror("fork-1");
                    exit(1);
            }

            free(cmd1);
            free(cmd2);
            for (i = 0; i < MAXWORDS; i++)
                free(pparam1[i]);
            for (i = 0; i < MAXWORDS; i++)
                free(pparam2[i]);

            free(path);
            for (i = 0; i < MAXWORDS; i++)
                free(params[i]);
        }

        return 0;   
    }

Typing echo one | wc -l at the prompt will only output one with the respective wait print statement following. It has been a few years since I've used C, so am I on the right track?

Thanks.

EDIT: Here is the runpipe function as it stands now. But the only thing that is printed is the wait statement.

void runpipe(int pfd[], char *cmd1, char *p1[], char *cmd2, char *p2[])
{
    const int READ = 0;
    const int WRITE = 1;
    int pid;
    int status;

    switch (pid = fork())
    {
        case 0:     //Child
            close(pfd[WRITE]);
            dup2(pfd[READ], STDIN_FILENO);
            close(pfd[READ]);
            execve(cmd2, p2, environ);
            perror(cmd2);

        default:    //Parent
            close(pfd[READ]);
            dup2(pfd[WRITE], STDOUT_FILENO);
            close(pfd[WRITE]);
            execve(cmd1, p1, environ);
            perror(cmd1);

        case -1:    //ERROR
            perror("fork-RP");
            exit(1);
    }
}

Answer 1

There are a couple of things going on there that are contributing to the unexpected behavior.

The first is that you're forking too much. If you unroll your runpipe() function call into the switch statement in main(), you'll see that you reach the great-grandchild level:

switch (pid0 = fork())
{
    case 0:     // Child
        switch (pid1 = fork())
        {
            case 0:     // GRAND-Child
                   // function call to runpipe()
                   switch (pid = fork())
                    {
                        case 0:     // GREAT-GRAND-Child
                            close(pfd[WRITE]);
                            dup2(pfd[READ], STDIN_FILENO);
                            close(pfd[READ]);
                            execve(cmd2, p2, environ);
                            perror(cmd2);

                        default:    // GRAND-Child
                            close(pfd[READ]);
                            dup2(pfd[WRITE], STDOUT_FILENO);
                            close(pfd[WRITE]);
                            execve(cmd1, p1, environ);
                            perror(cmd1);

Which is not necessary. Fork once in main() and then call your runpipe() function.

Related to this issue is where you're creating your pipe. When you fork, the newly created child process inherits all of the parent process's open files (among many other things). This includes the default descriptors 0, 1, and 2 (stdin, stdout, and stderr), as well as any other open files, including the pipe you created called fd. This means that the parent, child, grandchild, and great-grandchild are all inheriting a copy of both ends of the pipe. You correctly close the unused ends inside the runpipe() function (the grandchild's and great-grandchild's copies), but the parent and child in your main() function also have copies!

Since the only pair of processes using the pipe are those created in runpipe(), you can move the declaration of fd and the call to pipe(2) into that function.

These two modifications will resolve your issues.

A completely unrelated issue that just relates to the flow of your shell is that your main() ends up doing its wait(2) on the "parent" process of the runpipe() function. Since that parent is the one running cmd1, your shell is going to return its prompt as soon as cmd1 finishes, instead of when the last command (cmd2 in this case) in the pipeline finishes. You can see the behavioral difference by running something like echo | sleep 10 into your shell and a real shell.

Answer 2

The dup function duplicates a file descriptor, and returns the new duplicate. However, this will not work, as stdin in the child still exists, and the new file descriptor will not be put in place of the standard input.

You must close the standard input file descriptor first, before doing dup. Or use dup2 which will close the destination file descriptor automatically first before doing the duplication:

dup2(pfd[0], STDIN_FILENO);