bugThe nmh Mail Handling System - Bugs: bug #50925, Lack of robustness when disk is...

 
 

bug #50925: Lack of robustness when disk is full

Submitter:  Ralph Corderoy <ralph>
Submitted:  Wed 03 May 2017 09:05:45 AM UTC
   
 
Category:  Bug Severity:  3 - Normal
Priority:  * 5 - Normal Status:  None
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 29 Oct 2018 04:54:09 PM UTC, comment #11: 

fflush after every fputs, etc., seems a bad idea as it discards the
buffering benefit of stdio in avoiding write(2) and its syscall
overhead.  But checking fputs's return value, and all the other fprintf,
etc, is fine as it can still return `disk full', along with other
errors, and save effort by breaking the loop.

stdio errors are sticky.  If fputs("a\n") write(2)s and gets `disk
full', and then fputs("b\n") write(2)s and doesn't, the FILE still has
its error indicator set.  So the error isn't lost by checking fclose in
done().

An fclose(stdout) would return any error without having to fflush() and
ferror().  stderr would still need the fflush() and ferror() because
otherwise file descriptor 2 would be close(2)d by the fclose().

freopen(3) does discard any existing error state, so a call to it is a
contender for fflush+ferror before the freopen.

As for dying in the middle of an inc(1) due to the scan having trouble,
that already happens thanks to SIGPIPE, e.g. `inc | sed 42q', or more
likely `inc | less' and not reading all the input.  And I think that's
fine.  (Perhaps the inc should happen silently, and then a scan follow.)

Ralph Corderoy <ralph>
Group Member
Mon 29 Oct 2018 11:59:44 AM UTC, comment #10: 

Would it make sense to call fflush() and ferror() on both stdout and sterr in done()?  That could provide global handling of errors with minimum code change.

There are a few uses of freopen(), in push.c and slocal.c.  I didn't look to see if they could mask an earlier error.  If so, maybe add the calls before calling it?

David Levine <levine>
Group administrator
Sun 28 Oct 2018 11:52:34 PM UTC, comment #9: 

I'm OK with calling them at the end.  Maybe we'd miss an odd error, but it's better than what we have now (where now is before my last commit).

David Levine <levine>
Group administrator
Sun 28 Oct 2018 09:41:25 PM UTC, comment #8: 

Maybe what makes sense is at the end of every program we call fflush(stdout) and/or ferror(stdout), and return an error if stdout has an error set?

I am uncomfortable modifying scansbr.c, as that's used by inc(1); calling adios() in the middle of that could cause bad things to happen.

Ken Hornstein <kenh>
Group administrator
Sun 28 Oct 2018 06:45:08 PM UTC, comment #7: 

I added a fflush() after each fputs() to be sure to not lose the error.  And that would be easy to turn into a macro.  But, not the best approach in terms of performance.  Any other ideas?

David Levine <levine>
Group administrator
Sun 28 Oct 2018 06:30:29 PM UTC, comment #6: 

commit f54674f1b5d07148d9655fc3b337ad5f725272a7
scan(1) now checks for full output device [Bug #50925].
   
Added fflush(3) after every output line:  may slow down scan
on output with large number of lines.

David Levine <levine>
Group administrator
Sun 28 Oct 2018 04:53:16 PM UTC, comment #5: 

Oh, I think commit a scan fix now.  No point waiting for some day when
it's all going to be fixed in step.  This bug was mainly about using
/dev/full in our tests so we shake some of these problems out and tackle
the important ones;  those, like scan, where a user might think they've
got the useful data saved and delete the email.

Ralph Corderoy <ralph>
Group Member
Sun 28 Oct 2018 01:58:44 PM UTC, comment #4: 

Indeed, fflush() reports the error.  There are many (73) unchecked calls to fflush() of stdout or stderr.  I didn't look to see if there are calls to fputs() on stdout or stderr without a subsequent fflush().  I don't have time now to go after all that.

Should I commit the fix to scan now, or wait for all of this to be addressed at once?

David Levine <levine>
Group administrator
Sat 27 Oct 2018 09:51:20 PM UTC, comment #3: 

fputs(3) is stdio.h so uses buffering.  When a write(2) is attempted, either
during or after the fputs(), the full disk will be seen.  A pending write()
can be forced with fflush(3) or fclose(3), amongst others.  A `sticky' error
from an earlier action can be tested for with ferror(3), though errno might
have been trampled by something else by then.

stdout to a TTY would be line buffered.  I'm guessing it's not with /dev/full
so, with the test program, the write() isn't happening before fputs() returns
that all is well.

Ralph Corderoy <ralph>
Group Member
Sat 27 Oct 2018 03:57:43 PM UTC, comment #2: 

scan uses fputs() and doesn't check its return value.  But even if it did, it doesn't seem to report ENOSPC (on Linux 4,18.8), as shown by attached program:
$ ./fputs_or_write >/dev/full
fputs() returned 1
$ ./fputs_or_write 1 >/dev/full
write() returned -1

Not sure where that leaves us.

(file #45312)

David Levine <levine>
Group administrator
Wed 03 May 2017 09:10:17 AM UTC, comment #1: 

    $ date >/dev/full
    /bin/date: write error: No space left on device
    $ echo $?
    1
    $ scan . >/dev/full
    $ echo $?
    0

packf(1) does check.  Others may or may not.  Could do with using /dev/full, if it's available, for more make-check tests.

Ralph Corderoy <ralph>
Group Member
Wed 03 May 2017 09:05:45 AM UTC, original submission:  


Ralph Corderoy <ralph>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #45312:  fputs_or_write.c added by levine (453B - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kenh (Posted a comment)
  • -email is unavailable- added by levine (Updated the item)
  • -email is unavailable- added by ralph (Submitted the item)
  •  

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-27 levine Attached File- Added fputs_or_write.c, #45312

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code