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 930A6941952 for ; Fri, 26 Jan 2024 15:43:25 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=7386PREbyysy0b4pCCMi8DlJqnE4QHGsLVsrDnqknuY=; c=relaxed/simple; d=groups.io; h=From:Message-Id:Mime-Version:Subject:Date:In-Reply-To:Cc:To:References:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1706283804; v=1; b=ahKlloUE+VCkPlkvM8GgzdYvsLDCQ+bRSmCHrpChhGl/0/k0sbh3TxSi1wggEADoCy2PZRbM zylGEnYkVm5ZA6Lm+xbHI5W6GTZvQBx8YsSOIEw6qFYBfKz9Kla7uxLgyDM0XGMDiIilUQ4o/gI bKgFB+pdyBLRUj+NRiAJmr2c= X-Received: by 127.0.0.2 with SMTP id ZgLtYY7687511xPiUt4YgouL; Fri, 26 Jan 2024 07:43:24 -0800 X-Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) by mx.groups.io with SMTP id smtpd.web11.908.1706283802959679534 for ; Fri, 26 Jan 2024 07:43:23 -0800 X-Received: by mail-wr1-f51.google.com with SMTP id ffacd0b85a97d-3387ef9fc62so628669f8f.2 for ; Fri, 26 Jan 2024 07:43:22 -0800 (PST) X-Gm-Message-State: NYKLqC4dfjgh5NJ0VBhMs2vqx7686176AA= X-Google-Smtp-Source: AGHT+IHepDWc1sq+W7lWy1pIoHEvwCbF75w6mXarEaCu6Ah6HXUFo72sb/FN4MCVNwDhXNN3/c2kew== X-Received: by 2002:adf:ce09:0:b0:339:35e7:ec2b with SMTP id p9-20020adfce09000000b0033935e7ec2bmr1044726wrn.58.1706283800917; Fri, 26 Jan 2024 07:43:20 -0800 (PST) X-Received: from smtpclient.apple ([195.158.249.22]) by smtp.gmail.com with ESMTPSA id u11-20020a056000038b00b00337d3465997sm1514872wrf.38.2024.01.26.07.43.19 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 26 Jan 2024 07:43:20 -0800 (PST) From: "Mike Maslenkin" Message-Id: <4907CA4D-C19B-4603-9EBF-1A102EFC215F@gmail.com> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\)) Subject: Re: [edk2-devel] [edk2-redfish-client][PATCH V2] RedfishClientPkg/FeatureDriver: ComputerSystem_1_13_0 driver Date: Fri, 26 Jan 2024 18:41:18 +0300 In-Reply-To: Cc: "devel@edk2.groups.io" , Nickle Wang , Igor Kulchytskyy To: "Chang, Abner" References: <20240126010023.958-1-abner.chang@amd.com> <5880033F-2E1E-445E-8B1B-F267A54853A8@gmail.com> <55E3AADE-AC5B-4C86-84CB-31EC4166F92E@gmail.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: multipart/alternative; boundary="Apple-Mail=_7B3B4C8B-C361-4B46-A609-9FC21CD277BA" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ahKlloUE; 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 --Apple-Mail=_7B3B4C8B-C361-4B46-A609-9FC21CD277BA Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 BTW did you consider to remove this Private->Uri someday at all ? I tried to remove it, but since it declared in a common header for all redf= ish client feature drivers those changes was huge comparing to a small memory leak I tried to fix. And now such pattern spreads into new feature drivers.=20 In short: there is no need to cache this value in Private->Uri, but pass it= as an argument down to stack in EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL imp= lementation. I think it just small design flaw.=20 The code I'm talking about: + // + // Check and see if "@Redfish.Settings" exist or not. + // + ZeroMem (&PendingSettingResponse, sizeof (REDFISH_RESPONSE)); + Status =3D GetPendingSettings ( + Private->RedfishService, + Response.Payload, + &PendingSettingResponse, + &PendingSettingUri + ); + if (!EFI_ERROR (Status)) { + DEBUG ((REDFISH_DEBUG_TRACE, "%a: @Redfish.Settings found: %s\n", __fu= nc__, PendingSettingUri)); + Private->Uri =3D PendingSettingUri; + ExpectedResponse =3D &PendingSettingResponse; + } else { + DEBUG ((REDFISH_DEBUG_TRACE, "%a: No @Redfish.Settings is found\n", __= func__)); + Private->Uri =3D Uri; + ExpectedResponse =3D &Response; + } In this pattern PendingSettingUri is leaked, since Private->Uri belongs to = caller and never released. But... in fact it is not true for Feature/Bios driver, and actually leaked = resource is original Private->Uri string. This is because these EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL function calle= d by HandleResource() function, which in turns called from here: // // Initialize collection path // Private->Uri =3D RedfishGetUri (ResourceUri); if (Private->Uri =3D=3D NULL) { ASSERT (FALSE); FreePool (ResourceUri); return EFI_OUT_OF_RESOURCES; } Status =3D HandleResource (Private, Private->Uri); if (EFI_ERROR (Status)) { DEBUG ((DEBUG_ERROR, "%a, process external resource: %a failed: %r\n", = __func__, Private->Uri, Status)); } FreePool (Private->Uri); Too many "Private->Uri" with a different value.=20 PS: Funny, just pay attention to the log trace above - %a used for Private-= >Uri. Probably it was one of the factors=20 induced me to write previous message :) Regards, Mike. > On 26. 1. 2024., at 17:13, Chang, Abner wrote: >=20 > [AMD Official Use Only - General] >=20 > It is no problem. =F0=9F=98=8A Thank you Mike for looking into this patch= . > Abner >=20 >> -----Original Message----- >> From: M M >> Sent: Friday, January 26, 2024 10:11 PM >> To: Chang, Abner >> Cc: devel@edk2.groups.io; Nickle Wang ; Igor >> Kulchytskyy >> Subject: Re: [edk2-devel] [edk2-redfish-client][PATCH V2] >> RedfishClientPkg/FeatureDriver: ComputerSystem_1_13_0 driver >>=20 >> Caution: This message originated from an External Source. Use proper cau= tion >> when opening attachments, clicking links, or responding. >>=20 >>=20 >> You are right. >>=20 >> Sorry for the noise! >>=20 >> No problems here. >> I mixed up with tags in my editor while looked into patch. >>=20 >> Regards, >> Mike >>=20 >>> On 26. 1. 2024., at 17:03, Chang, Abner wrote: >>>=20 >>> [AMD Official Use Only - General] >>>=20 >>> Hi Mike, >>> I can't identify the issue on %s as Private->Uri is defined as EFI_STRI= NG, or I >> miss something? >>>=20 >>> Thanks >>> Abner >>>=20 >>>> -----Original Message----- >>>> From: M M >>>> Sent: Friday, January 26, 2024 9:55 PM >>>> To: devel@edk2.groups.io; Nickle Wang >>>> Cc: Chang, Abner ; Igor Kulchytskyy >>>> >>>> Subject: Re: [edk2-devel] [edk2-redfish-client][PATCH V2] >>>> RedfishClientPkg/FeatureDriver: ComputerSystem_1_13_0 driver >>>>=20 >>>> Caution: This message originated from an External Source. Use proper >> caution >>>> when opening attachments, clicking links, or responding. >>>>=20 >>>>=20 >>>> Hi Abner, >>>>=20 >>>>> On 26. 1. 2024., at 05:20, Nickle Wang via groups.io >>>> wrote: >>>>>=20 >>>>> Hi Abner, >>>>>=20 >>>>> Same minor issue as 1_5_0. Please add "%a:" to below DEBUG call. >>>>>=20 >>>>>> + DEBUG ((DEBUG_MANAGEABILITY, " No platform Redfish >>>> ConfigureLang >>>>>> found for %s\n", __func__, Private->Uri)); >>>>>=20 >>>>> Regards, >>>>> Nickle >>>>=20 >>>> There are two issues in this string. The second one is incorrect %s f= ormat >> used >>>> for Private->Uri. >>>> This issue exists in V3 not only in the line mentioned above. >>>>=20 >>>> Regards, >>>> Mike. >=20 -=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 (#114618): https://edk2.groups.io/g/devel/message/114618 Mute This Topic: https://groups.io/mt/103967446/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- --Apple-Mail=_7B3B4C8B-C361-4B46-A609-9FC21CD277BA Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
BTW did you consider to remove this Private->Uri someday at all = ?
I t= ried to remove it, but since it declared in a common header for all redfish= client feature drivers
those changes was huge comparing to a small memory leak = I tried to fix.
And now such pattern spreads into new feature drivers. 
In short= : there is no need to cache this value in Private->Uri, but pass it as a= n argument down to stack in EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL implemen= tation.
I think it just small design flaw. 


The code I'm talking about:
+  //
+  // Check and see if "@R= edfish.Settings" exist or not.
+  //
+  ZeroMem (&PendingSettingRespons= e, sizeof (REDFISH_RESPONSE));
+  Status =3D GetPendingSettings (
+    = ;         Private->RedfishService,
+     &nbs= p;       Response.Payload,
+           &= nbsp; &PendingSettingResponse,
+             &= amp;PendingSettingUri
+             );
+  if (!EFI= _ERROR (Status)) {
+    DEBUG ((REDFISH_DEBUG_TRACE, "%a: @Redfish.Set= tings found: %s\n", __func__, PendingSettingUri));
+    Private->Ur= i     =3D PendingSettingUri;
+    ExpectedResponse =3D &= PendingSettingResponse;
+  } else {
+    DEBUG ((REDFISH_DEBUG_TRACE, = "%a: No @Redfish.Settings is found\n", __func__));
+    Private->Ur= i     =3D Uri;
+    ExpectedResponse =3D &Response;
+  }=


In this pattern Pendi= ngSettingUri is leaked, since Private->Uri belongs to caller and never r= eleased.

But... in fact it is = not true for Feature/Bios driver, and actually leaked resource is original = Private->Uri string.

=
This is because these EDKII_REDFISH_RESOURCE_CONFIG_PROTOCOL functio= n called by
HandleResource() function, which in turns called fr= om here:
  //
  // Initialize collection = path
  //
  Private->Uri =3D RedfishGe= tUri (ResourceUri);
  if (Private->Uri =3D=3D NULL) {
    ASSERT (FALSE);
    FreePool= (ResourceUri);
    return EFI_OUT_OF_RESOURCES;
  }

=   Status =3D HandleResource (Private, Private->Uri);
&n= bsp; if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERRO= R, "%a, process external resource: %a failed: %r\n", __func__, Private->= Uri, Status));
  }

  FreePool (Private->Uri);

