1

I'm writing a function that echo an input to a sed and then another sed. I thinck i used all my wait signal in the right way but the last print i can get is before the call to dup2() in my first child process in the echo.

void sendbc (char * str_ ) {
    int fd[2];
    int fd1[2];
    int pid,pid1;
    char* echo[] = {"echo", str_,NULL};
    char* sed1[] = {"sed","s/[^:]*;"" " "//",NULL};
    char* sed2[] = {"sed","s/[^:]*."" " "//",NULL};
    int status,er;
    FILE *f;
    if(pipe(fd) < 0){
        exit(100);
    }
    if(pipe(fd1) < 0){
        exit(100);
    }

    pid = fork();
    if (pid == 0) {
        dup2(fd[1], 1) //last command before blocking
        close(fd[1]);
        close(fd[0]);
        execvp(echo[0], echo);
        printf("Error in execvp1\n");
    }else{
        wait(&status);
        pid = fork();
        if (pid == 0){
            dup2(fd[0], 0);
            dup2(fd1[1], 1);
            dup2(fd1[1], 2);
            close(fd[1]);
            close(fd[0]);
            close(fd1[1]);
            close(fd1[0]);
            execvp(sed1[0],sed1);
            printf("Error in execvp2\n");
        }else{
            wait(&status);
            dup2(fd1[0],0);
            dup2(1,2);
            //dup2(1,1);
            close(fd1[1]);
            close(fd1[0]);
            execvp(sed2[0],sed2);
            printf("Error in execvp3\n");
        }
    }

    if(pid!=0)
        wait(&status);
    close(fd[0]);
    close(fd[1]);
    close(fd1[1]);
    close(fd1[0]);
}

