From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web11.12773.1601640141032504140 for ; Fri, 02 Oct 2020 05:02:21 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.115, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: p1hTn8LPG2wEGDKXWDyKO3ouXAAoJtfkpmxMFdPvtipSrfEP1iu03i38hxQI7CaTIh6OsyzI4s 3wwkAPpYtFsQ== X-IronPort-AV: E=McAfee;i="6000,8403,9761"; a="162207427" X-IronPort-AV: E=Sophos;i="5.77,327,1596524400"; d="scan'208";a="162207427" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Oct 2020 05:02:15 -0700 IronPort-SDR: KCS8R03CfdZGVVJ3ANWAOla4UbNNpzrLRrJfa4Q3efGQ5CB7WYCRzJ7QbG2JyzobQ6gb8yBKFD EoGD5u0zHY7Q== X-IronPort-AV: E=Sophos;i="5.77,327,1596524400"; d="scan'208";a="508285099" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.30.53]) ([10.213.30.53]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Oct 2020 05:02:12 -0700 Subject: Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser To: Vladimir Olovyannikov , devel@edk2.groups.io Cc: Jiaxin Wu , Siyuan Fu , Laszlo Ersek References: <20200828181706.25296-1-vladimir.olovyannikov@broadcom.com> <2d7b8b14f01cc630017e3e1134f17585@mail.gmail.com> <4b4d9ed6f95926f5029beb97fbf8f47a@mail.gmail.com> From: "Maciej Rabeda" Message-ID: <4f0f875d-865e-cc4b-ae21-21a422b4b0b9@linux.intel.com> Date: Fri, 2 Oct 2020 14:02:07 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <4b4d9ed6f95926f5029beb97fbf8f47a@mail.gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl Hi Vladimir, Functionally the patch is fine. However, from coding standard perspective, !PortionLen is not allowed - such structure is used only for BOOLEAN type values. Reference: Table 10, https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/57_c_programming#5-7-2-2-a-comparison-of-any-pointer-to-zero-must-be-done-via-the-null-type Do not submit v2, I will correct that upon merging. On terms CS issue is addressed, I am giving: Reviewed-by: Maciej Rabeda Thanks, Maciej On 01-Oct-20 17:25, Vladimir Olovyannikov wrote: > Hi Maciej, > > Thank you for looking into this. > > Vladimir >> -----Original Message----- >> From: Rabeda, Maciej >> Sent: Wednesday, September 30, 2020 2:57 AM >> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com >> Cc: Jiaxin Wu ; Siyuan Fu ; >> Laszlo Ersek >> Subject: Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite >> loop >> in HTTP msg body parser >> >> Hi Vladimir, >> >> Yes, this must have go past my radar, sorry. Things are becoming more and >> more busy out here :/ I will take a look at it by the end of week. >> >> On 24-Sep-20 23:57, Vladimir Olovyannikov via groups.io wrote: >>> Hi Maciej, >>> >>> Can you please review this patch? >>> It is sitting there for a while, looks like it slipped through the >>> cracks. >>> >>> Thank you, >>> Vladimir >>>> -----Original Message----- >>>> From: Vladimir Olovyannikov >>>> Sent: Friday, August 28, 2020 11:17 AM >>>> To: devel@edk2.groups.io >>>> Cc: Vladimir Olovyannikov ; >>>> Maciej Rabeda ; Jiaxin Wu >>>> ; Siyuan Fu >>>> Subject: [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP >>>> msg >>> body >>>> parser >>>> >>>> When an HTTP server sends a non-chunked body data with no Content- >>>> Length header, the HttpParserMessageBody in DxeHttpLib gets confused >>>> and never sets the Char pointer beyond the body start. >>>> This causes "for" loop to never break because the condition of "Char >>>>> = >>> Body >>>> + BodyLength" is never satisfied. >>>> Use BodyLength as the ContentLength for the parser when >> ContentLength >>>> is absent in HTTP response headers. >>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2941 >>>> >>>> Signed-off-by: Vladimir Olovyannikov >>>> >>>> Cc: Maciej Rabeda >>>> Cc: Jiaxin Wu >>>> Cc: Siyuan Fu >>>> --- >>>> NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 19 ++++++++++++++++- >> -- >>>> 1 file changed, 16 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>> b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>> index 180d9321025a..e550c9962dc1 100644 >>>> --- a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>> +++ b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>> @@ -1122,6 +1122,7 @@ HttpParseMessageBody ( >>>> CHAR8 *Char; >>>> UINTN RemainderLengthInThis; >>>> UINTN LengthForCallback; >>>> + UINTN PortionLength; >>>> EFI_STATUS Status; >>>> HTTP_BODY_PARSER *Parser; >>>> >>>> @@ -1173,19 +1174,31 @@ HttpParseMessageBody ( >>>> // >>>> // Identity transfer-coding, just notify user to save the >>>> body >>> data. >>>> // >>>> + PortionLength = MIN ( >>>> + BodyLength, >>>> + Parser->ContentLength - >>> Parser->ParsedBodyLength >>>> + ); >>>> + if (!PortionLength) { >>>> + // >>>> + // Got BodyLength, but no ContentLength. Use BodyLength. >>>> + // >>>> + PortionLength = BodyLength; >>>> + Parser->ContentLength = PortionLength; >>>> + } >>>> + >>>> if (Parser->Callback != NULL) { >>>> Status = Parser->Callback ( >>>> BodyParseEventOnData, >>>> Char, >>>> - MIN (BodyLength, Parser->ContentLength - >>> Parser- >>>>> ParsedBodyLength), >>>> + PortionLength, >>>> Parser->Context >>>> ); >>>> if (EFI_ERROR (Status)) { >>>> return Status; >>>> } >>>> } >>>> - Char += MIN (BodyLength, Parser->ContentLength - Parser- >>>>> ParsedBodyLength); >>>> - Parser->ParsedBodyLength += MIN (BodyLength, Parser- >>>>> ContentLength - Parser->ParsedBodyLength); >>>> + Char += PortionLength; >>>> + Parser->ParsedBodyLength += PortionLength; >>>> if (Parser->ParsedBodyLength == Parser->ContentLength) { >>>> Parser->State = BodyParserComplete; >>>> if (Parser->Callback != NULL) { >>>> -- >>>> 2.26.2.266.ge870325ee8 >>> >>> >>>