<= /b>

Too many "Private->Uri" with a different va= lue. 

PS: Fu= nny, just pay attention to the log trace above - %a used for Private->Ur= i. Probably it was one of the factors 
induced me= to write previous message :)

Regards,
Mike.


On 26. 1. 2024., at 17:13, Chang, Abner <Abner.Chang@amd.com> wrote:
[AMD = Official Use Only - General]

It is no problem.= =F0=9F=98=8A Thank you Mike for looking into this patch.
Abn= er

-----O= riginal Message-----
From: M M <mike.maslenkin@gmail.com>
Sent: Friday, January 26, 2024 10:11 PM
To: Chang, Abner <= ;Abner.Chang@amd.com&= gt;
Cc: de= vel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>; Igor
Kulchytskyy &l= t;igork@ami.com>
Subject: Re: [edk2-devel] [edk2-redfish-client][PATCH V2]
RedfishClientPkg/FeatureDriver: ComputerSystem_1_13_0 driver

Caution: This message originated from an External Sour= ce. Use proper caution
when opening attachments, clicking lin= ks, or responding.


You are righ= t.

Sorry for the noise!

No problems here.
I mixed up with tags in my editor wh= ile looked into patch.

Regards,
= Mike

On 2= 6. 1. 2024., at 17:03, Chang, Abner <Abner.Chang@amd.com> wrote:

