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.
> 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
