mitnk / cicada

An old-school bash-like Unix shell written in Rust
https://hugo.wang/cicada/
MIT License
981 stars 50 forks source link

implementation of pipes does not work #34

Closed godmar closed 3 years ago

godmar commented 3 years ago

Try in Cicada:

yes | cat | head -n 3

which should finish. In Cicada, it does not.

mitnk commented 3 years ago

Indeed a bug here. The reason is like #33: cicada's wait() ing children one by one synchronously. waitpid(-1) or setting up a signal handler would able to fix the issue.

Thanks for catching it!

godmar commented 3 years ago

No, it's a different reason. Observe:

12658 pts/0    S      0:00  |   |   \_ target/debug/cicada
12662 pts/0    S+     0:00  |   |       \_ yes
12663 pts/0    S+     0:00  |   |       \_ cat
12664 pts/0    Z+     0:00  |   |       \_ [head] <defunct>

Notice how yes and cat never finish?

The reason is that you fail to close the file descriptors returned by pipe; you're leaking them into the children:

$ ls -l /proc/12662/fd
total 0
lrwx------ 1 gback gback 64 Jul  6 10:56 0 -> /dev/pts/0
l-wx------ 1 gback gback 64 Jul  6 10:56 1 -> 'pipe:[14645553]'
lrwx------ 1 gback gback 64 Jul  6 10:56 2 -> /dev/pts/0
l-wx------ 1 gback gback 64 Jul  6 10:56 4 -> 'pipe:[14645553]'
lr-x------ 1 gback gback 64 Jul  6 10:56 5 -> 'pipe:[14645554]'
l-wx------ 1 gback gback 64 Jul  6 10:56 6 -> 'pipe:[14645554]'

Here, file descriptors 4, 5, and 6 must be closed.

godmar commented 3 years ago

Here's a program you can use:

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

int
main()
{
    int c3 = close(3);
    int c4 = close(4);
    int c5 = close(5);
    int c6 = close(6);
    if (c3 != -1 || c4 != -1 || c5 != -1 || c6 != -1) {
        fprintf(stderr, "file descriptor was leaked\n");
        abort();
    }
}

Run with:

./checkfd | ./checkfd | ./checkfd
mitnk commented 3 years ago

you're leaking file descriptors

Hmm, that's interesting. I had once fixed related issue. Will dig into it.

mitnk commented 3 years ago

Trying fix it in #35, the pipeline now does not stuck anymore, but it cannot pass the ./checkfd check yet.

mitnk commented 3 years ago

Closed more fds not in use, now it passed the .checkfd check.

mitnk commented 3 years ago

PR merged. Thanks again @godmar!