Re: [PATCH 4/4] mkfs: terminate getsubopt arrays properly

["Darrick J. Wong" <djwong@kernel.org>]

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