diff mbox series

[4/4] mkfs: terminate getsubopt arrays properly

Message ID 165767459958.891854.15344618102582353193.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfsprogs: random fixes | expand

Commit Message

Darrick J. Wong July 13, 2022, 1:09 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Having not drank any (or maybe too much) coffee this morning, I typed:

$ mkfs.xfs -d agcount=3 -d nrext64=0
Segmentation fault

I traced this down to getsubopt walking off the end of the dopts.subopts
array.  The manpage says you're supposed to terminate the suboptions
string array with a NULL entry, but the structure definition uses
MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Sandeen July 14, 2022, 1:39 a.m. UTC | #1 
On 7/12/22 8:09 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Having not drank any (or maybe too much) coffee this morning, I typed:
> 
> $ mkfs.xfs -d agcount=3 -d nrext64=0
> Segmentation fault
> 
> I traced this down to getsubopt walking off the end of the dopts.subopts
> array.  The manpage says you're supposed to terminate the suboptions

(the getsubopt(3) manpage for those following along at home)

> string array with a NULL entry, but the structure definition uses
> MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/xfs_mkfs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 61ac1a4a..9a58ff8b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -141,7 +141,7 @@ enum {
>  };
>  
>  /* Just define the max options array size manually right now */
> -#define MAX_SUBOPTS	D_MAX_OPTS
> +#define MAX_SUBOPTS	(D_MAX_OPTS + 1)

Hah, I had not noticed this before. So this relies on there being more
suboptions for -d than anything else, I guess. What could go wrong?

OK, so this fixes it because opt_params is a global, and it contains 
subopt_params[MAX_SUBOPTS];, so the last array entry will be null
(by virtue of globals being zeroed) and that's all perfectly clear :D

Well, it fixes it for now.  I'd like to add i.e.

