On 12/15/16 6:58 PM, Darrick J. Wong wrote:
> On Thu, Dec 15, 2016 at 05:17:14PM -0600, Eric Sandeen wrote:
>> On 12/15/16 12:11 PM, Darrick J. Wong wrote:
>>> Fix some potential NULL pointer deferences that Coverity pointed out,
>>> and remove a trivial dead integer check.
>>>
>>> Coverity-id: 1375789, 1375790, 1375791, 1375792
>>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>> ---
>>> repair/phase5.c | 2 +-
>>> repair/rmap.c | 2 +-
>>> repair/slab.h | 2 +-
>>> 3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>>
>>> diff --git a/repair/phase5.c b/repair/phase5.c
>>> index 3604d1d..cbda556 100644
>>> --- a/repair/phase5.c
>>> +++ b/repair/phase5.c
>>> @@ -1925,7 +1925,7 @@ _("Insufficient memory to construct refcount cursor."));
>>> refc_rec = pop_slab_cursor(refc_cur);
>>> lptr = &btree_curs->level[0];
>>>
>>> - for (i = 0; i < lptr->num_blocks; i++) {
>>> + for (i = 0; i < lptr->num_blocks && refc_rec != NULL; i++) {
>>
>> Ok, you sent this patch already as well :)
>>
>> And it matches the exact same thing that exists for a while in
>> the rmap_tree builder, so yay for that. :)
>>
>> but is this just shutting up coverity, or is this something that could
>> happen? If it /does/ happen, is skipping the loop all we really need
>> to do? Sorry - I have to think harder about what's going on in here
>> as we build the tree, but maybe you already know.
>
> It's just shutting Coverity up -- we should always run out of num_blocks
> before pop_slab_cursor runs out of records to put in the leaf blocks and
> returns NULL. init_{rmap,refcount}bt_cursor does the following:
>
> num_recs = {rmap,refcount}_record_count(...);
> lptr->num_blocks = (num_recs + maxrecs - 1) / maxrecs;
>
> But there's no harm in sanity checking just in case this ever becomes
> untrue.
I wonder if an ASSERT would also shut Coverity up - that'd communicate
to coverity that we can't ever deref a null ptr, but also communicates
to the reader that a null ptr is not expected (better than a test
in a loop, which looks like maybe it is expected...)
/me wishes covscan allowed scratch builds to test
-Eric
>> My sense of tidiness and consistency is bothered by the fact that
>> we don't have similar null-check-skip-loop for, say, ino_rec
>> in build_ino_tree, or ext_ptr in build_freespace_tree.
>
> The other tree builders don't use slabs, so the loops are different.
>
> --D
>
>>
>>
>>> /*
>>> * block initialization, lay in block header
>>> */
>>> diff --git a/repair/rmap.c b/repair/rmap.c
>>> index 45e183a..7508973 100644
>>> --- a/repair/rmap.c
>>> +++ b/repair/rmap.c
>>> @@ -790,7 +790,7 @@ compute_refcounts(
>>> mark_inode_rl(mp, stack_top);
>>>
>>> /* Set nbno to the bno of the next refcount change */
>>> - if (n < slab_count(rmaps))
>>> + if (n < slab_count(rmaps) && array_cur)
>>> nbno = array_cur->rm_startblock;
>>> else
>>> nbno = NULLAGBLOCK;
>>> diff --git a/repair/slab.h b/repair/slab.h
>>> index 4aa5512..a2201f1 100644
>>> --- a/repair/slab.h
>>> +++ b/repair/slab.h
>>> @@ -54,7 +54,7 @@ extern void *bag_item(struct xfs_bag *, size_t);
>>>
>>> #define foreach_bag_ptr_reverse(bag, idx, ptr) \
>>> for ((idx) = bag_count(bag) - 1, (ptr) = bag_item((bag), (idx)); \
>>> - (idx) >= 0 && (ptr) != NULL; \
>>> + (ptr) != NULL; \
>>> (idx)--, (ptr) = bag_item((bag), (idx)))
>>>
>>> #endif /* SLAB_H_ */
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
