[core/process] Fix redirections

[akinomyoga]

• 77e3af8 Add failing test cases which are fixed by the subsequent fixes.
• abe89a5 Fix "Try to parse and run ble.sh #653 33. BUG: Redirection of 2 is persistent after : 2>/dev/null >&30"
• 4ed53e1 Fix "Try to parse and run ble.sh #653 37. BUG: {fd}>&- does not close the fd"
• 7f86a1a Fix a problem that >&100 does not fail even if fd 100 does not exist.

[andychu]

Thanks! It's not that surprising to me that there are still bugs left, since even after restructuring I don't grok every part of this code... I remember I mainly wrote it with strace :-/ The FD manipulations are still confusing to me.

I'm strace-ing bash and zsh, and they don't seem to do this check?

bash:

fcntl(1, F_GETFD)                       = 0
fcntl(1, F_DUPFD, 101)                  = 101
fcntl(1, F_GETFD)                       = 0
fcntl(101, F_SETFD, FD_CLOEXEC)         = 0
dup2(100, 1)                            = -1 EBADF (Bad file descriptor)
brk(0x1ce0000)                          = 0x1ce0000

zsh:

fcntl(3, F_DUPFD, 10)                   = 11
close(3)                                = 0
fcntl(11, F_GETFL)                      = 0x8000 (flags O_RDONLY|O_LARGEFILE)
rt_sigprocmask(SIG_BLOCK, [CHLD], [WINCH], 8) = 0
dup(100)                                = -1 EBADF (Bad file descriptor)

mksh has 2 different errors:

$ mksh -c 'echo foo >& 24'
mksh: >&24 : file descriptor too large

$ mksh -c 'echo foo >& 23'
mksh: >&23 : bad file descriptor

OK I guess that's why it already fails for descriptor 99, but not 100... ?

---

But I guess the test cases are good so I should merge this and figure out the code again later ...

[andychu]

Here is what Oil does now...

So I think there could be a pure check that fd1 is not _SHELL_MIN_FD or greater? Since 99 fails but not 100?

I'm not entirely sure about it, but I think that is what mksh does.

fcntl(1, F_DUPFD, 100)                  = 100
close(1)                                = 0
fcntl(100, F_SETFD, FD_CLOEXEC)         = 0
dup2(100, 1)                            = 1
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 45), ...}) = 0
write(1, "foo\n", 4foo
)                    = 4
dup2(100, 1)                            = 1
close(100)                              = 0

[akinomyoga]

Thank you. I think I should have explained what happens here. The problem of not-failing echo hello N>&M occurs when M matches with the smallest unused number (>= _SHELL_MIN_FD). In the current Oil,

1. First M is saved to an unused fd which is N
2. Then N is copied to M

Even if N is invalid before the redirection, it becomes valid by the step 1. But this new N is just a temporary backup of M, so it is not the fd that the user wanted to copy.

I checked strace of bash, zsh and mksh. They actually work around this problem with different approaches. None has this problem that the current Oil has.

Bash

# strace for "echo hello >&12" (valid fd)

fcntl(1, F_GETFD)                       = 0
fcntl(1, F_DUPFD, 13)                   = 13
fcntl(1, F_GETFD)                       = 0
fcntl(13, F_SETFD, FD_CLOEXEC)          = 0
dup2(12, 1)                             = 1
fcntl(12, F_GETFD)                      = 0
write(1, "hello\n", 6)                  = 6
dup2(13, 1)                             = 1
fcntl(13, F_GETFD)                      = 0x1 (flags FD_CLOEXEC)
close(13)                               = 0

# strace for "echo hello >&13" (invalid fd)

fcntl(1, F_GETFD)                       = 0
fcntl(1, F_DUPFD, 14)                   = 14
fcntl(1, F_GETFD)                       = 0
fcntl(14, F_SETFD, FD_CLOEXEC)          = 0
dup2(13, 1)                             = -1 EBADF (Bad file descriptor)
dup2(14, 1)                             = 1
fcntl(14, F_GETFD)                      = 0x1 (flags FD_CLOEXEC)
close(14)                               = 0

Bash behaves like this:

1. First M is saved to the smallest unused fd that is >= max(N+1, 10).
2. Then N is copied to M

Because Bash explicitly specifies X := max(N+1,10) (which is ensured to be larger than N) as fcntl(fd1,F_DUPFD,X), there will be no collision of saved fd and N. This is somewhat a clever way but it performs unneeded fd manipulations when N is invalid. Also, I don't know what happens when N matches with the largest available fd value.

Zsh

# strace for "echo hello >&12" (valid fd)

dup(12)                                 = 3
fcntl(3, F_DUPFD, 10)                   = 15
close(3)                                = 0
fcntl(1, F_DUPFD, 10)                   = 16
close(1)                                = 0
dup2(15, 1)                             = 1
close(15)                               = 0
write(1, "hello\n", 6)                  = 6
dup2(16, 1)                             = 1
close(16)                               = 0

# strace for "echo hello >&15" (invalid fd)

dup(15)                                 = -1 EBADF (Bad file descriptor)

Zsh behaves like this:

1. First N is copied to an unused fd (X).
2. Then M is saved to another fd.
3. Finally X is copied to M

When N is invalid, zsh can detect it on the first step, so zsh can produce the error message. But this approach involves additional fd moves.

Mksh

# strace for "echo hello >&12" (valid fd)

fcntl(12, F_GETFL)                      = 0x8402 (flags O_RDWR|O_APPEND|O_LARGEFILE)
fcntl(1, F_DUPFD, 24)                   = 25
fcntl(25, F_SETFD, FD_CLOEXEC)          = 0
dup2(12, 1)                             = 1
write(1, "hello\n", 6)                  = 6
dup2(25, 1)                             = 1
close(25)                               = 0

# strace for "echo hello >&13" (invalid fd)

fcntl(13, F_GETFL)                      = -1 EBADF (Bad file descriptor)

Mksh uses the same approach as this PR. It first checks if N is valid or not. Only when N is valid it tries to save M and copy N to M.

1. Check validify of N
2. Save M to an unused fd
3. Copy N to M

In addition, mksh prohibits access to fds greater or equal to _SHELL_MIN_FD (24).

---

So I think there could be a pure check that fd1 is not _SHELL_MIN_FD or greater?

If you restrict the value of fd1, we don't have a way to access fds allocated by the redirection of the form {fd}>. mksh could restrict the value of fd because it doesn't have {fd}>.

$ exec {fd}> a.txt # <-- $fd becomes _SHELL_MIN_FD or larger.
$ echo hello >&$fd # <-- we need to access $fd (>= _SHELL_MIN_FD)
$ exec {fd}>&-

[akinomyoga]

I added a test case 48474aa.

[andychu]

OK thank you for the detailed answers! I merged it.

I feel like the check could be optimized away in some cases but since I don't fully understand it yet, it's better to move forward. The tests will help with future refactoring!

[akinomyoga]

Thank you!

I feel like the check could be optimized away

Actually, it will work without the check when if fd1 >= _SHELL_MIN_FD. But, with the check, we can eliminate redundant save/restore of fd2 in the case that fd1 is invalid.