Minibash in C, problem making pipes between execvp and parent proccess

Question

I have to do this as a university project so I cant share the whole code, im sorry for that.

I have to create a function called "read" that enables the user to create new env variables, thats the easy part. The problem comes when I call that function as the last one of the commands array e.g "ls | grep aux.txt | read a" this should give the env var A the value aux.txt, the problem is that it get stuck in the

fgets(value, sizeof(value),stdin);

and I cant even recover the terminal. Thanks in advance for the help if you need more info about the problem I will happily give it.

I can't reproduce exactly the main function as there are parts that are not mine but I hope this helps:

char **argvv;
int fd[2][2];
int pid;
int main(int argc, char ***argvv) {
argvv[0][0] = "echo";
argvv[0][1] = "elpmaxe";
argvv[1][0] = "rev";
argvv[2][0] = "read";
argvv[2][1] = "a";
for (int i = 0; i < 2; i++) {
    pipe(fd[i]);
}
for(int i = 0; i< 3; i++){
    pid = fork();
    if(pid == 0){
        if(i ==0){
            dup2(fd[0][1], 1);
            fun_close(fd);
            execvp(argvv[0][0], argvv[0]);
        }
        if(i == 1){
            dup2(fd[0][0], 0);
            dup2(fd[1][1], 1);
            fun_close(fd);
            execvp(argvv[1][0], argvv[0]);
        }
    }else{
        if(i == 2){
            close(fd[0][1]);
            close(fd[0][0]);
            fun_read("read a", 3, fd[1]);
        }
    }
}
int corpse;
int status;
while ((corpse = wait(&status)) > 0)
    printf("Child %d exited with status 0x%.4X\n", corpse, status);

return 0;
void fun_close(int **fd){
close(fd[0][0]);
close(fd[0][1]);
close(fd[1][0]);
close(fd[1][1]);

}

And here is the fun_read:

int fun_read(char **command, int argc, int fd[]){
char **env_varv;
char value[1024];
char last_var[1024];
long size = 0;
char *token;
    int status;
char *delim = " \t\n";

env_varv = malloc((argc-1) * sizeof(char *));

for(int i = 1; i < argc; i++){
env_varv[i-1] = strdup(command[i]);
wait(status);
}
if (fd[0] !=0){
    printf("%d\n", fd[0]);
    dup2(fd[0],0);
    close(fd[0]);
    close(fd[1]);
}
fgets(value, sizeof(value),stdin);
int i = 0;
token = strtok(value, delim);
last_var[0] = '\0';
while(token != NULL){
    if(i == argc-2){
        while (token != NULL){
            strcat(last_var,token);
            setenv(env_varv[i],last_var,1);
            token = strtok(NULL,delim);
            strcat(last_var," ");
        }
    }
    else if (env_varv[i] != NULL){
        setenv(env_varv[i],token,1);
        token = strtok(NULL,delim);
        i++;
    }
    else{
        break;
    }
}
return 0;

The program should put an envariomental variable called a with the value of example.

postscript: it seems like there is no problem if the previous command is a builtin "echo hi | echo hi2 | read a" $a=hi2

Sincerely I have tried all, changing the pipes doesnt work, changing fgets for read doesn't help either. Is the only part of the code I haven't been able to fix

Answer 1

This fragment of code shows some problems:

char ***argvv;
int fd[2][2];
int pid;
int main(int argc, char ***argvv) {
argvv[0][0] = "echo";
argvv[0][1] = "elpmaxe";
argvv[1][0] = "rev";
argvv[2][0] = "read";
argvv[2][1] = "a";
for (int i = 0; i < 2; i++) {
    pipe(fd[i]);
}
for(int i = 0; i< 3; i++){
    pid = fork();
    if(pid == 0){
        if(i ==0){
            close(fd[0][0]);
            close(fd[1][1]);
            close(fd[1][0]);
            dup2(fd[0][1], 1);
            execvp(argvv[0][0], argvv[0]);
        }
        if(i = 1){
            close(fd[0][1]);
            close(fd[1][0]);
            dup2(fd[0][0], 0);
            dup2(fd[1][1], 1);
            execvp(argvv[1][0], argvv[0]);
        }
        if(i = 2){
            close(fd[0][1]);
            close(fd[0][0]);
            close(fd[1][1]);
            dup2(fd[1][0], 0);
            fun_read("read a", 3, fd[1]);
        }
    }
}

---

Rule of Thumb

You aren't closing enough pipe file descriptors in any of the processes.

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 or F_DUPFD_CLOEXEC.

---

Other comments on the use of pipes

If the parent process will not communicate with any of its children via the pipe, it must ensure that it closes both ends of the pipe early enough (before waiting, for example) so that its children can receive EOF indications on read (or get SIGPIPE signals or write errors on write), rather than blocking indefinitely. Even if the parent uses the pipe without using dup2(), it should normally close at least one end of the pipe — it is extremely rare for a program to read and write on both ends of a single pipe.

Note that the O_CLOEXEC option to open(), and the FD_CLOEXEC and F_DUPFD_CLOEXEC options to fcntl() can also factor into this discussion.

If you use posix_spawn() and its extensive family of support functions (21 functions in total), you will need to review how to close file descriptors in the spawned process (posix_spawn_file_actions_addclose(), etc.).

Note that using dup2(a, b) is safer than using close(b); dup(a); for a variety of reasons. One is that if you want to force the file descriptor to a larger than usual number, dup2() is the only sensible way to do that. Another is that if a is the same as b (e.g. both 0), then dup2() handles it correctly (it doesn't close b before duplicating a) whereas the separate close() and dup() fails horribly. This is an unlikely, but not impossible, circumstance.

---

Analyzing your code

The parent process has the pipes open; if the commands are reading from the pipes, they won't get EOF until the parent process closes them. Although you close most of the pipes in the child processes, you don't close those that you duplicate to the standard I/O channels — and yet that is required too.

Note that if (i = 1) should be if (i == 1), and if (i = 2) should be if (i == 2). The first of those bugs prevents your fun_read() from being invoked — which is why it isn't responding. Using diagnostic printing to standard error would confirm that fun_read() is never called.

So, at bare minimum, you need to have code like this:

char ***argvv;
int fd[2][2];
int pid;

int main(int argc, char ***argvv)
{
    argvv[0][0] = "echo";
    argvv[0][1] = "elpmaxe";
    argvv[1][0] = "rev";
    argvv[2][0] = "read";
    argvv[2][1] = "a";
    for (int i = 0; i < 2; i++)
    {
        pipe(fd[i]);
    }
    for (int i = 0; i < 3; i++)
    {
        pid = fork();
        if (pid == 0)
        {
            if (i == 0)
            {
                dup2(fd[0][1], 1);
                close(fd[0][0]);
                close(fd[0][1]);
                close(fd[1][0]);
                close(fd[1][1]);
                execvp(argvv[0][0], argvv[0]);
                fprintf(stderr, "failed to execute %s\n", argvv[0][0]);
                exit(EXIT_FAILURE);
            }
            if (i == 1)
            {
                dup2(fd[0][0], 0);
                dup2(fd[1][1], 1);
                close(fd[0][0]);
                close(fd[0][1]);
                close(fd[1][0]);
                close(fd[1][1]);
                execvp(argvv[1][0], argvv[0]);
                fprintf(stderr, "failed to execute %s\n", argvv[1][0]);
                exit(EXIT_FAILURE);
            }
            if (i == 2)
            {
                dup2(fd[1][0], 0);
                close(fd[0][0]);
                close(fd[0][1]);
                close(fd[1][0]);
                close(fd[1][1]);
                fun_read("read a", 3, fd[1]);
                exit(EXIT_SUCCESS);
            }
        }
    }
    close(fd[0][0]);
    close(fd[0][1]);
    close(fd[1][0]);
    close(fd[1][1]);

    /* wait loop here - and not before */
    int corpse;
    int status;
    while ((corpse = wait(&status)) > 0)
        printf("Child %d exited with status 0x%.4X\n", corpse, status);

    return 0;
}

Note that it is important to handle failure to execute. And error messages should be reported to standard error, not to standard output.

Given that the same sequence of 4 calls to close() is made 4 times, a function to do the job seems appropriate. You could make it:

static inline void close_pipes(int fd[2][2])
{
    close(fd[0][0]);
    close(fd[0][1]);
    close(fd[1][0]);
    close(fd[1][1]);
}

There is a decent chance the compiler will inline the function, but it is easier to see that the same 4 descriptors are closed if one function always does the closing. For bigger arrays of pipes (more processes), you'd have a loop inside the close_pipes() function with a counter as well as the array.

There are still some issues to be resolved, notably with the fun_read() function. The fd[1] file descriptors were both closed, so passing those to fun_read() doesn't seem likely to be useful. Since fun_read() is executed in a separate process, any changes made by fun_read() won't be reflected in the parent process. There are probably other problems too.

AFAICT, on looking at fun_read() more closely, the fd argument should not be needed at all. The paragraph of code:

if (fd[0] != 0) {
    printf("%d\n", fd[0]);
    dup2(fd[0], 0);
}

is not useful. You've already redirected standard input so it comes from the pipe and then closed the pipe file descriptor. This paragraph then changes standard input to come from the closed descriptor, which isn't going to help anything. But none of this helps you with the fact that anything done by fun_read() is done in a child process of your shell, so the environment in the main shell is not going to be affected.