From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/7461 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Static analysis results Date: Tue, 21 Apr 2015 12:57:53 -0400 Message-ID: <20150421165753.GV6817@brightrain.aerifal.cx> References: Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1429635497 17073 80.91.229.3 (21 Apr 2015 16:58:17 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 21 Apr 2015 16:58:17 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-7474-gllmg-musl=m.gmane.org@lists.openwall.com Tue Apr 21 18:58:09 2015 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1YkbUu-0004vR-Nz for gllmg-musl@m.gmane.org; Tue, 21 Apr 2015 18:58:08 +0200 Original-Received: (qmail 30416 invoked by uid 550); 21 Apr 2015 16:58:07 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 30395 invoked from network); 21 Apr 2015 16:58:06 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:7461 Archived-At: On Tue, Apr 21, 2015 at 07:28:30PM +0300, Alexander Monakov wrote: > New round of static analysis results. This time it's mostly opportunities for > very minor cleanups (I'm showing only a few results that I think make sense). > If there's a problem in balance of usefulness vs annoyance, please let me know. > > dynlink.c:343 > 'if (runtime)' is already established as true at line 337 This is a matter of a mechanical transform of all calls to error() that was recently made to get the longjmp out of error(). It could just be removed. > sem_open.c:sem_open > I didn't try to follow the code in detail, but it seems possible that 'goto > fail' can be executed from e.g. line 133 after successful mmap, in which > case the region is not unmapped I agree. I think immediately after line 128 (if (!e) break;) we need munmap(map,sizeof(sem_t)). That covers unmapping for both the failure and retry cases. > duplocale.c:17 > neither of the conditions cannot hold Indeed, that was cruft from before the body of the function was added. The whole memcpy seems wrong though; it undoes work done above. I need to look into this. > dynlink.c:1503 > the first two conditions cannot hold after check at line 1489 and exit at > line 1501 These useless checks were added as the only content of commit 637dd2d383cc1f63bf02a732f03786857b22c7bd claiming it fixed a regression, but I don't have any information on whether that regression was observed (in which case it must have been a problem somewhere else) or just theoretical and incorrect. I would guess the latter. > fcntl.c:42 > F_SETLKW is already taken care of at line 16 Yes. > also, why does this file cast arg to 'void *' in several places? It's an utter mess. Really we should be calling va_arg with the right type for the command but this results in much larger code and has no practical benefits at this time. > regcomp.c:2848 > condition 'stack != NULL' cannot hold Didn't you mean it's always true? > dynlink.c:428 > on 64-bit arches, multiplication can overflow in 32-bit type before assignment It can on 32-bit too. At present the validity of stuff from loaded ELF files is not scrutinized. We're going to be running code from them anyway. There's been some interest in making it safe against invalid ELF files for the sake of ldd, but that would be a big project. Rich