[AMD Official Use Only - General]
> -----Original Message-----
> From: Mike Maslenkin <mike.maslen...@gmail.com>
> Sent: Tuesday, December 19, 2023 10:12 AM
> To: Chang, Abner <abner.ch...@amd.com>
> Cc: devel@edk2.groups.io; nick...@nvidia.com; ig...@ami.com
> Subject: Re: [PATCH v2 13/14] RedfishDiscoverDxe: handle memory allocation
> error conditions.
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Abner,
>
> On Mon, Dec 18, 2023 at 7:47 AM Chang, Abner <abner.ch...@amd.com>
> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > > -----Original Message-----
> > > From: Mike Maslenkin <mike.maslen...@gmail.com>
> > > Sent: Friday, December 15, 2023 7:25 AM
> > > To: devel@edk2.groups.io
> > > Cc: Chang, Abner <abner.ch...@amd.com>; nick...@nvidia.com;
> > > ig...@ami.com; Mike Maslenkin <mike.maslen...@gmail.com>
> > > Subject: [PATCH v2 13/14] RedfishDiscoverDxe: handle memory allocation
> > > error conditions.
> > >
> > > Caution: This message originated from an External Source. Use proper
> caution
> > > when opening attachments, clicking links, or responding.
> > >
> > >
> > > Cc: Abner Chang <abner.ch...@amd.com>
> > > Cc: Nickle Wang <nick...@nvidia.com>
> > > Cc: Igor Kulchytskyy <ig...@ami.com>
> > > Signed-off-by: Mike Maslenkin <mike.maslen...@gmail.com>
> > > ---
> > > .../RedfishDiscoverDxe/RedfishDiscoverDxe.c | 85 ++++++++++++++++--
> -
> > > 1 file changed, 75 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > index 3499a855570c..9d1678c3429e 100644
> > > --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> > > @@ -748,38 +748,103 @@ InitInformationData (
> > > if (RedfishLocation != NULL) {
> > >
> > > AllocationSize = AsciiStrSize (RedfishLocation) * sizeof
> > > (CHAR16);
> > >
> > > Information->Location = AllocatePool (AllocationSize);
> > >
> > > - AsciiStrToUnicodeStrS (RedfishLocation, Information->Location,
> > > AllocationSize);
> > >
> > > - DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n",
> > > Information->Location));
> > >
> > > + if (Information->Location != NULL) {
> > >
> > > + AsciiStrToUnicodeStrS (RedfishLocation, Information->Location,
> > > AllocationSize);
> > >
> > > + DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n",
> > > Information->Location));
> > >
> > > + } else {
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_ERROR,
> > >
> > > + "%a: Can not allocate memory for Redfish service location:
> > > %a.\n",
> > >
> > > + __func__,
> > >
> > > + RedfishLocation
> > >
> > > + ));
> > >
> > > + }
> > >
> > > }
> > >
> > >
> > >
> > > if (Uuid != NULL) {
> > >
> > > AllocationSize = AsciiStrSize (Uuid) * sizeof (CHAR16);
> > >
> > > Information->Uuid = AllocatePool (AllocationSize);
> > >
> > > - AsciiStrToUnicodeStrS (Uuid, Information->Uuid, AllocationSize);
> > >
> > > - DEBUG ((DEBUG_MANAGEABILITY, "Service UUID: %s.\n", Information-
> > > >Uuid));
> > >
> > > + if (Information->Uuid != NULL) {
> > >
> > > + AsciiStrToUnicodeStrS (Uuid, Information->Uuid, AllocationSize);
> > >
> > > + DEBUG ((DEBUG_MANAGEABILITY, "Service UUID: %s.\n",
> Information-
> > > >Uuid));
> > >
> > > + } else {
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_ERROR,
> > >
> > > + "%a: Can not allocate memory for Service UUID: %a.\n",
> > >
> > > + __func__,
> > >
> > > + Uuid
> > >
> > > + ));
> > >
> > > + }
> > >
> > > }
> > >
> > >
> > >
> > > if (Os != NULL) {
> > >
> > > AllocationSize = AsciiStrSize (Os) * sizeof (CHAR16);
> > >
> > > Information->Os = AllocatePool (AllocationSize);
> > >
> > > - AsciiStrToUnicodeStrS (Os, Information->Os, AllocationSize);
> > >
> > > + if (Information->Os != NULL) {
> > >
> > > + AsciiStrToUnicodeStrS (Os, Information->Os, AllocationSize);
> > >
> > > + } else {
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_ERROR,
> > >
> > > + "%a: Can not allocate memory for Redfish service OS: %a.\n",
> > >
> > > + __func__,
> > >
> > > + Os
> > >
> > > + ));
> > >
> > > + }
> > >
> > > }
> > >
> > >
> > >
> > > if (OsVer != NULL) {
> > >
> > > AllocationSize = AsciiStrSize (OsVer) * sizeof (CHAR16);
> > >
> > > Information->OsVersion = AllocatePool (AllocationSize);
> > >
> > > - AsciiStrToUnicodeStrS (OsVer, Information->OsVersion,
> > > AllocationSize);
> > >
> > > - DEBUG ((DEBUG_MANAGEABILITY, "Redfish service OS: %s,
> Version:%s.\n",
> > > Information->Os, Information->OsVersion));
> > >
> > > + if (Information->OsVersion != NULL) {
> > >
> > > + AsciiStrToUnicodeStrS (OsVer, Information->OsVersion,
> AllocationSize);
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_MANAGEABILITY,
> > >
> > > + "Redfish service OS: %s, Version:%s.\n",
> >
> > Jus a note here: We expect the debug message will be <null string> for the
> case when Information->Os is NULL.
>
> Not sure I understood right, but there is no problem with this at this time.
> But I see this patch is bad.
> 1. Did original code print OsVer correctly at all?
>
> There were two conditions:
>
> if (Os != NULL) {
> DiscoveredInstance->Information.Os = (CHAR16 *)AllocatePool
> (AsciiStrSize ((const CHAR8 *)Os) * sizeof (CHAR16));
> AsciiStrToUnicodeStrS ((const CHAR8 *)Os,
> DiscoveredInstance->Information.Os, AsciiStrSize ((const CHAR8 *)Os) *
> sizeof (CHAR16));
> DEBUG ((DEBUG_MANAGEABILITY, "Redfish service OS: %s,
> Version:%s.\n", DiscoveredInstance->Information.Os,
> DiscoveredInstance->Information.OsVersion));
> }
>
> if (OsVer != NULL) {
> DiscoveredInstance->Information.OsVersion = (CHAR16
> *)AllocatePool (AsciiStrSize ((const CHAR8 *)OsVer) * sizeof
> (CHAR16));
> AsciiStrToUnicodeStrS ((const CHAR8 *)OsVer,
> DiscoveredInstance->Information.OsVersion, AsciiStrSize ((const CHAR8
> *)OsVer) * sizeof (CHAR16));
> }
>
> So, in scope of the first condition
> DiscoveredInstance->Information.OsVersion could be NULL, unless it was
> a second call for "InfoRefresh".
> But anyway, it's not clear why on info refresh Os matches to the old OsVer.
Ok, here is the history when I was implemented this code with Redfish discovery
on both Redfish host interface and SSDP in HPE lab. Many Redfish services were
discovered and one of them is also indicated by Redfish host interface. We have
an upper layer EFI Redfish application to discover Redfish via both
methodologies. However, we didn’t upstream SSDP code to opensource. I think
InfoRefresh was not removed back to that moment.
> I would suggest to change the order of these conditions.
I think the code for Product and ProductInfo is not good also. We can also
make it similar to Os and OsVer.
>
> 2. From said above, this patch doesn't take into account (as well as
> original code) the case of "InfoRefresh" when DiscoveredInstance
> wasn't allocated but taken from a some list and all string resources
> could be allocated previously. It seems these resource must be freed
> before allocating it again for refreshed values.
> Hm... not a great to have smth like the following for every string:
> if (OsVer != NULL) {
> AllocationSize = AsciiStrSize (OsVer) * sizeof (CHAR16);
> if (DiscoveredInstance->Information.OsVersion != NULL) {
> FreePool (DiscoveredInstance->Information.OsVersion);
> }
> DiscoveredInstance->Information.OsVersion = AllocatePool
> (AllocationSize);
> if (DiscoveredInstance->Information.OsVersion != NULL) {
> AsciiStrToUnicodeStrS (OsVer,
> DiscoveredInstance->Information.OsVersion, AllocationSize);
> } else {
> DEBUG ((
> DEBUG_ERROR,
> "%a: Can not allocate memory for Redfish OS Version:%a.\n",
> __func__,
> OsVer
> ));
> }
> }
>
> Can I assume that on info refresh it is possible to deallocate all
> strings stored into this Information structure?
I think here we have two ways, however I prefer the latter.
1. Remove InfoRefresh, but we have to revisit the call if one day someone
contribute SSDP. I believe I can’t upstream that code because legal issue as we
didn't opensource SSDP code when I was with HPE.
2. Deallocate memory when InfoRefresh.
I would leave this to you, I am fine if you choose #1.
Thanks
Abner
> In this case a simple helper function deallocating strings could be
> implemented and then also used here:
> https://github.com/tianocore/edk2/blob/master/RedfishPkg/RedfishDiscove
> rDxe/RedfishDiscoverDxe.c#L1469
>
> >
> > >
> > > + Information->Os,
> > >
> > > + Information->OsVersion
> > >
> > > + ));
> > >
> > > + } else {
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_ERROR,
> > >
> > > + "%a: Can not allocate memory for Redfish OS Version:%a.\n",
> > >
> > > + __func__,
> > >
> > > + OsVer
> > >
> > > + ));
> > >
> > > + }
> > >
> > > }
> > >
> > >
> > >
> > > if ((Product != NULL) && (ProductVer != NULL)) {
> >
> > Not for your change, but could you please help to remove this check? I think
> we can have a consistent code as Os/OsVer, just print out the product
> information separately.
> >
>
> Ok.
>
> >
> > Thanks
> > Abner
> >
> >
> > >
> > > AllocationSize = AsciiStrSize (Product) * sizeof (CHAR16);
> > >
> > > Information->Product = AllocatePool (AllocationSize);
> > >
> > > - AsciiStrToUnicodeStrS (Product, Information->Product,
> > > AllocationSize);
> > >
> > > + if (Information->Product != NULL) {
> > >
> > > + AsciiStrToUnicodeStrS (Product, Information->Product,
> AllocationSize);
> > >
> > > + } else {
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_ERROR,
> > >
> > > + "%a: Can not allocate memory for Redfish service product: %a.\n",
> > >
> > > + __func__,
> > >
> > > + Product
> > >
> > > + ));
> > >
> > > + }
> > >
> > > +
> > >
> > > AllocationSize = AsciiStrSize (ProductVer) * sizeof
> > > (CHAR16);
> > >
> > > Information->ProductVer = AllocatePool (AllocationSize);
> > >
> > > - AsciiStrToUnicodeStrS (ProductVer, Information->ProductVer,
> > > AllocationSize);
> > >
> > > - DEBUG ((DEBUG_MANAGEABILITY, "Redfish service product: %s,
> > > Version:%s.\n", Information->Product, Information->ProductVer));
> > >
> > > + if (Information->ProductVer != NULL) {
> > >
> > > + AsciiStrToUnicodeStrS (ProductVer, Information->ProductVer,
> > > AllocationSize);
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_MANAGEABILITY,
> > >
> > > + "Redfish service product: %s, Version:%s.\n",
> > >
> > > + Information->Product,
> > >
> > > + Information->ProductVer
> > >
> > > + ));
> > >
> > > + } else {
> > >
> > > + DEBUG ((
> > >
> > > + DEBUG_ERROR,
> > >
> > > + "%a: Can not allocate memory for Redfish service product Version:
> > > %a.\n",
> > >
> > > + __func__,
> > >
> > > + ProductVer
> > >
> > > + ));
> > >
> > > + }
> > >
> > > }
> > >
> > > }
> > >
> > >
> > >
> > > --
> > > 2.32.0 (Apple Git-132)
> >
>
> As you requested, I have created a bug for this issue:
> https://bugzilla.tianocore.org/show_bug.cgi?id=4625
>
>
> Regards,
> Mike
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112684): https://edk2.groups.io/g/devel/message/112684
Mute This Topic: https://groups.io/mt/103181050/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
