Re: [edk2-devel] [PATCH v2 13/14] RedfishDiscoverDxe: handle memory allocation error conditions.

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

Hi Abner,

On Mon, Dec 18, 2023 at 7:47 AM Chang, Abner <Abner.Chang@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> > -----Original Message-----
> > From: Mike Maslenkin <mike.maslenkin@gmail.com>
> > Sent: Friday, December 15, 2023 7:25 AM
> > To: devel@edk2.groups.io
> > Cc: Chang, Abner <Abner.Chang@amd.com>; nicklew@nvidia.com;
> > igork@ami.com; Mike Maslenkin <mike.maslenkin@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.chang@amd.com>
> > Cc: Nickle Wang <nicklew@nvidia.com>
> > Cc: Igor Kulchytskyy <igork@ami.com>
> > Signed-off-by: Mike Maslenkin <mike.maslenkin@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.
I would suggest to change the order of these conditions.

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?
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/RedfishDiscoverDxe/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 (#112664): https://edk2.groups.io/g/devel/message/112664
Mute This Topic: https://groups.io/mt/103181050/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-