[AMD Official Use Only - General]

Hi Mik= e,
I can't identify the issue on %s as Private->Uri is def= ined as EFI_STRING, or I
miss something?

Thanks
Abner

-----Original Message-----
From: M M <mike.maslenkin@gmail.com>
Sent: Friday, January 26, 2024 9:55 PM
To: devel@edk2.groups.io; Nickl= e Wang <nicklew@nvidia.= com>
Cc: Chang, Abner <Abner.Chang@amd.com>; Igor Kulchytskyy
<igork@ami.com>= ;
Subject: Re: [edk2-devel] [edk2-redfish-client][PATCH V2]RedfishClientPkg/FeatureDriver: ComputerSystem_1_13_0 driver
Caution: This message originated from an Extern= al Source. Use proper
caution
when opening attachments, clicking links, or responding.


Hi Abner,

=
On 26. 1. 2024., at 05:20, Nickle Wang= via groups.io
<nicklew=3Dnvidia.com@groups.io> wrote:

Hi Abner,

Same minor issue as 1_5_0. Please add "%a:" to below DEBUG call.

+   &nb= sp;  DEBUG ((DEBUG_MANAGEABILITY, "  No platform Redfish
ConfigureLang
found for %= s\n", __func__, Private->Uri));

Regards,
Nickle

T= here are two issues in this string. The second one is incorrect  %s fo= rmat
used
for Private-= >Uri.
This issue exists in V3 not only in the line mention= ed above.

Regards,
Mike.


_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#114618) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--Apple-Mail=_7B3B4C8B-C361-4B46-A609-9FC21CD277BA--