Date: Sun, 7 Feb 2016 18:13:17 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Pedro F. Giffuni" <pfg@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r295362 - head/sys/fs/cd9660 Message-ID: <20160207175356.A867@besplex.bde.org> In-Reply-To: <201602070348.u173meT4000314@repo.freebsd.org> References: <201602070348.u173meT4000314@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160207175356.A867>