emersion / mrsh

A minimal POSIX shell
MIT License
489 stars 35 forks source link

run_pipeline() misbehaves if pipe() returns fd 0 #171

Closed caseman closed 3 years ago

caseman commented 3 years ago

Howdy,

I'm embedding mrsh in a project that does some additional process and pty management outside of mrsh. tl;dr my program creates a new pty for each command execution and forks setting the pty as stdin, stdout, and stderr. It then invokes mrsh_run_program() in the child process.

There was a bug in my code where I was accidentally closing fd 0 in some cleanup code. This would cause fd 0 to be returned inside mrsh in calls to pipe() (e.g., inside run_pipeline()). This code in pipeline.c does not behave properly in that instance:

            if (i > 0) {
                if (dup2(cur_stdin, STDIN_FILENO) < 0) {
                    fprintf(stderr, "failed to duplicate stdin: %s\n",
                        strerror(errno));
                    return false;
                }
                close(cur_stdin);
            }

There's an implicit assumption above that cur_stdin != STDIN_FILENO (cur_stdin is generated by pipe()) which is not guaranteed. dup2 treats it as a no-op, but the call to close(cur_stdin) leaves things in a bad state for the next command in the pipe. The same holds true for code paths for stdout.

This is definitely an edge case, but some additional checking before closing fds 0-2 could make the library more robust. Happy to work on a PR if that makes sense.

Also thanks very much for your work on mrsh, so far I am very impressed with the quality of your code!

emersion commented 3 years ago

This is definitely an edge case, but some additional checking before closing fds 0-2 could make the library more robust. Happy to work on a PR if that makes sense.

Yes, this definitely sounds like something worth doing. Here we can just add a cur_stdin != STDIN_FILENO check to the if: if cur_stdin is already the same as STDIN_FILENO, there's no need for the dup call in the first place. Does that make sense?

Also thanks very much for your work on mrsh, so far I am very impressed with the quality of your code!

Glad you like it!