Re: [PATCH] fix: add safety check for OPTION_END and refactor code for consistency

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Fri, Nov 29, 2024 at 04:13:53PM +0530, Atharva Tiwari wrote:
> - Added a null check for 'o' before copying the last OPTION_END in options__order function to prevent potential uninitialized usage.
> - Refactored the parse_long_opt function for improved readability by aligning function signature.
> - Minor formatting fix to ensure consistency in the codebase.
> - Updated the wrapper script for pseries architecture to handle 'notext' option in a more reliable way.
> 
> Signed-off-by: Atharva Tiwari <atharvatiwari1101@outlook.in>
> ---
>  arch/powerpc/boot/wrapper        |  1 +
>  tools/lib/subcmd/parse-options.c | 10 +++++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
> index 1db60fe13802..d25ad8c622f4 100755
> --- a/arch/powerpc/boot/wrapper
> +++ b/arch/powerpc/boot/wrapper
> @@ -267,6 +267,7 @@ pseries)
>      link_address='0x4000000'
>      if [ "$format" != "elf32ppc" ]; then
>  	link_address=
> +	notext='-z notext'

This is a completely differnt change and should be sent to powerpc devs
separately.


>  	pie=-pie
>      fi
>      make_space=n
> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
> index 555d617c1f50..f85b642bc9aa 100644
> --- a/tools/lib/subcmd/parse-options.c
> +++ b/tools/lib/subcmd/parse-options.c
> @@ -360,8 +360,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
>  	return -2;
>  }
>  
> -static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
> -                          const struct option *options)
> +static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg, const struct option *options)

Please don't change this unnecessarily.


>  {
>  	const char *arg_end = strchr(arg, '=');
>  	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> @@ -828,9 +827,10 @@ static struct option *options__order(const struct option *opts)
>  
>  		nr_parent = nr_opts;
>  	}
> -	/* copy the last OPTION_END */
> -	memcpy(&ordered[nr_opts], o, sizeof(*o));
> -
> +	/* Check whether o is  valid before using it to copy the last OPTION_END. */
> +	if (o && o->type == OPTION_END) {
> +		memcpy(&ordered[nr_opts], o, sizeof(*o));
> +	}

Do you any real problems with this?  I think the existing code assumes
the last entry should be OPTION_END.  Otherwise it won't work well.

Thanks,
Namhyung


>  	/* sort each option group individually */
>  	for (opt = group = ordered; opt->type != OPTION_END; opt++) {
>  		if (opt->type == OPTION_GROUP) {
> -- 
> 2.43.-1
>