From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: insyde.com, ip: 210.71.195.44, mailfrom: tim.lewis@insyde.com) Received: from out01.hibox.biz (out01.hibox.biz [210.71.195.44]) by groups.io with SMTP; Tue, 07 May 2019 13:04:30 -0700 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2DGAgBh4dFc/w00GKxkHAEBAQQBAQc?= =?us-ascii?q?EAQGBZYJ6chIog0hIiHuMKTUBgwOLBRGESCOHExAjCQGEOgQCAoI5OBMBAwE?= =?us-ascii?q?BBQEBAQECbRwMhUoBAQUIAhkrCAgbDAEFBgMNAQMEAQEBAgIjAwIbGBUJCAI?= =?us-ascii?q?EARILBQ0EgwKCCg+uAIEvGgKDagErARFDhSmBCyeBD1GLRT+BEYMSPoQNIYM?= =?us-ascii?q?gglgEincHGYI/hXqTDWUHAgKCCVkBhUOEMoVTgi0bghBihWKDXQOJI4wkgSG?= =?us-ascii?q?FLI5RgWYhgVZwgzwJNoIHgieDdYIFhTMBViIwgSEIE41LglIBAQ?= X-IronPort-AV: E=Sophos;i="5.60,443,1549900800"; d="scan'208";a="35783280" Received: from unknown (HELO hb3-BKT203.hibox.biz) ([172.24.52.13]) by out01.hibox.biz with ESMTP; 08 May 2019 04:04:24 +0800 Received: from unknown (HELO hb3-BKT101.hibox.biz) ([172.24.51.11]) by hb3-BKT203.hibox.biz with ESMTP; 08 May 2019 04:04:24 +0800 Received: from unknown (HELO hb3-IN03.hibox.biz) ([172.24.12.13]) by hb3-BKT101.hibox.biz with ESMTP; 08 May 2019 04:04:24 +0800 X-Remote-IP: 73.116.1.175 X-Remote-Host: c-73-116-1-175.hsd1.ca.comcast.net X-SBRS: -10.0 X-MID: 23881974 X-Auth-ID: tim.lewis@insyde.com X-EnvelopeFrom: tim.lewis@insyde.com hiBox-Sender: 1 Received: from c-73-116-1-175.hsd1.ca.comcast.net (HELO TIMSLAPTOP) ([73.116.1.175]) by hb3-IN03.hibox.biz with ESMTP/TLS/AES256-SHA; 08 May 2019 04:04:23 +0800 From: "Tim Lewis" To: "'Jonathan Watt'" , "'Carsey, Jaben'" , , "'Gao, Zhichao'" , "'Ni, Ray'" Cc: "'Bi, Dandan'" In-Reply-To: <37952ef0-4ca3-25d5-8e77-e5977ff0e687@jwatt.org> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option Date: Tue, 7 May 2019 13:04:20 -0700 Message-ID: <05c501d50510$11140d00$333c2700$@insyde.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQItgveyoF7dUDEOhbclfoTnL2voH6WvGK7w Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us Jonathan -- The bcfg command pre-dates the UEFI shell specification. I know of at leas= t two non-EDK2 implementations, including one maintained by my company, tha= t are implemented to the specification. Server platforms that use the "appl= ication" style boot options can regularly run over 10 options.=20 I believe the better alternative is to add a new option in the specificat= ion and leave the existing syntax for -opt. Thanks, Tim -----Original Message----- From: Jonathan Watt =20 Sent: Tuesday, May 7, 2019 12:06 PM To: Carsey, Jaben ; devel@edk2.groups.io; tim.lewi= s@insyde.com; Gao, Zhichao ; Ni, Ray Cc: Bi, Dandan Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:= Fix '-opt' option I should add, for me personally, once I noticed the inconsistency I change= d my scripts to use the "0x" prefix to avoid this real footgun. I imagine t= hat 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=20 > scripts would need to be both omitting the "0x" prefix and be pass an=20 > option number greater than 9. The fact this very unexpected=20 > inconsistency (which will corrupt the wrong option when those same two= =20 > things are true!) hasn't been reported before would seem to indicate=20 > this combination doesn't really happen/is rare in practice. >=20 > Also, is TianoCore's bcfg the only implementation people are using? If= =20 > there are other implementations, would this bring TianoCore's=20 > implementation into or out of line with them? That may impact whether th= e spec could/should change. >=20 > On 07/05/2019 18:40, Carsey, Jaben wrote: >> It will break existing scripts. Do you have such scripts in your envir= onment dependent on this parameter? >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf=20 >>> Of Tim Lewis >>> Sent: Tuesday, May 07, 2019 9:20 AM >>> To: devel@edk2.groups.io; Carsey, Jaben ;=20 >>> Gao, Zhichao ; Ni, Ray ;=20 >>> 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=20 >>> shell scripts. In order to maintain that compatibility, it may be=20 >>> 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=20 >>> Carsey, Jaben >>> Sent: Tuesday, May 7, 2019 7:36 AM >>> To: Gao, Zhichao ; devel@edk2.groups.io; Ni,=20 >>> 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 ;=20 >>>> jwatt@jwatt.org >>>> Cc: Carsey, Jaben ; Bi, Dandan=20 >>>> >>>> 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=20 >>>> binary data to be associated with the driver or boot option=20 >>>> optional data, or else the quote- delimited data that will be=20 >>>> associated with the driver or boot option optional data. >>>> ... >>>> >>>> This description lack the comment of '#' parameter and that may=20 >>>> make the consumer confused. Usually consumers would regard it as=20 >>>> the same in other option, such as ' bcfg driver|boot [rm #]'. The=20 >>>> '#' is clearly descripted as a hexadecimal parameter: >>>> rm >>>> Remove an option. The # parameter lists the option number to remove= =20 >>>> 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= =20 >>>>> Of >>>> Ni, >>>>> Ray >>>>> Sent: Monday, May 6, 2019 10:02 PM >>>>> To: jwatt@jwatt.org; devel@edk2.groups.io >>>>> Cc: Carsey, Jaben ; Bi, Dandan=20 >>>>> >>>>> 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=20 >>>>>> >>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt= ' >>>>>> option >>>>>> >>>>>> From: Jonathan Watt >>>>>> >>>>>> For all other bcfg commands the "#" (option number) argument(s)=20 >>>>>> are treated as hexedecimal values regardless of whether or not=20 >>>>>> 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=20 >>>>>> that has been using a number without a "0x" prefix with other=20 >>>>>> bcfg commands finds that, on using that exact same number with=20 >>>>>> '-opt', it has this time unexpectedly been interpreted as a=20 >>>>>> decimal number and they have modified >>>>>> (corrupted) an unrelated load option. For example, a user may=20 >>>>>> have been specifying "10" to other commands to have them act on=20 >>>>>> the 16th option (because simply "10", without any prefix, is how=20 >>>>>> 'bcfg boot dump' displayed the option number for the 16th option). >>>>>> Unfortunately for them, if they also use '-opt' with "10" it=20 >>>>>> 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 =3D ShellConvertStringToUint64(Walker, &Intermediate,=20 >>>>>> FALSE, TRUE); >>>>>> + Status =3D ShellConvertStringToUint64(Walker, &Intermediate,=20 >>>>>> + TRUE, TRUE); >>>>>> if (EFI_ERROR(Status) || (((UINT16)Intermediate) !=3D >>>>>> Intermediate) >>>>>> || StrStr(Walker, L" ") =3D=3D NULL || ((UINT16)Intermediate) > >>>>>> ((UINT16)OrderCount)) { >>>>>> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN=20 >>>>>> (STR_GEN_PARAM_INV), gShellBcfgHiiHandle, L"bcfg", L"Option >>> Index"); >>>>>> ShellStatus =3D SHELL_INVALID_PARAMETER; >>>>>> -- >>>>>> 2.21.0 >>>>> >>>>> >>>>> >>> >>> >>> >>> >>> >>> >>>=20 >> >> >=20