From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@jwatt.org header.s=jwatt.org header.b=s5Yfju6Z; spf=softfail (domain: jwatt.org, ip: 23.83.214.59, mailfrom: jwatt@jwatt.org) Received: from firebrick.maple.relay.mailchannels.net (firebrick.maple.relay.mailchannels.net [23.83.214.59]) by groups.io with SMTP; Tue, 07 May 2019 12:06:40 -0700 X-Sender-Id: dreamhost|x-authsender|jwatt@jwatt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id C42F52C26C3; Tue, 7 May 2019 19:06:36 +0000 (UTC) Received: from pdx1-sub0-mail-a17.g.dreamhost.com (100-96-79-6.trex.outbound.svc.cluster.local [100.96.79.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 0C5A82C1BFC; Tue, 7 May 2019 19:06:36 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|jwatt@jwatt.org Received: from pdx1-sub0-mail-a17.g.dreamhost.com ([TEMPUNAVAIL]. [64.90.62.162]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384) by 0.0.0.0:2500 (trex/5.17.2); Tue, 07 May 2019 19:06:36 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|jwatt@jwatt.org X-MailChannels-Auth-Id: dreamhost X-Rock-Blushing: 214033c52160dd48_1557255996511_2151356364 X-MC-Loop-Signature: 1557255996511:1550638571 X-MC-Ingress-Time: 1557255996511 Received: from pdx1-sub0-mail-a17.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a17.g.dreamhost.com (Postfix) with ESMTP id 5B9D87FA5D; Tue, 7 May 2019 12:06:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=jwatt.org; h=subject:from :to:cc:references:message-id:date:mime-version:in-reply-to :content-type:content-transfer-encoding; s=jwatt.org; bh=7G0pOHw X4I3O3mpXWaHNqfk92Eg=; b=s5Yfju6Zb+2lrUeBHdbmvw11+7zwn2h9bz7OtZ8 cQ/D7fdKxk8jaiYE63Uf78F/4EmW65bk/ucdpA9px1de/rU7nogNZL61yRd5Kyom 5B0ucAEjMLro5FH8xV6gFpkzRaZIWuhb0gFdHQl4qB9UVBS4y0FC1wv8dl6oI6kJ dn+g= Received: from MacBook-Pro-6.localdomain (cpc120804-nrwh12-2-0-cust660.4-4.cable.virginm.net [82.40.106.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: jwatt@jwatt.org) by pdx1-sub0-mail-a17.g.dreamhost.com (Postfix) with ESMTPSA id 74EA67FA5A; Tue, 7 May 2019 12:06:29 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option X-DH-BACKEND: pdx1-sub0-mail-a17 From: "Jonathan Watt" To: "Carsey, Jaben" , "devel@edk2.groups.io" , "tim.lewis@insyde.com" , "Gao, Zhichao" , "Ni, Ray" Cc: "Bi, Dandan" References: <047401d504f0$c6020160$52060420$@insyde.com> Openpgp: preference=signencrypt Message-ID: <37952ef0-4ca3-25d5-8e77-e5977ff0e687@jwatt.org> Date: Tue, 7 May 2019 20:06:27 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: X-VR-OUT-STATUS: OK X-VR-OUT-SCORE: -100 X-VR-OUT-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduuddrkedtgddufeekucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuggftfghnshhusghstghrihgsvgdpffftgfetoffjqffuvfenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhepuffhvfhfkffffgggjggtgfesthejredttdefjeenucfhrhhomheplfhonhgrthhhrghnucghrghtthcuoehjfigrthhtsehjfigrthhtrdhorhhgqeenucfkphepkedvrdegtddruddtiedrudegleenucfrrghrrghmpehmohguvgepshhmthhppdhhvghlohepofgrtgeuohhokhdqrfhrohdqiedrlhhotggrlhguohhmrghinhdpihhnvghtpeekvddrgedtrddutdeirddugeelpdhrvghtuhhrnhdqphgrthhhpeflohhnrghthhgrnhcuhggrthhtuceojhifrghtthesjhifrghtthdrohhrgheqpdhmrghilhhfrhhomhepjhifrghtthesjhifrghtthdrohhrghdpnhhrtghpthhtohepjhgrsggvnhdrtggrrhhsvgihsehinhhtvghlrdgtohhmnecuvehluhhsthgvrhfuihiivgeptd Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit I should add, for me personally, once I noticed the inconsistency I changed my scripts to use the "0x" prefix to avoid this real footgun. I imagine that anyone else that may have encountered this would have done the same and so, like me, wouldn't be affected by the change if it were to happen. On 07/05/2019 20:00, Jonathan Watt wrote: > There is potential for that, but it's not certain. For it to happen scripts > would need to be both omitting the "0x" prefix and be pass an option number > greater than 9. The fact this very unexpected inconsistency (which will corrupt > the wrong option when those same two things are true!) hasn't been reported > before would seem to indicate this combination doesn't really happen/is rare in > practice. > > Also, is TianoCore's bcfg the only implementation people are using? If there are > other implementations, would this bring TianoCore's implementation into or out > of line with them? That may impact whether the spec could/should change. > > On 07/05/2019 18:40, Carsey, Jaben wrote: >> It will break existing scripts. Do you have such scripts in your environment dependent on this parameter? >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >>> Tim Lewis >>> Sent: Tuesday, May 07, 2019 9:20 AM >>> To: devel@edk2.groups.io; Carsey, Jaben ; Gao, >>> Zhichao ; Ni, Ray ; >>> jwatt@jwatt.org >>> Cc: Bi, Dandan >>> Subject: Re: [edk2-devel] [PATCH v1 1/1] >>> ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option >>> Importance: High >>> >>> The question is whether this will break compatibility with existing shell >>> scripts. In order to maintain that compatibility, it may be necessary to add >>> a new option rather than trying to update an existing one. >>> >>> Tim >>> >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of Carsey, >>> Jaben >>> Sent: Tuesday, May 7, 2019 7:36 AM >>> To: Gao, Zhichao ; devel@edk2.groups.io; Ni, Ray >>> ; jwatt@jwatt.org >>> Cc: Bi, Dandan >>> Subject: Re: [edk2-devel] [PATCH v1 1/1] >>> ShellPkg/UefiShellBcfgCommandLib: >>> Fix '-opt' option >>> >>> Zhichao, >>> I can help submit errata for shell spec if needed. >>> >>> Per patch, >>> I agree. This looks good. >>> Reviewed-by: Jaben Carsey >>> >>> >>>> -----Original Message----- >>>> From: Gao, Zhichao >>>> Sent: Tuesday, May 07, 2019 2:52 AM >>>> To: devel@edk2.groups.io; Ni, Ray ; jwatt@jwatt.org >>>> Cc: Carsey, Jaben ; Bi, Dandan >>>> >>>> Subject: RE: [edk2-devel] [PATCH v1 1/1] >>>> ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option >>>> Importance: High >>>> >>>> This patch looks good for me. >>>> Reviewed-by: Zhichao Gao >>>> >>>> But when I view the command in UEFI SHELL 2.2 spec: >>>> ... >>>> bcfg driver|boot [-opt # [[filename]|["data"]] | [KeyData >>> UnicodeChar>*]] >>>> ... >>>> -opt >>>> Modify the optional data associated with a driver or boot option. >>>> Followed either by the filename of the file which contains the binary >>>> data to be associated with the driver or boot option optional data, or >>>> else the quote- delimited data that will be associated with the driver >>>> or boot option optional data. >>>> ... >>>> >>>> This description lack the comment of '#' parameter and that may make >>>> the consumer confused. Usually consumers would regard it as the same >>>> in other option, such as ' bcfg driver|boot [rm #]'. The '#' is >>>> clearly descripted as a hexadecimal parameter: >>>> rm >>>> Remove an option. The # parameter lists the option number to remove in >>>> hexadecimal. >>>> >>>> So I think we should update the shell spec by the way. >>>> >>>> Thanks, >>>> Zhichao >>>> >>>>> -----Original Message----- >>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf >>>>> Of >>>> Ni, >>>>> Ray >>>>> Sent: Monday, May 6, 2019 10:02 PM >>>>> To: jwatt@jwatt.org; devel@edk2.groups.io >>>>> Cc: Carsey, Jaben ; Bi, Dandan >>>>> >>>>> Subject: Re: [edk2-devel] [PATCH v1 1/1] >>>> ShellPkg/UefiShellBcfgCommandLib: >>>>> Fix '-opt' option >>>>> >>>>> Dandan, >>>>> Can you please help to review? >>>>> >>>>> Thanks, >>>>> Ray >>>>> >>>>>> -----Original Message----- >>>>>> From: jwatt@jwatt.org [mailto:jwatt@jwatt.org] >>>>>> Sent: Monday, May 6, 2019 9:03 PM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Carsey, Jaben ; Ni, Ray >>>>>> >>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' >>>>>> option >>>>>> >>>>>> From: Jonathan Watt >>>>>> >>>>>> For all other bcfg commands the "#" (option number) argument(s) >>>>>> are treated as hexedecimal values regardless of whether or not >>>>>> they are prefixed by "0x". This change fixes '-opt' to handle its "#" >>>>>> (option number) argument consistently with the other commands. >>>>>> >>>>>> Making this change removes a potential footgun whereby a user that >>>>>> has been using a number without a "0x" prefix with other bcfg >>>>>> commands finds that, on using that exact same number with '-opt', >>>>>> it has this time unexpectedly been interpreted as a decimal number >>>>>> and they have modified >>>>>> (corrupted) an unrelated load option. For example, a user may >>>>>> have been specifying "10" to other commands to have them act on >>>>>> the 16th option (because simply "10", without any prefix, is how >>>>>> 'bcfg boot dump' displayed the option number for the 16th option). >>>>>> Unfortunately for them, if they also use '-opt' with "10" it would >>>>>> unexpectedly and inconsistently act on the 10th option. >>>>>> >>>>>> CC: Jaben Carsey >>>>>> CC: Ray Ni >>>>>> Signed-off-by: Jonathan Watt >>>>>> --- >>>>>> >>>>>> >>> ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c >>>> | >>>>>> 2 >>>>>> +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git >>>>>> >>>> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c >>>>>> >>>> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c >>>>>> index d033c7c1dc59..e8b48b4990dd 100644 >>>>>> --- >>>>>> >>>> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c >>>>>> +++ >>>>>> >>>> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c >>>>>> @@ -1019,7 +1019,7 @@ BcfgAddOpt( >>>>>> // >>>>>> // Get the index of the variable we are changing. >>>>>> // >>>>>> - Status = ShellConvertStringToUint64(Walker, &Intermediate, >>>>>> FALSE, TRUE); >>>>>> + Status = ShellConvertStringToUint64(Walker, &Intermediate, >>>>>> + TRUE, TRUE); >>>>>> if (EFI_ERROR(Status) || (((UINT16)Intermediate) != >>>>>> Intermediate) >>>>>> || StrStr(Walker, L" ") == NULL || ((UINT16)Intermediate) > >>>>>> ((UINT16)OrderCount)) { >>>>>> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN >>>>>> (STR_GEN_PARAM_INV), gShellBcfgHiiHandle, L"bcfg", L"Option >>> Index"); >>>>>> ShellStatus = SHELL_INVALID_PARAMETER; >>>>>> -- >>>>>> 2.21.0 >>>>> >>>>> >>>>> >>> >>> >>> >>> >>> >>> >>> >> >> >