Date: Sat, 30 Dec 2017 20:35:13 +0000 From: "Poul-Henning Kamp" <phk@phk.freebsd.dk> To: Ian Lepore <ian@freebsd.org> Cc: "Rodney W. Grimes" <freebsd-rwg@pdx.rh.CN85.dnsmgr.net>, Eugene Grosbein <eugen@grosbein.net>, Yuri <yuri@rawbw.com>, Freebsd hackers list <freebsd-hackers@freebsd.org> Subject: Re: Is it considered to be ok to not check the return code of close(2) in base? Message-ID: <63076.1514666113@critter.freebsd.dk> In-Reply-To: <1514654174.12000.15.camel@freebsd.org> References: <201712301544.vBUFivES076428@pdx.rh.CN85.dnsmgr.net> <1514654174.12000.15.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--------
In message <1514654174.12000.15.camel@freebsd.org>, Ian Lepore writes:
>No, you're trying to impose a ridiculously simple rule onto what is
>inherently a complex situation. The idea that every close() should be
>checked is just ludicrous.
I used to think so too, but I changed my mind: It is a lot simpler
and much more robust to assert all calls to close succeed, than to
determine which of them are important enough that it needs to be
checked.
Needless to say, if we are talking about /tmp/hack.c which is run once
and thrown away, the rules are different than if you check the program
into FreeBSD.
But for code meant to *live*, I propose that the correct policy at
this time and age is to assert *all* assumptions, so the compiler
and the subsequent maintainers can see what you thought you knew.
And I have hard evidence to back up this position because this policy
has been in effect from day one of the Varnish Cache project, where
as a result close to 10% of source lines contain some kind of assert.
It took 11 years for before Varnish hit the first major security issue.
But as the cherry on top, that issue was "only" a DoS, because the
crash was caused by a well placed assert which prevented it from
becoming a buffer overflow and remote execution problem.
I've also found along the way, that all the asserts greatly speed
up both development and debugging, because they document the usually
tacit assumptions, and in difference from comments, which are almost
always out of date and imprecise, you can trust asserts, because
they are there on every single run through the code.
>What about output to stdout? Does a program under your rules explicitly
>need to close() stdout and check the return code?
This is an interesting thing to bring up.
UNIX provides some hackish conveniences for std{in,out,err} when
they are pipes or terminals, but not when they are regular files.
This means that programs which happily stop when your terminal
session dies does not if the filesystem you redirected std* to fills.
(Consistent behaviour would be to also send SIGPIPE for failed std*
writes to regular files.)
Most programs dont explicityly close std* fd's and explictly
do *not* check all writes, because these workarounds would give a lot
of false positives.
Python does check, and that is why running a python-script into
head(1) results in an annoying error.
For all these reasons I don't think we should use std* as a strawman
in the discussion about close.
>Even the earlier simplistic notion that all close() calls should be
>asserted still overlooks the fact that virtually every application has
>multiple files open, and if an assert triggers on one close(), none of
>the other closes (perhaps equally or more important ones) will ever get
>checked or reported.
This is an utterly bogus argument.
If you want to explicitly ignore the return value of close(2) because
(for whatever reason) it is not important, you should explicitly
tell us so in your source code:
(void)close(fd);
If you want to make sure it worked, but have important stuff to do
in case it fails, you should obviously not use assert(), but write
the proper code to test & handle the issue.
But if you just close a file, and you're 100% sure that will work,
you should write it as:
assert(close(fd) == 0);
To tell the rest of us about your assumption and your confidence in it.
--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
phk@FreeBSD.ORG | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?63076.1514666113>