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

From: Eric Sandeen <sandeen@sandeen.net>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 4/4] mkfs: terminate getsubopt arrays properly
Date: Wed, 13 Jul 2022 20:39:24 -0500	[thread overview]
Message-ID: <2a452d57-9e2e-7908-b3f5-0444b6a62761@sandeen.net> (raw)
In-Reply-To: <165767459958.891854.15344618102582353193.stgit@magnolia>

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
> 
