C Unix Pipes Example

Question

Trying to implement a shell, mainly piping. I've written this test case which I expect to simply pipe ls to wc...it definitely doesn't work as expected. It prints ls to the terminal then prints memory exhausted. I'm very lost in how to fix this and get it to work. find_path works in all of my tests.

Edit - I have to use execv for the project, its a class thing, but I've tried it with execvp just in case and it does the exact same thing. Also this is just an example, a test to see why it does not work, I call fork twice once for both commands and waitpid because I have nothing else to do.

#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>
int find_path(char* execname, char** dst)
{        
        char *path = getenv("PATH");
        path = strdup(path);
        char *pos;
        path = strtok_r(path, ":", &pos);
        char *originalpath  = path;
        do
        {
                char* test = (char*)calloc(strlen(path) + strlen(execname) + 2, sizeof(char));
                test = strcpy(test, path);
                int testlen = strlen(test);
                (*(test+testlen)) = '/';
                strcpy(test + testlen + 1,execname);
                struct stat buf;
                int result = stat(test, &buf);
                if (result == 0)
                {
                        *dst = test;
                        free (originalpath);
                        return 1;
                }
                else
                {
                        free(test);
                }

        } while ((path = strtok_r(NULL, ":", &pos)) != NULL);
        free(originalpath);
        return 0;
}

int main()
{
    char *cmd1 = "ls";
    char *cmd2 = "wc";
    int filedes[2];
    pipe(filedes);
    char** argv = (char**)calloc(1, sizeof(char*)); 
    argv[0] = (char*)malloc(sizeof(char*));
    argv[0] = NULL;

    pid_t pid = fork();
    if (pid == 0)
    {
        char *path;
                find_path(cmd1, &path);
        dup2(filedes[1],stdout);

        execv(path,argv); 
    }
    pid = fork();
    if (pid == 0)
    {
        dup2(filedes[0], stdin);
        char *path;
        find_path(cmd2, &path);
        execv(path, argv);

    }
    else
        waitpid(pid);

}

Solution 1

Often when it is hard to debug a program, it is best to simplify it a little to eliminate sources of error. Here is your program, simplified to remove find_path as a source of errors:

#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <stdio.h>

int main(void)
{
    int filedes[2];
    pipe(filedes);

    /* Run LS. */
    pid_t pid = fork();
    if (pid == 0) {
        /* Set stdout to the input side of the pipe, and run 'ls'. */
        dup2(filedes[1], 1);
        char *argv[] = {"ls", NULL};
        execv("/bin/ls", argv);
    } else {
        /* Close the input side of the pipe, to prevent it staying open. */
        close(filedes[1]);
    }

    /* Run WC. */
    pid = fork();
    if (pid == 0) {
        dup2(filedes[0], 0);
        char *argv[] = {"wc", NULL};
        execv("/usr/bin/wc", argv);
    }

    /* Wait for WC to finish. */
    waitpid(pid);
}

This should behave as you expect.

During simplification, a few errors came out:

• argv[] wasn't being setup correctly, in particular, argv[0] was being set to NULL;
• The program was not closing the input side of the pipe that was being given to ls. When ls finished, the pipe wasn't being closed (because the wc process still had it open), preventing wc from ever finishing.
• The program was confusing the values stdout and stdin (which are of type FILE*) with the file descriptor numbers 0 and 1 (used by dup, pipe, etc.)

Solution 2

There is a lot you can do to improve this code (e.g. breaking this into smaller functions would be a start), but I suspect your out of memory issue is from the code in find_path(), which you could avoid entirely by using execvp which will locate the executable using the standard PATH mechanism for you. It is probably a good idea to install a signal handler using sigaction to handle SIGCHLD and invoke waitpid from the signal handler, instead of just invoking waitpid() ad-hoc like you are doing. You appear to be forking more times than you want, and you aren't checking for errors. Hope these suggestions help.

Comments

[Ben]

I edited my post to explain why I use find_path, but execvp does the same thing, and waiting may not be the best way but I'm simply creating an example of the problem here so that's not really related to the error.

[Ben]

Awesome, you got all of my stupid mistakes. In my testing I tried all of what you suggested, except that I completely forgot the first command line argument passed in should be the executable name! Thanks so much.