Page MenuHomeFreeBSD

lam: fix using stdin more than once
ClosedPublic

Authored by kevans on Fri, Nov 14, 1:53 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 16, 2:34 PM
Unknown Object (File)
Sun, Nov 16, 2:34 PM
Unknown Object (File)
Sun, Nov 16, 2:34 PM
Unknown Object (File)
Sun, Nov 16, 12:26 PM
Unknown Object (File)
Sat, Nov 15, 10:13 AM
Unknown Object (File)
Sat, Nov 15, 9:52 AM
Unknown Object (File)
Fri, Nov 14, 9:04 PM
Unknown Object (File)
Fri, Nov 14, 9:04 PM
Subscribers

Details

Summary

Historically, lam(1) closes stdin once we've hit EOF the first time,
which would stop it from doing anything else on subsequent gatherline()
calls with another openfile. However, this doesn't seem to be strictly
necessary- the EOF flag on FILEs is quite sticky, so we can assume that
a subsequent fgetc(stdin) will flag EOF properly.

This 'fixes' the below-referenced commit in the sense that it surfaced
this problem as a fatal error, but the issue was pre-existing. If we
do lam - -, then one gatherline() will fclose(stdin) and set ip->eof
for *that* openfile, while the next one will then observe that
STDIN_FILENO has been closed and turn it into an EBADF.

Add a few tests that were easy to snipe while I'm here, but I haven't
aimed for anything close to exhaustive because think re@ would prefer
this fix go in sooner rather than later to land in 15.0.

Reported by: cperciva
Fixes: 4472fd66d006 ("lam: fail on I/O errors")
Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jlduran added a subscriber: jlduran.

I suggested via email:

diff
--- a/usr.bin/lam/lam.c
+++ b/usr.bin/lam/lam.c
@@ -213,11 +213,11 @@ gatherline(struct openfile *ip)
        *p = '\0';
        if (c == EOF) {
                ip->eof = 1;
+               if (ip->fp == stdin)
+                       fclose(stdin);
                if (ferror(ip->fp)) {
                        err(EX_IOERR, NULL);
                }
-               if (ip->fp == stdin)
-                       fclose(stdin);
                morefiles--;
                return (pad(ip));
        }

But this is fine as well. Plus, it has tests!

This revision is now accepted and ready to land.Fri, Nov 14, 3:26 AM

I suggested via email:

diff
--- a/usr.bin/lam/lam.c
+++ b/usr.bin/lam/lam.c
@@ -213,11 +213,11 @@ gatherline(struct openfile *ip)
        *p = '\0';
        if (c == EOF) {
                ip->eof = 1;
+               if (ip->fp == stdin)
+                       fclose(stdin);
                if (ferror(ip->fp)) {
                        err(EX_IOERR, NULL);
                }
-               if (ip->fp == stdin)
-                       fclose(stdin);
                morefiles--;
                return (pad(ip));
        }

But this is fine as well. Plus, it has tests!

I believe that would actually just blow away the error state and we'd then never detect a real error on stdin (+ the next gatherline call would still have the 'other' ip in an inconsistent state)

des added inline comments.
usr.bin/lam/lam.c
217–218

style nit (introduced in the previous commit): inconsistent style, the rest of the file does not put braces around single-statement clauses.

kevans added inline comments.
usr.bin/lam/lam.c
217–218

Addressed locally, thanks

This revision was automatically updated to reflect the committed changes.