Bad File Descriptor In Recursive Piping Function using Execvp, fork, and pipe

Question

I'm attempting to create a recursive piping function using dup2(), fork(), and pipe(). However, when my array is something like {"ls", "grep shell"} (where shell is the name of my shell), it goes in an endless loop of displaying the results of ls and saying "write error: bad file descriptor". I'm sure that somehow I'm not properly terminating my recursion, and I suspect the issue is with trying to dup either fd[1] or fd[0], but after debugging this for hours I still can't figure it out. Any help is appreciated!

void recursive_piping(char *recursive_pipe_args[MAX_ARGS]) {
    int i = 0;
    int fd[2];

    char *first_arg[2] = {"", NULL};
    char *rest_of_args[81];

    // if its of size 1, base case
    if (recursive_pipe_args[1] == NULL) {
        if (execvp(recursive_pipe_args[0], recursive_pipe_args) == -1) {
            printf("\nExecute didn't work");
            fflush(stdout); 
        }
        return;
    }

    // recursive case, split args into the first on and the rest of them
    first_arg[0] = recursive_pipe_args[0];
    for (i = 0; i < (num_pipes); i++) {
        rest_of_args[i] = malloc(81);
        strcpy(rest_of_args[i], recursive_pipe_args[i+1]);
    }
    if (pipe(fd) < 0) {
        printf("\npipe Failure");
    }

    // parent section, reads file descriptor fd[0]
    if (fork()) {
        close(fd[0]);
        dup2(fd[1], 0);
        recursive_piping(rest_of_args);
        // return;
    }
    close(fd[1]);
    dup2(fd[0], 1);
    execvp(first_arg[0], first_arg);
}

`if (fork()) ...` Ouch. Have you thought about what happens if `fork()` fails? — Andrew Henle, Aug 23 '18 at 13:43
the number of pipes in a user input. As in, literally, if the user input , num_pipes would be 1. Following that, there should be exactly num_pipes + 1 arguments — rDev, Aug 23 '18 at 13:51
You are `dup2`ing the read ends of your pipes onto file descriptors that are expected to be used for output, and *vise versa*. This is likely the underlying cause of your "bad file descriptor" errors. — John Bollinger, Aug 23 '18 at 13:52
It is probably wasted effort to duplicate the contents of the argument strings. I don't see any reason to think that copying just the pointers would present an issue. — John Bollinger, Aug 23 '18 at 13:54
Furthermore, `"grep shell"` as one of the elements of the input array does not make sense, as it is highly unlikely that your system has a command with that name. When you use the exec-family functions, the responsibility is on you to present the command name and each command argument to as a separate function argument. You don't have a shell between you and the system to do any parsing. — John Bollinger, Aug 23 '18 at 14:00
Ah, I got "grep shell" from the fact that if I do ls | grep shell, I get back both the c file and the shell with the name shell. — rDev, Aug 23 '18 at 14:02
Yes, @Roug, when you execute that command *via the shell*, the shell parses "grep shell" into the command name "grep" and one argument, "shell". That's implemented in the shell, not in the underlying exec call. — John Bollinger, Aug 23 '18 at 14:07
Anyway, if indeed the recursion does not terminate then it is probably because `num_pipes` is too small, so that the trailing `NULL` in the input arguments is not copied over to the `rest_of_args` array. I don't really see why you're using that termination condition anyway -- instead, continue copying until you've copied the `NULL`. If you cannot trust the caller to provide a short enough input array, then bail out with an error in the event that the array is too long, or else allocate dynamically. — John Bollinger, Aug 23 '18 at 14:11
are you sure that you want to dup fd[1] (write) to stdin? i guess you need to read from stdin and write to fd[1] or whatever, but not dup. or dup fd[0] to stdin. — Serge, Aug 23 '18 at 14:23

score 0 · Answer 1 · answered Aug 23 '18 at 14:54

I prepared a variation of your function with these changes:

it adds a check for the first element of recursive_pipe_args being NULL, in which case it emits a diagnostic without executing anything.
it counts the number of initial non-NULL elements in the recursive_pipe_args array, and dynamically allocates rest_of_args with that capacity (thus allowing space for one fewer argument plus the NULL sentinel).
it copies the tail of recursive_pipe_args into rest_of_args based on the actual count of elements of the former, thus being certain to copy the NULL sentinel.
it recognizes and handles errors in fork().
it corrects the duping so that the write end of each pipe (fd[1]) is duped onto a standard output (file descriptor 1), the read end is duped onto a standard input (fd[0] and 0, respectively), and in each process the pipe ends that go unused in that process are closed.
it writes error messages to the standard error stream instead of the standard output, relying on perror() for that as much as possible.

The resulting program works as expected for me: it emits the result of the specified pipeline of commands to its standard output. Note well, however, that your design is inherently limited to commands without any arguments, which heavily constrains its utility. My test case was { "ls", "wc" }.

Code implementing those changes is left as an exercise.

Bad File Descriptor In Recursive Piping Function using Execvp, fork, and pipe

1 Answers1