linux-kernel - Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPnjgZ2yrk38kApMSiUOmsNM8M9cDa3gtH-ozJZzk=VjLh+PLQ@mail.gmail.com>
Date:   Tue, 7 Nov 2023 06:11:04 -0700
From:   Simon Glass <sjg@...omium.org>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     linux-arm-kernel@...ts.infradead.org,
        U-Boot Mailing List <u-boot@...ts.denx.de>,
        Tom Rini <trini@...sulko.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nicolas Schier <nicolas@...sle.eu>,
        linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] arm: boot: Use double quotes for image name

Hi Masahiro,

On Tue, 7 Nov 2023 at 03:13, Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> On Sat, Nov 4, 2023 at 9:42 PM Simon Glass <sjg@...omium.org> wrote:
> >
> > The use of single quotes in the image name causes them to appear in
> > the image description when the uImage is created. Use double quotes, to
> > avoid this.
> >
> > Signed-off-by: Simon Glass <sjg@...omium.org>
> > ---
> >
> > Changes in v2:
> > - Split double-quote change out into its own patch
> >
> >  scripts/Makefile.lib | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 68d0134bdbf9..03e79e319293 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -487,7 +487,7 @@ UIMAGE_OPTS-y ?=
> >  UIMAGE_TYPE ?= kernel
> >  UIMAGE_LOADADDR ?= arch_must_set_this
> >  UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
> > -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> >
> >  quiet_cmd_uimage = UIMAGE  $@
> >        cmd_uimage = $(BASH) $(MKIMAGE) -A $(UIMAGE_ARCH) -O linux \
> > --
> > 2.42.0.869.gea05f2083d-goog
> >
>
>
> NACK.
>
>
> This is because you are doing *WRONG* in 3/3.
>
> Look at your code closely.
>
> https://lore.kernel.org/linux-kbuild/20231104194207.3370542-4-sjg@chromium.org/T/#me2fb68151d6f4f330808406f9a711fffee149529
>
>
>
> In the mainline kernel, the quotation appears
> only in the definition of UIMAGE_NAME.
>
>
> masahiro@zoe:~/ref/linux(master)$ git grep UIMAGE_NAME
> scripts/Makefile.lib:UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> scripts/Makefile.lib:                   -n $(UIMAGE_NAME) -d $< $@
>
>
> The single quotes are consumed by shell.
>
>
>
>
>
>
> This is mainline + your patch set.
>
> masahiro@zoe:~/ref/linux(simon-v2)$ git grep UIMAGE_NAME
> scripts/Makefile.lib:UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
> scripts/Makefile.lib:                   -n "$(UIMAGE_NAME)" -d $< $@
> scripts/Makefile.lib:                   --name "$(UIMAGE_NAME)" \
>
>
> You quoted the definition of UIMAGE_NAME,
> and also variable references.
>
>
>
>
> See how it is expanded.
>
>
> --name "$(UIMAGE_NAME)"
>
>
>  ==>
>
>
> --name ""Linux-$(KERNELRELEASE)""
>
>
>  ==>
>
>
> --name Linux-$(KERNELRELEASE)
>
>
>
>
> You added double quotes in a row, just to cancel it.

Yes, I understand that. But without the quotes in -n "$(UIMAGE_NAME)"
then the name cannot contain spaces. So we do need some sort of
quoting, right?

It just seems strange to use single quotes in a Makefile variable. I
found it confusing.

I think you are saying you want to keep the single quotes in the var
declaration and drop the quotes from the cmd_fit rule. I am OK with
that, but I do think it is unusual not to quote something which might
have spaces. It may cause confusion for others, as it did for me?

Anyway, I'll send a new version with the quoting reverted.

Regards,
Simon