Re: [PATCH 02/10] xfs_db: don't crash on a corrupt inode

[Eric Sandeen <sandeen@sandeen.net>]

On 8/14/15 8:43 PM, Darrick J. Wong wrote:
> If the user selects a corrupt inode via the 'inode XXX' command, the
> read verifier will fail and the io cursor at the top of the ring will
> not have any data attached.  When this is the case, we cannot
> dereference the NULL pointer or xfs_db will crash.  Therefore, check
> the buffer pointer before using it.
> 
> It's arguable that we ought to retry the read without the verifiers
> if the inode is corrupt or fails CRC, since this /is/ a debugging
> tool, and maybe you wanted the contents anyway.

I agree.  It seems like we should do that, though it probably needs to
be done across the board for all metadata types if it's going to be done.
Maybe something to add to the TODO?

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/inode.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> 
> diff --git a/db/inode.c b/db/inode.c
> index e86dabd..64b263b 100644
> --- a/db/inode.c
> +++ b/db/inode.c
> @@ -682,6 +682,8 @@ set_cur_inode(
>  	set_cur(&typtab[TYP_INODE], XFS_AGB_TO_DADDR(mp, agno, cluster_agbno),
>  		numblks, DB_RING_IGN, NULL);
>  	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);

off_cur checks for iocur_top == NULL, and warns if it is, so that's good.
The user should have a clue about what's gone wrong, at least.

But, callers of set_cur_inode() are still going to crash often as not:

ablock_f:
        set_cur_inode(iocur_top->ino);
        haveattr = XFS_DFORK_Q((xfs_dinode_t *)iocur_top->data);

bmap:

        set_cur_inode(iocur_top->ino);
        nex = *nexp;
        *nexp = 0;
        ASSERT(nex > 0);
        dip = iocur_top->data;

bmap_f:

                set_cur_inode(iocur_top->ino);
                dip = iocur_top->data;

and a few more :(

Perhaps set_cur_inode() should return failure, so the caller knows to bail,
pop_cur if it needs to, etc?

-Eric

> +	if (!iocur_top->data)
> +		return;
>  	dip = iocur_top->data;
>  	iocur_top->ino_buf = 1;
>  	iocur_top->ino = ino;
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs