xfs
[Top] [All Lists]

Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs/quota: fix NULL pointer dereference in report_f
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Fri, 15 Nov 2013 09:42:03 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131114140916.GA13501@xxxxxxxxxxxxx>
References: <52849CC5.10109@xxxxxxxxxx> <20131114140916.GA13501@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0
On 11/14 2013 22:09 PM, Christoph Hellwig wrote:
> On Thu, Nov 14, 2013 at 05:49:57PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> Run xfs_quota report against an invalid XFS path without desired quota
>> limitation is enabled will hit SEGSEGV as fs_path is uninitialized, e.g.
>>
>> # xfs_quota -xc 'report -up' /invalid_path
>> xfs_quota: cannot setup path for mount /invalid_path: No such file or 
>> directory
>> Segmentation fault (core dumped)
>>
>> (gdb) r -xc 'report -up' /invalid_path
>> xfs_quota: cannot setup path for mount /invalid_path: No such file or 
>> directory
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x0000000000408b4d in report_f (argc=2, argv=0x105ea70) at report.c:627
>> 627                  else if (fs_path->fs_flags & FS_MOUNT_POINT)
>>
>> This patch fix report_f() to only do report if the fs_path is initialized.
> 
> quot_f in quot.c and various functions in state.c seem to have the same
> issue, and need fixing as well.
As per a rough tryout while fixing this problem, it seems that most of those
functions are just not be invoked since other precheck-ups were failed before
calling them.  We have another old fix for stat_f():

commit 04418c5901b846cf904d929210cb71dfba44e9a7
Author: Donald Douwsma <donaldd@xxxxxxx>
Date:   Mon Jul 23 04:14:27 2007 +0000

    Fix null pointer dereference in state_f.
    Merge of master-melb:xfs-cmds:29194a by kenmcd.

> I'd love to avoid having to expose fs_path as a global and just have a
> few safe accessors:
> 
> char *global_fsname(void)
> {
>       return fs_path ? fs_path->fs_name : NULL;
> }
> 
> char *global_dir(void)
> {
>       return fs_path ? fs_path->fs_dir : NULL;
> }
> 
> char *global_mountpoint(void)
> {
>       if (fs_path && (fs_path->fs_flags & FS_MOUNT_POINT))
>               return fs_path->fs_dir;
>       return NULL;
> }
> 
> Together with moving a bit of code from quota/init.c to libxcmd/path.c
> that should allow mkaing fs_path and fs_table static there.
Sounds reasonable to me, thanks for your suggestion.
> 
> I'd also really love to see a regression test for xfstests.
I'll take care of it, let me double check all those sub-commands at first.

Thanks,
-Jeff

<Prev in Thread] Current Thread [Next in Thread>