[edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

Tim Lewis tim.lewis at insyde.com
Tue May 7 20:48:48 UTC 2019 

Jim --

Well, speaking of shooting-oneself-in-the-foot, it turns out that our non-EDK2 implementation followed the recommendation in the patch. 

I agree that the spec is ambiguous and, it turns out that our largest use case already uses the recommended behavior.

Sorry to one and all.

Tim

-----Original Message-----
From: Jim.Dailey at dell.com <Jim.Dailey at dell.com> 
Sent: Tuesday, May 7, 2019 1:30 PM
To: devel at edk2.groups.io; tim.lewis at insyde.com
Cc: dandan.bi at intel.com; jwatt at jwatt.org; jaben.carsey at intel.com; zhichao.gao at intel.com; ray.ni at intel.com
Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

Tim,

Out of curiosity, what does the specification you refer to that was used to write the non-EDK2 implementations say about the -opt switch?

Regards,
Jim

-----Original Message-----
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Tim Lewis
Sent: Tuesday, May 7, 2019 3:04 PM
To: 'Jonathan Watt'; 'Carsey, Jaben'; devel at edk2.groups.io; 'Gao, Zhichao'; 'Ni, Ray'
Cc: 'Bi, Dandan'
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

Jonathan --

The bcfg command pre-dates the UEFI shell specification. I know of at least two non-EDK2 implementations, including one maintained by my company, that are implemented to the specification. Server platforms that use the "application" style boot options can regularly run over 10 options. 

I believe the better  alternative is to add a new option in the specification and leave the existing syntax for -opt.

Thanks,

Tim

-----Original Message-----
From: Jonathan Watt <jwatt at jwatt.org>
Sent: Tuesday, May 7, 2019 12:06 PM
To: Carsey, Jaben <jaben.carsey at intel.com>; devel at edk2.groups.io; tim.lewis at insyde.com; Gao, Zhichao <zhichao.gao at intel.com>; Ni, Ray <ray.ni at intel.com>
Cc: Bi, Dandan <dandan.bi at intel.com>
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 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf 
>>> Of Tim Lewis
>>> Sent: Tuesday, May 07, 2019 9:20 AM
>>> To: devel at edk2.groups.io; Carsey, Jaben <jaben.carsey at intel.com>; 
>>> Gao, Zhichao <zhichao.gao at intel.com>; Ni, Ray <ray.ni at intel.com>; 
>>> jwatt at jwatt.org
>>> Cc: Bi, Dandan <dandan.bi at intel.com>
>>> 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of 
>>> Carsey, Jaben
>>> Sent: Tuesday, May 7, 2019 7:36 AM
>>> To: Gao, Zhichao <zhichao.gao at intel.com>; devel at edk2.groups.io; Ni, 
>>> Ray <ray.ni at intel.com>; jwatt at jwatt.org
>>> Cc: Bi, Dandan <dandan.bi at intel.com>
>>> 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 <jaben.carsey at intel.com>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Gao, Zhichao
>>>> Sent: Tuesday, May 07, 2019 2:52 AM
>>>> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; 
>>>> jwatt at jwatt.org
>>>> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Bi, Dandan 
>>>> <dandan.bi at intel.com>
>>>> 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 <zhichao.gao at intel.com>
>>>>
>>>> But when I view the command in UEFI SHELL 2.2 spec:
>>>> ...
>>>> bcfg driver|boot [-opt # [[filename]|["data"]] | [KeyData <ScanCode
>>>> 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf 
>>>>> Of
>>>> Ni,
>>>>> Ray
>>>>> Sent: Monday, May 6, 2019 10:02 PM
>>>>> To: jwatt at jwatt.org; devel at edk2.groups.io
>>>>> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Bi, Dandan 
>>>>> <dandan.bi at intel.com>
>>>>> 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 at jwatt.org [mailto:jwatt at jwatt.org]
>>>>>> Sent: Monday, May 6, 2019 9:03 PM
>>>>>> To: devel at edk2.groups.io
>>>>>> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Ni, Ray 
>>>>>> <ray.ni at intel.com>
>>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt'
>>>>>> option
>>>>>>
>>>>>> From: Jonathan Watt <jwatt at jwatt.org>
>>>>>>
>>>>>> 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 <jaben.carsey at intel.com>
>>>>>> CC: Ray Ni <ray.ni at intel.com>
>>>>>> Signed-off-by: Jonathan Watt <jwatt at jwatt.org>
>>>>>> ---
>>>>>>
>>>>>>
>>> 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
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> 
>>
>>
> 







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#40136): https://edk2.groups.io/g/devel/message/40136
Mute This Topic: https://groups.io/mt/31520134/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

More information about the edk2-devel-archive mailing list