I can imagine 2 possibilities... dup2 is blocking or i need to create more process because it end process on call, but this sounds not right after a quick read on his man page... what could it be?

  • 1
    I can't answer your question, but are you sure this string is what it's supposed to be?: `"s/[^:]*;"" " "//"` – ssbssa Jun 12 '20 at 21:08
  • No, it was a random type but it means cut all before ';' and all before '.'. Anyway i printed the tracking of the processes and all it's blocked in the echo dup2... – SaltyGiampy Jun 12 '20 at 21:20
  • You should normally run the processes in a pipeline concurrently, rather than running one, waiting for that to complete, and then running the next. Your data volumes are likely to be small enough that there's no problem; with large volumes of data, that can lead to deadlock. Also, error messages should be printed to `stderr`, not to `stdout`. – Jonathan Leffler Jun 12 '20 at 21:29
  • Note that `.` is a metacharacter to `sed`. If you want to search for a dot, use `[.]` or a backslash escape (but how many backslashes — you need two `"\\."` so the compiler generates a string backslash-dot. If you used `system()` instead of `fork()` and `execvp()`, you'd need 4 backslashes in the C code. – Jonathan Leffler Jun 12 '20 at 21:32
  • 1
    You probably aren't closing enough file descriptors in the child. **Rule of thumb**: If you [`dup2()`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/dup2.html) one end of a pipe to standard input or standard output, close both of the original file descriptors from `pipe()` as soon as possible. In particular, that means before using any of the [`exec*()`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/execvp.html) family of functions. The rule also applies with `dup()` or [`fcntl()`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html) with `F_DUPFD`. – Jonathan Leffler Jun 12 '20 at 21:33
  • I need them on stdout. For the main thing i need them to exec in the correct order, i can't just avoid wait... if i get your advice rightly. For the regex is wrong and i fixed it, also tried from terminal and is correct now. – SaltyGiampy Jun 12 '20 at 21:33
  • but i close them instantly after the dup... and now i also moved the one unused before that... still blocked. – SaltyGiampy Jun 12 '20 at 21:35

1 Answers1

2

General Problem

You aren't closing enough file descriptors in the various processes.


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


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.


Specific Issues

  • You aren't closing enough file descriptors for safety.
  • Your regexes are dubious.
  • You should not make processes in a pipeline wait for each other.

Pet peeve: I prefer to use fd1 and fd2 when I have two closely related variables like the pairs of pipe file descriptors; I find fd and fd1 and the like silly. You may, however, choose to ignore this.

Working Code

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

static void dump_argv(char **argv)
{
    printf("%d:\n", getpid());
    while (*argv != NULL)
    {
        printf("%d: <<%s>>\n", getpid(), *argv++);
    }
}

static void sendbc(char *str)
{
    int fd1[2];
    int fd2[2];
    int pid;
    char *echo[] = {"echo", str, NULL};
    char *sed1[] = {"sed", "s/[^:]*[;]//", NULL};
    char *sed2[] = {"sed", "s/[^:]*[.]//", NULL};
    if (pipe(fd1) < 0)
        exit(100);
    if (pipe(fd2) < 0)
        exit(101);

    printf("%d: at work\n", getpid());
    pid = fork();
    if (pid < 0)
        exit(102);
    else if (pid == 0)
    {
        printf("%d: child 1 - echo\n", getpid());
        dump_argv(echo);
        dup2(fd1[1], 1);
        close(fd1[1]);
        close(fd1[0]);
        close(fd2[0]);
        close(fd2[1]);
        execvp(echo[0], echo);
        fprintf(stderr, "Error in execvp1\n");
        exit(103);
    }
    else
    {
        printf("%d: parent - before second fork\n", getpid());
        pid = fork();
        if (pid == 0)
        {
            printf("%d: child 2 - sed 1\n", getpid());
            dump_argv(sed1);
            dup2(fd1[0], 0);
            dup2(fd2[1], 1);
            close(fd1[1]);
            close(fd1[0]);
            close(fd2[1]);
            close(fd2[0]);
            execvp(sed1[0], sed1);
            fprintf(stderr, "Error in execvp2\n");
            exit(104);
        }
        else
        {
            printf("%d: parent - sed 2\n", getpid());
            dump_argv(sed1);
            dup2(fd2[0], 0);
            close(fd1[1]);
            close(fd1[0]);
            close(fd2[1]);
            close(fd2[0]);
            execvp(sed2[0], sed2);
            fprintf(stderr, "Error in execvp3\n");
            exit(105);
        }
    }
    fprintf(stderr, "Reached unexpectedly\n");
    exit(106);
}

int main(void)
{
    char message[] =
        "This is the first line\n"
        "and this is the second - with a semicolon ; here before a :\n"
        "and the third line has a colon : before the semicolon ;\n"
        "but the fourth line has a dot . before the colon\n"
        "whereas the fifth line has a colon : before the dot .\n"
    ;

    sendbc(message);
    return 0;
}

Example output

$ ./pipe29
74829: at work
74829: parent - before second fork
74829: parent - sed 2
74829:
74829: <<sed>>
74829: <<s/[^:]*[;]//>>
74830: child 1 - echo
74830:
74830: <<echo>>
74830: <<This is the first line
and this is the second - with a semicolon ; here before a :
and the third line has a colon : before the semicolon ;
but the fourth line has a dot . before the colon
whereas the fifth line has a colon : before the dot .
>>
74831: child 2 - sed 1
74831:
74831: <<sed>>
74831: <<s/[^:]*[;]//>>
This is the first line
 here before a :
and the third line has a colon :
 before the colon
whereas the fifth line has a colon :

$

Apart from the diagnostic printing, the primary differences are that this code rigorously closes all the unused ends of the pipes and it contains no calls to wait() or its relatives — they are not needed and in general are harmful when they block concurrent execution of the processes in the pipeline.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • Damn man that is an accurate explanation of everything. I really appreciate your help and thank you so much for your time. It's also a very clean and easy debuggable code! The only problem is that it work just for 1 call, then it stops execution of main funct, but it will be sufficent to add another fork and the game is done! Thank you again! – SaltyGiampy Jun 12 '20 at 22:23
  • 1
    Your original code stopped after the first call so this code does too. However, as you say, it is easily fixed. Just make sure the parent code doesn't get involved with the pipes, or that it closes all the pipes quickly enough (before it waits). – Jonathan Leffler Jun 13 '20 at 17:07
  • I fixed everything thanks to you! Now i'm on my last step of the main code(that's just a little but very problematic funct!) and the problem is to take some pipe content to a var, if you want to check it's my last topic! – SaltyGiampy Jun 13 '20 at 17:22