[PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

[PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

[Jeff Westfahl]

The format specifier for the LoadOptions field of the LoadedImage protocol
is "%s". However, the data in LoadOptions is often generic binary data. A
format specifier of "%x" is more appropriate for this field.

Using "dh -v" with format specifier "%s" on BIOS images based on EDK II
source before commit 891d844 can cause a crash.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
index 0d51627..273a420 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
@@ -354,7 +354,7 @@
                                                   "     DeviceHandle..: %%H%x%%N\r\n"
                                                   "     FilePath......: %%H%x%%N\r\n"
                                                   "     OptionsSize...: %%H%x%%N\r\n"
-                                                  "     LoadOptions...: %%H%s%%N\r\n"
+                                                  "     LoadOptions...: %%H%x%%N\r\n"
                                                   "     ImageBase.....: %%H%x%%N\r\n"
                                                   "     ImageSize.....: %%H%Lx%%N\r\n"
                                                   "     CodeType......: %%H%s%%N\r\n"
-- 
2.7.4

Re: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

[Carsey, Jaben]

Does the print call need to be updated to print this out properly?

-Jaben

> -----Original Message-----
> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> Sent: Tuesday, March 14, 2017 2:02 PM
> To: edk2-devel@lists.01.org
> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Carsey, Jaben <jaben.carsey@intel.com>
> Subject: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for
> LoadedImage
> Importance: High
> 
> The format specifier for the LoadOptions field of the LoadedImage protocol
> is "%s". However, the data in LoadOptions is often generic binary data. A
> format specifier of "%x" is more appropriate for this field.
> 
> Using "dh -v" with format specifier "%s" on BIOS images based on EDK II
> source before commit 891d844 can cause a crash.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> ---
>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> index 0d51627..273a420 100644
> --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> @@ -354,7 +354,7 @@
>                                                    "     DeviceHandle..: %%H%x%%N\r\n"
>                                                    "     FilePath......: %%H%x%%N\r\n"
>                                                    "     OptionsSize...: %%H%x%%N\r\n"
> -                                                  "     LoadOptions...: %%H%s%%N\r\n"
> +                                                  "     LoadOptions...: %%H%x%%N\r\n"
>                                                    "     ImageBase.....: %%H%x%%N\r\n"
>                                                    "     ImageSize.....: %%H%Lx%%N\r\n"
>                                                    "     CodeType......: %%H%s%%N\r\n"
> --
> 2.7.4

Re: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

[Jeff Westfahl]

Jaben,

I think the output looks good with the udpated format. All of the output 
values are aligned, and it prints the hex address of the load options, 
just like it prints the hex address of the image address right below.

Regards,
Jeff

On Wed, 15 Mar 2017, Carsey, Jaben wrote:

> Does the print call need to be updated to print this out properly?
>
> -Jaben
>
>> -----Original Message-----
>> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
>> Sent: Tuesday, March 14, 2017 2:02 PM
>> To: edk2-devel@lists.01.org
>> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
>> Carsey, Jaben <jaben.carsey@intel.com>
>> Subject: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for
>> LoadedImage
>> Importance: High
>>
>> The format specifier for the LoadOptions field of the LoadedImage protocol
>> is "%s". However, the data in LoadOptions is often generic binary data. A
>> format specifier of "%x" is more appropriate for this field.
>>
>> Using "dh -v" with format specifier "%s" on BIOS images based on EDK II
>> source before commit 891d844 can cause a crash.
>>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
>> ---
>>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
>> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
>> index 0d51627..273a420 100644
>> --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
>> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
>> @@ -354,7 +354,7 @@
>>                                                    "     DeviceHandle..: %%H%x%%N\r\n"
>>                                                    "     FilePath......: %%H%x%%N\r\n"
>>                                                    "     OptionsSize...: %%H%x%%N\r\n"
>> -                                                  "     LoadOptions...: %%H%s%%N\r\n"
>> +                                                  "     LoadOptions...: %%H%x%%N\r\n"
>>                                                    "     ImageBase.....: %%H%x%%N\r\n"
>>                                                    "     ImageSize.....: %%H%Lx%%N\r\n"
>>                                                    "     CodeType......: %%H%s%%N\r\n"
>> --
>> 2.7.4
>
>

Re: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

[Carsey, Jaben]

I was unsure if printing the hex address of the load options was useful (useful enough?).

I know for some images it was nice to get the load options printed as a string since they are command line parameters. I guess that might be more focused on learning about the shell itself and how it was launched than other images.  Hence my question.

I am good with this patch.

Ray can you review and push it if you agree?

-Jaben

> -----Original Message-----
> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> Sent: Wednesday, March 15, 2017 2:09 PM
> To: Carsey, Jaben <jaben.carsey@intel.com>
> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; edk2-devel@lists.01.org; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: RE: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier
> for LoadedImage
> Importance: High
> 
> Jaben,
> 
> I think the output looks good with the udpated format. All of the output
> values are aligned, and it prints the hex address of the load options,
> just like it prints the hex address of the image address right below.
> 
> Regards,
> Jeff
> 
> On Wed, 15 Mar 2017, Carsey, Jaben wrote:
> 
> > Does the print call need to be updated to print this out properly?
> >
> > -Jaben
> >
> >> -----Original Message-----
> >> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> >> Sent: Tuesday, March 14, 2017 2:02 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>;
> >> Carsey, Jaben <jaben.carsey@intel.com>
> >> Subject: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier
> for
> >> LoadedImage
> >> Importance: High
> >>
> >> The format specifier for the LoadOptions field of the LoadedImage
> protocol
> >> is "%s". However, the data in LoadOptions is often generic binary data. A
> >> format specifier of "%x" is more appropriate for this field.
> >>
> >> Using "dh -v" with format specifier "%s" on BIOS images based on EDK II
> >> source before commit 891d844 can cause a crash.
> >>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Jaben Carsey <jaben.carsey@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> >> ---
> >>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git
> a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> index 0d51627..273a420 100644
> >> --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> @@ -354,7 +354,7 @@
> >>                                                    "     DeviceHandle..: %%H%x%%N\r\n"
> >>                                                    "     FilePath......: %%H%x%%N\r\n"
> >>                                                    "     OptionsSize...: %%H%x%%N\r\n"
> >> -                                                  "     LoadOptions...: %%H%s%%N\r\n"
> >> +                                                  "     LoadOptions...: %%H%x%%N\r\n"
> >>                                                    "     ImageBase.....: %%H%x%%N\r\n"
> >>                                                    "     ImageSize.....: %%H%Lx%%N\r\n"
> >>                                                    "     CodeType......: %%H%s%%N\r\n"
> >> --
> >> 2.7.4
> >
> >

Re: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

[Ni, Ruiyu]

I will do that.

Thanks/Ray

> -----Original Message-----
> From: Carsey, Jaben
> Sent: Thursday, March 16, 2017 6:28 AM
> To: Jeff Westfahl <jeff.westfahl@ni.com>
> Cc: edk2-devel@lists.01.org; Ni, Ruiyu <ruiyu.ni@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>
> Subject: RE: [PATCH v2] ShellPkg/HandleParsingLib: Correct format specifier
> for LoadedImage
> 
> I was unsure if printing the hex address of the load options was useful
> (useful enough?).
> 
> I know for some images it was nice to get the load options printed as a string
> since they are command line parameters. I guess that might be more focused
> on learning about the shell itself and how it was launched than other images.
> Hence my question.
> 
> I am good with this patch.
> 
> Ray can you review and push it if you agree?
> 
> -Jaben
> 
> > -----Original Message-----
> > From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> > Sent: Wednesday, March 15, 2017 2:09 PM
> > To: Carsey, Jaben <jaben.carsey@intel.com>
> > Cc: Jeff Westfahl <jeff.westfahl@ni.com>; edk2-devel@lists.01.org; Ni,
> > Ruiyu <ruiyu.ni@intel.com>
> > Subject: RE: [PATCH v2] ShellPkg/HandleParsingLib: Correct format
> > specifier for LoadedImage
> > Importance: High
> >
> > Jaben,
> >
> > I think the output looks good with the udpated format. All of the
> > output values are aligned, and it prints the hex address of the load
> > options, just like it prints the hex address of the image address right below.
> >
> > Regards,
> > Jeff
> >
> > On Wed, 15 Mar 2017, Carsey, Jaben wrote:
> >
> > > Does the print call need to be updated to print this out properly?
> > >
> > > -Jaben
> > >
> > >> -----Original Message-----
> > >> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> > >> Sent: Tuesday, March 14, 2017 2:02 PM
> > >> To: edk2-devel@lists.01.org
> > >> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu
> > <ruiyu.ni@intel.com>;
> > >> Carsey, Jaben <jaben.carsey@intel.com>
> > >> Subject: [PATCH v2] ShellPkg/HandleParsingLib: Correct format
> > >> specifier
> > for
> > >> LoadedImage
> > >> Importance: High
> > >>
> > >> The format specifier for the LoadOptions field of the LoadedImage
> > protocol
> > >> is "%s". However, the data in LoadOptions is often generic binary
> > >> data. A format specifier of "%x" is more appropriate for this field.
> > >>
> > >> Using "dh -v" with format specifier "%s" on BIOS images based on
> > >> EDK II source before commit 891d844 can cause a crash.
> > >>
> > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > >> Cc: Jaben Carsey <jaben.carsey@intel.com>
> > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > >> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> > >> ---
> > >>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2
> > >> +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git
> > a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> > >> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> > >> index 0d51627..273a420 100644
> > >> ---
> > >> a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> > >> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.un
> > >> +++ i
> > >> @@ -354,7 +354,7 @@
> > >>                                                    "     DeviceHandle..: %%H%x%%N\r\n"
> > >>                                                    "     FilePath......: %%H%x%%N\r\n"
> > >>                                                    "     OptionsSize...: %%H%x%%N\r\n"
> > >> -                                                  "     LoadOptions...: %%H%s%%N\r\n"
> > >> +                                                  "     LoadOptions...: %%H%x%%N\r\n"
> > >>                                                    "     ImageBase.....: %%H%x%%N\r\n"
> > >>                                                    "     ImageSize.....: %%H%Lx%%N\r\n"
> > >>                                                    "     CodeType......: %%H%s%%N\r\n"
> > >> --
> > >> 2.7.4
> > >
> > >