svn commit: r295362 - head/sys/fs/cd9660

[Pedro Giffuni]

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