From owner-freebsd-hackers@freebsd.org Sat Dec 30 20:35:35 2017 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D4012EAB3A3 for ; Sat, 30 Dec 2017 20:35:35 +0000 (UTC) (envelope-from phk@critter.freebsd.dk) Received: from phk.freebsd.dk (phk.freebsd.dk [130.225.244.222]) by mx1.freebsd.org (Postfix) with ESMTP id 6B63B6BDA5; Sat, 30 Dec 2017 20:35:34 +0000 (UTC) (envelope-from phk@critter.freebsd.dk) Received: from critter.freebsd.dk (unknown [192.168.55.3]) by phk.freebsd.dk (Postfix) with ESMTP id ED4A627365; Sat, 30 Dec 2017 20:35:32 +0000 (UTC) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.15.2/8.15.2) with ESMTPS id vBUKZHSl063078 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 30 Dec 2017 20:35:17 GMT (envelope-from phk@critter.freebsd.dk) Received: (from phk@localhost) by critter.freebsd.dk (8.15.2/8.15.2/Submit) id vBUKZDWv063077; Sat, 30 Dec 2017 20:35:13 GMT (envelope-from phk) To: Ian Lepore cc: "Rodney W. Grimes" , Eugene Grosbein , Yuri , Freebsd hackers list Subject: Re: Is it considered to be ok to not check the return code of close(2) in base? In-reply-to: <1514654174.12000.15.camel@freebsd.org> From: "Poul-Henning Kamp" References: <201712301544.vBUFivES076428@pdx.rh.CN85.dnsmgr.net> <1514654174.12000.15.camel@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <63075.1514666113.1@critter.freebsd.dk> Date: Sat, 30 Dec 2017 20:35:13 +0000 Message-ID: <63076.1514666113@critter.freebsd.dk> X-Mailman-Approved-At: Sun, 31 Dec 2017 00:07:03 +0000 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Dec 2017 20:35:35 -0000 -------- 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.