Date: Sun, 7 Feb 2016 11:03:06 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> 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: <56B76ABA.8000607@FreeBSD.org> In-Reply-To: <20160207175356.A867@besplex.bde.org> References: <201602070348.u173meT4000314@repo.freebsd.org> <20160207175356.A867@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 02/07/16 02:13, Bruce Evans wrote:
> 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.
>
I admittedly oversimplified the commit log here.
Not only the value can't be null, our brelse() also ignores NULL values.
From sys/kern/vfs_bio.c:
____
/*
* Many function erroneously call brelse with a NULL bp under rare
* error conditions. Simply return when called with a NULL bp.
*/
if (bp == NULL)
return;
...
____
And yes, NULL was being misspelled 0 here.
I will look at doing more cleanups later.
Thanks,
Pedro.
>> 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?56B76ABA.8000607>