@@ -251,6 +251,7 @@ static struct opt_params bopts = {
        .ini_section = "block",
        .subopts = {
                [B_SIZE] = "size",
+               [B_MAX_OPTS] = NULL,
        },

etc to each suboption array to be explicit about it, sound ok? I can do
that on commit if it seems ok.

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

Thanks,
-Eric

>  
>  #define SUBOPT_NEEDS_VAL	(-1LL)
>  #define MAX_CONFLICTS	8
>
Darrick J. Wong July 14, 2022, 1:59 a.m. UTC | #2 
On Wed, Jul 13, 2022 at 08:39:24PM -0500, Eric Sandeen wrote:
> On 7/12/22 8:09 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Having not drank any (or maybe too much) coffee this morning, I typed:
> > 
> > $ mkfs.xfs -d agcount=3 -d nrext64=0
> > Segmentation fault
> > 
> > I traced this down to getsubopt walking off the end of the dopts.subopts
> > array.  The manpage says you're supposed to terminate the suboptions
> 
> (the getsubopt(3) manpage for those following along at home)
> 
> > string array with a NULL entry, but the structure definition uses
> > MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  mkfs/xfs_mkfs.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 61ac1a4a..9a58ff8b 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -141,7 +141,7 @@ enum {
> >  };
> >  
> >  /* Just define the max options array size manually right now */
> > -#define MAX_SUBOPTS	D_MAX_OPTS
> > +#define MAX_SUBOPTS	(D_MAX_OPTS + 1)
> 
> Hah, I had not noticed this before. So this relies on there being more
> suboptions for -d than anything else, I guess. What could go wrong?
> 
> OK, so this fixes it because opt_params is a global, and it contains 
> subopt_params[MAX_SUBOPTS];, so the last array entry will be null
> (by virtue of globals being zeroed) and that's all perfectly clear :D

<nod>

> Well, it fixes it for now.  I'd like to add i.e.
> 
> @@ -251,6 +251,7 @@ static struct opt_params bopts = {
>         .ini_section = "block",
>         .subopts = {
>                 [B_SIZE] = "size",
> +               [B_MAX_OPTS] = NULL,
>         },
> 
> etc to each suboption array to be explicit about it, sound ok? I can do
> that on commit if it seems ok.

Oh, that /is/ a good idea, in case B_MAX_OPTS > D_MAX_OPTS ever happens.

--D

> Reviewed-by: Eric Sandeen <sandeen@sandeen.net>
> 
> Thanks,
> -Eric
> 
> >  
> >  #define SUBOPT_NEEDS_VAL	(-1LL)
> >  #define MAX_CONFLICTS	8
> >
Eric Sandeen July 14, 2022, 2:03 a.m. UTC | #3 
On 7/13/22 8:59 PM, Darrick J. Wong wrote:
> On Wed, Jul 13, 2022 at 08:39:24PM -0500, Eric Sandeen wrote:
>> On 7/12/22 8:09 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@kernel.org>
>>>
>>> Having not drank any (or maybe too much) coffee this morning, I typed:
>>>
>>> $ mkfs.xfs -d agcount=3 -d nrext64=0
>>> Segmentation fault
>>>
>>> I traced this down to getsubopt walking off the end of the dopts.subopts
>>> array.  The manpage says you're supposed to terminate the suboptions
>>
>> (the getsubopt(3) manpage for those following along at home)
>>
>>> string array with a NULL entry, but the structure definition uses
>>> MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>> ---
>>>  mkfs/xfs_mkfs.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index 61ac1a4a..9a58ff8b 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -141,7 +141,7 @@ enum {
>>>  };
>>>  
>>>  /* Just define the max options array size manually right now */
>>> -#define MAX_SUBOPTS	D_MAX_OPTS
>>> +#define MAX_SUBOPTS	(D_MAX_OPTS + 1)
>>
>> Hah, I had not noticed this before. So this relies on there being more
>> suboptions for -d than anything else, I guess. What could go wrong?
>>
>> OK, so this fixes it because opt_params is a global, and it contains 
>> subopt_params[MAX_SUBOPTS];, so the last array entry will be null
>> (by virtue of globals being zeroed) and that's all perfectly clear :D
> 
> <nod>
> 
>> Well, it fixes it for now.  I'd like to add i.e.
>>
>> @@ -251,6 +251,7 @@ static struct opt_params bopts = {
>>         .ini_section = "block",
>>         .subopts = {
>>                 [B_SIZE] = "size",
>> +               [B_MAX_OPTS] = NULL,
>>         },
>>
>> etc to each suboption array to be explicit about it, sound ok? I can do
>> that on commit if it seems ok.
> 
> Oh, that /is/ a good idea, in case B_MAX_OPTS > D_MAX_OPTS ever happens.

I, uh, think that in that case, gcc will barf out with something like:

xfs_mkfs.c:311:3: error: array index in initializer exceeds array bounds
   [D_MAX_OPTS] = NULL,
   ^
xfs_mkfs.c:311:3: error: (near initialization for ‘dopts.subopts’)
xfs_mkfs.c:311:3: warning: excess elements in array initializer [enabled by default]
xfs_mkfs.c:311:3: warning: (near initialization for ‘dopts.subopts’) [enabled by default]
cc1: warning: unrecognized command line option "-Wno-address-of-packed-member" [enabled by default]
gmake[2]: *** [xfs_mkfs.o] Error 1
gmake[1]: *** [mkfs] Error 2
make: *** [default] Error 2

(with s/dopts/bopts/ in your case)

-Eric
diff mbox series

Patch

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 61ac1a4a..9a58ff8b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -141,7 +141,7 @@  enum {
 };
 
 /* Just define the max options array size manually right now */
-#define MAX_SUBOPTS	D_MAX_OPTS
+#define MAX_SUBOPTS	(D_MAX_OPTS + 1)
 
 #define SUBOPT_NEEDS_VAL	(-1LL)
 #define MAX_CONFLICTS	8