From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id AE577740034 for ; Tue, 19 Dec 2023 02:13:01 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=MEzxD337Us/tEY4d6tS51YDpmMxW0ERpLoj2EFrdtA0=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1702951980; v=1; b=NHWo2bAtYxZuB/Z4VeX3Th4f+HYg+Lv4HYWh1mVcfbHtBwXdjX39YyFo5GZ5vCNa7E+A9fHD VyNb8YdH+jhFDJN6TNu8/3HyMfOQFLv0zzCtYqrSAtj+u3Jy9SlYaXW3FWNFYJ13cSYNkMXIu+V 8lV6uj0xyds83yzqSI+dDKKs= X-Received: by 127.0.0.2 with SMTP id lf10YY7687511xg5CB9IgZty; Mon, 18 Dec 2023 18:13:00 -0800 X-Received: from mail-yb1-f170.google.com (mail-yb1-f170.google.com [209.85.219.170]) by mx.groups.io with SMTP id smtpd.web10.2317.1702951979447776713 for ; Mon, 18 Dec 2023 18:12:59 -0800 X-Received: by mail-yb1-f170.google.com with SMTP id 3f1490d57ef6-dbd46eabce4so809354276.0 for ; Mon, 18 Dec 2023 18:12:59 -0800 (PST) X-Gm-Message-State: uNZ1HnAWWS0Ntpc8BekdHg0Kx7686176AA= X-Google-Smtp-Source: AGHT+IEaWzSZcWgoh1lYo4Ixi2P02BQYTQVM5ghIMI2Ghl3tQmsiWAZEf092soi7CPxUy53STg5iFkc42N1m9U/6sgU= X-Received: by 2002:a25:b8f:0:b0:dbc:dfa4:b96f with SMTP id 137-20020a250b8f000000b00dbcdfa4b96fmr227661ybl.46.1702951978505; Mon, 18 Dec 2023 18:12:58 -0800 (PST) MIME-Version: 1.0 References: <20231214232458.4636-1-mike.maslenkin@gmail.com> <20231214232458.4636-14-mike.maslenkin@gmail.com> In-Reply-To: From: "Mike Maslenkin" Date: Tue, 19 Dec 2023 05:12:22 +0300 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 13/14] RedfishDiscoverDxe: handle memory allocation error conditions. To: "Chang, Abner" Cc: "devel@edk2.groups.io" , "nicklew@nvidia.com" , "igork@ami.com" Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,mike.maslenkin@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=NHWo2bAt; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io Hi Abner, On Mon, Dec 18, 2023 at 7:47=E2=80=AFAM Chang, Abner = wrote: > > [AMD Official Use Only - General] > > > -----Original Message----- > > From: Mike Maslenkin > > Sent: Friday, December 15, 2023 7:25 AM > > To: devel@edk2.groups.io > > Cc: Chang, Abner ; nicklew@nvidia.com; > > igork@ami.com; Mike Maslenkin > > Subject: [PATCH v2 13/14] RedfishDiscoverDxe: handle memory allocation > > error conditions. > > > > Caution: This message originated from an External Source. Use proper ca= ution > > when opening attachments, clicking links, or responding. > > > > > > Cc: Abner Chang > > Cc: Nickle Wang > > Cc: Igor Kulchytskyy > > Signed-off-by: Mike Maslenkin > > --- > > .../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 !=3D NULL) { > > > > AllocationSize =3D AsciiStrSize (RedfishLocation) * sizeof = (CHAR16); > > > > Information->Location =3D AllocatePool (AllocationSize); > > > > - AsciiStrToUnicodeStrS (RedfishLocation, Information->Location, > > AllocationSize); > > > > - DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n", > > Information->Location)); > > > > + if (Information->Location !=3D 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 !=3D NULL) { > > > > AllocationSize =3D AsciiStrSize (Uuid) * sizeof (CHAR16); > > > > Information->Uuid =3D AllocatePool (AllocationSize); > > > > - AsciiStrToUnicodeStrS (Uuid, Information->Uuid, AllocationSize); > > > > - DEBUG ((DEBUG_MANAGEABILITY, "Service UUID: %s.\n", Information- > > >Uuid)); > > > > + if (Information->Uuid !=3D 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 !=3D NULL) { > > > > AllocationSize =3D AsciiStrSize (Os) * sizeof (CHAR16); > > > > Information->Os =3D AllocatePool (AllocationSize); > > > > - AsciiStrToUnicodeStrS (Os, Information->Os, AllocationSize); > > > > + if (Information->Os !=3D 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 !=3D NULL) { > > > > AllocationSize =3D AsciiStrSize (OsVer) * sizeof (CHAR16); > > > > Information->OsVersion =3D AllocatePool (AllocationSize); > > > > - AsciiStrToUnicodeStrS (OsVer, Information->OsVersion, AllocationSi= ze); > > > > - DEBUG ((DEBUG_MANAGEABILITY, "Redfish service OS: %s, Version:%s.\= n", > > Information->Os, Information->OsVersion)); > > > > + if (Information->OsVersion !=3D NULL) { > > > > + AsciiStrToUnicodeStrS (OsVer, Information->OsVersion, Allocation= Size); > > > > + DEBUG (( > > > > + DEBUG_MANAGEABILITY, > > > > + "Redfish service OS: %s, Version:%s.\n", > > Jus a note here: We expect the debug message will be for t= he 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 !=3D NULL) { DiscoveredInstance->Information.Os =3D (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 !=3D NULL) { DiscoveredInstance->Information.OsVersion =3D (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 !=3D NULL) { AllocationSize =3D AsciiStrSize (OsVer) * sizeof (CHAR16); if (DiscoveredInstance->Information.OsVersion !=3D NULL) { FreePool (DiscoveredInstance->Information.OsVersion); } DiscoveredInstance->Information.OsVersion =3D AllocatePool (Allocatio= nSize); if (DiscoveredInstance->Information.OsVersion !=3D 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 !=3D NULL) && (ProductVer !=3D NULL)) { > > Not for your change, but could you please help to remove this check? I th= ink we can have a consistent code as Os/OsVer, just print out the product i= nformation separately. > Ok. > > Thanks > Abner > > > > > > AllocationSize =3D AsciiStrSize (Product) * sizeof (CHAR16); > > > > Information->Product =3D AllocatePool (AllocationSize); > > > > - AsciiStrToUnicodeStrS (Product, Information->Product, AllocationSi= ze); > > > > + if (Information->Product !=3D NULL) { > > > > + AsciiStrToUnicodeStrS (Product, Information->Product, Allocation= Size); > > > > + } else { > > > > + DEBUG (( > > > > + DEBUG_ERROR, > > > > + "%a: Can not allocate memory for Redfish service product: %a.\= n", > > > > + __func__, > > > > + Product > > > > + )); > > > > + } > > > > + > > > > AllocationSize =3D AsciiStrSize (ProductVer) * sizeof (CH= AR16); > > > > Information->ProductVer =3D AllocatePool (AllocationSize); > > > > - AsciiStrToUnicodeStrS (ProductVer, Information->ProductVer, > > AllocationSize); > > > > - DEBUG ((DEBUG_MANAGEABILITY, "Redfish service product: %s, > > Version:%s.\n", Information->Product, Information->ProductVer)); > > > > + if (Information->ProductVer !=3D 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 Versi= on: > > %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=3D4625 Regards, Mike -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-