From owner-svn-src-all@freebsd.org Sun Feb 7 07:36:35 2016 Return-Path: Delivered-To: svn-src-all@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 3E031A9E898; Sun, 7 Feb 2016 07:36:35 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 034D6B29; Sun, 7 Feb 2016 07:36:34 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c110-21-41-193.carlnfd1.nsw.optusnet.com.au (c110-21-41-193.carlnfd1.nsw.optusnet.com.au [110.21.41.193]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 7AB181A4A63; Sun, 7 Feb 2016 18:13:19 +1100 (AEDT) Date: Sun, 7 Feb 2016 18:13:17 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Pedro F. Giffuni" cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r295362 - head/sys/fs/cd9660 In-Reply-To: <201602070348.u173meT4000314@repo.freebsd.org> Message-ID: <20160207175356.A867@besplex.bde.org> References: <201602070348.u173meT4000314@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=cK4dyQqN c=1 sm=1 tr=0 a=73JWPhLeruqQCjN69UNZtQ==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=GXk1i5ywecH38PYzJGUA:9 a=mjrdrH4rthIuh85Y:21 a=sZ_X_tx9_h26_S3M:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 07 Feb 2016 07:36:35 -0000 On Sun, 7 Feb 2016, Pedro F. Giffuni wrote: > Log: > cd9660: Drop an unnecessary check for NULL. > > This was unnecessary and also confused Coverity. > > Confirmed on: NetBSD > CID: 978558 This leaves many similar bugs unfixed nearby. One is a null pointer panic, not just an unnecessary check. > Modified: head/sys/fs/cd9660/cd9660_vfsops.c > ============================================================================== > --- head/sys/fs/cd9660/cd9660_vfsops.c Sun Feb 7 01:45:24 2016 (r295361) > +++ head/sys/fs/cd9660/cd9660_vfsops.c Sun Feb 7 03:48:40 2016 (r295362) > @@ -741,8 +741,7 @@ cd9660_vget_internal(mp, ino, flags, vpp > if (off + isonum_711(isodir->length) > > imp->logical_block_size) { > vput(vp); > - if (bp != 0) > - brelse(bp); > + brelse(bp); > printf("fhtovp: directory crosses block boundary %d[off=%d/len=%d]\n", > off +isonum_711(isodir->length), off, > isonum_711(isodir->length)); > - 11 lines later, there is the same bug in an #if 0 block. - 9 lines earlier, bp was brelse()ed without a similar check. bp is always null at that point, so checking for this would be just a style bug. Not checking, but doing the wrong cleanup of calling brelse() after bread() fails, gives a null pointer panic in brelse() whenever bread() fails. That seems to be all the similar bugs. bp is correctly abused as a flag later. Other functions depend more critically on bread() setting bp to NULL when it fails. Typical code is to "goto out" when bread() fails, and brelse() bp there if it is not null. Not-so-similar bugs include spelling NULL as 0 or (empty). bread(9) is undocumented. Someone broke its KPI by changing it to a macro. This annoys me when I try to set a breakpoint at it in kernels with the broken KPI. The unimportant badly designed bread(3) is documented. Bruce