script: close our side of the pty if reading stdin throws an error

[kevans]

Summary

In all of the other cases where we'd break out and call finish(), the
other end is probably already gone:

• pselect() won't really error out in our usage, except for EINTR
• if read() on our side of the pty is EOF, the other side is gone
• read() on our side of the pty shouldn't really fail since we've configured it non-blocking and know that data (or EOF) is available

In this case, the child has no way to know that something has gone awry,
so it's helpful to terminate it abruptly rather than hang forever if
it's waiting on something from us, so signal that we should close the
pty after our input buffer has been completely written out to the
process.

While we're here, fix the EOF on stdin case to enqueue our VEOF instead
of sending it immediately to avoid writing events to the process
out-of-order. In the event that one, e.g., redirects a file to script's
stdin, this would almost certainly mean a loss of some input to the
process as we enqueue the file contents pretty quickly and race to
sending it an EOF before the process can read all of it.

PR: 152132

[kib]

usr.bin/script/script.c

403

From quick code reading, it is possible that we have some data read from stdin and buffered in obuf_list, when read(stdin) reports error. Should we wait until all the buffered data is fed to the child before closing master?

416

BTW, the same question should be asked there: isn't it too premature to send VEOF? De-facto it might be sent out of band, should VEOF be queued and pushed in the right order?

[kevans]

Fix both bugs pointed out, since they're somewhat related:

• Drain our buffer to the process before we close the pty
• Enqueue VEOF in-order to avoid truncated input in some cases

[kevans]

usr.bin/script/script.c

416

That's an amusing bug, indeed. I tested with a trivial cat(1) example:

$ echo Hello > inp
$ script _.script cat < inp
Script started, output file is _.script
Hello

Script done, output file is _.script

This isn't obviously wrong until you consider that we don't disable echo on the pty, so we should have seen output doubling: once from echo, once from cat(1) itself. ktrace confirms that we sent "^DHello" so cat(1) hung-up on the zero-sized line without reading our input.

[kib]

I think this would work, but I think it is more future-proof to fix the issues in more generic way.

We might start adding a type to buf_elm. In particular, besides data (as they keep it now), we can add VEOF and close types of buf_elms. Then the write_input() routine would act based on type. The exit criteria would be whatever you have now + empty obuf_list.

[kevans]

Implement our close() special-case as another enqueued element instead.

[kevans]

usr.bin/script/script.c

440

I inadvertently dropped a master >= 0 that I had here, but now I'm wondering: should we avoid processing the ET_CLOSE element above as long as master is still readable as a result of the same pselect(2), just to drain anything immediately available before we lose it?

[kib]

usr.bin/script/script.c

130

147

157

164

I remember that memcpy()ing from NULL is UB, same as memcpy()ing past the allocated object. Even if size == 0.

440

To drain and write it to the stdout and script?
This sounds as a right thing to do.

But if master is set to -1, we have nowhere to read from.

[kevans]

usr.bin/script/script.c

440

Right, my proposal is specifically:

diff --git a/usr.bin/script/script.c b/usr.bin/script/script.c
index 0313aee822e2..435d3e87a412 100644
--- a/usr.bin/script/script.c
+++ b/usr.bin/script/script.c
@@ -431,6 +431,12 @@ main(int argc, char *argv[])
                }
                if (n > 0 && FD_ISSET(master, &wfd)) {
                        while ((be = TAILQ_FIRST(&obuf_list)) != NULL) {
+                               /*
+                                * Drain any pending output before we close it.
+                                */
+                               if (be->etype == ET_CLOSE &&
+                                   FD_ISSET(master, &rfd))
+                                       break;
                                if (!write_input(be, &stt))
                                        break;
 

We'll break out, read the pending stuff, then pselect() -- presumably master would be immediately writable, and we'll close it rather than waiting even longer for more data to maybe become available.

[kib]

usr.bin/script/script.c

440

I am not sure about re-checking FD_ISSET() in the loop, we do not change rfd there.
Did you mean master != -1 instead? But then why ever checking the type of be?

I would just do write_input() until master != -1 and pbuf_list is not empty.

[kevans]

usr.bin/script/script.c

440

No, sorry- since we process rfd only after wfd, I was trying to avoid losing any in-flight reads for this one specific scenario where we know that we're going to close the pty with the very next write_input().

[kib]

usr.bin/script/script.c

440

I do not quite understand the reply. What is the stuff that you said 'no' to? My main claim was that FD_ISSET(master, &rfd) is constant.

[kevans]

usr.bin/script/script.c

440

Mainly: FD_ISSET(master, &rfd) has not yet been tested yet at this point in the loop -- that is in the next block. It could be true, and then we would close master without grabbing data known to be readable (if we had bothered to check)

[kib]

usr.bin/script/script.c

440

Isn't line 440 tests exactly FD_ISSET(master, &rfd)? And the value cannot change in the loop.

[kevans]

usr.bin/script/script.c

440

I see the confusion, sorry; the patch is against the write_input() loop up on line 430 -- I had started the comment here because I had dropped a condition that would defend against us trying to FD_ISSET(-1, ...).

[kib]

usr.bin/script/script.c

440

Ok.