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 9B8D1D800F6 for ; Mon, 12 Feb 2024 19:16:57 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=MA+0dWPVHAE/bcLGl4fKzrfvZdBv1YEPEP8VEDoPFTA=; c=relaxed/simple; d=groups.io; h=Date:From:To:CC:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1707765416; v=1; b=r9HTsSpWOd32ShMWQVt/sXsV+GtwXMFqxgofiPwuLWxhlvxAikoQg/PvHP/H4tAR8STB4Zyy EvsnoZHKlYPAGyqwLTueVakH1uxZY3TodetsBZWpQFRaPK4mBXntwLE0anNWCpdWX/KjAJm3zUj 0CuVt2wyrAQpeyNCAg84jQaI= X-Received: by 127.0.0.2 with SMTP id 12StYY7687511xzp7nqUIuL9; Mon, 12 Feb 2024 11:16:56 -0800 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web10.15430.1707765415295425118 for ; Mon, 12 Feb 2024 11:16:55 -0800 X-Received: from pps.filterd (m0279869.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.24/8.17.1.24) with ESMTP id 41CBD4k9013299; Mon, 12 Feb 2024 19:16:51 GMT X-Received: from nasanppmta01.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3w62q2v7nx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Feb 2024 19:16:50 +0000 (GMT) X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 41CJGnc2002724 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Feb 2024 19:16:49 GMT X-Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.40; Mon, 12 Feb 2024 11:16:47 -0800 Date: Mon, 12 Feb 2024 19:16:44 +0000 From: "Leif Lindholm" To: "Douglas Flick [MSFT]" CC: , Doug Flick , Saloni Kasbekar , Zachary Clark-williams , Andrew Fish , Michael D Kinney Subject: Re: [edk2-devel] [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup Message-ID: References: <25e7367bd09b43a4f4d9e084ad5019dcd1f28446.1707534069.git.doug.edk2@gmail.com> MIME-Version: 1.0 In-Reply-To: <25e7367bd09b43a4f4d9e084ad5019dcd1f28446.1707534069.git.doug.edk2@gmail.com> X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-ORIG-GUID: mF3D7kQMbdkHcJRsdQB2Xt5wZkrssMi5 X-Proofpoint-GUID: mF3D7kQMbdkHcJRsdQB2Xt5wZkrssMi5 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,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: en3lME7gBLKYuSV49L5OJH4vx7686176AA= Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=r9HTsSpW; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none) On Fri, Feb 09, 2024 at 19:04:57 -0800, Douglas Flick [MSFT] wrote: > From: Doug Flick > > Removes duplicate check after merge > > > > > // > > // Verify the PacketCursor is within the packet > > // > > if ( (*PacketCursor < Packet->Dhcp6.Option) > > || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - > sizeof (EFI_DHCP6_HEADER)))) > > { > > return EFI_INVALID_PARAMETER; > > } > > > > Additionally Calculates the Packet->Length before appending If this was one of the packages I maintain, I would ask you to break any "Additionally" out as a separate commit. > Cc: Saloni Kasbekar > Cc: Zachary Clark-williams > > Cc: Andrew Fish > Cc: Leif Lindholm > Cc: Michael D Kinney > > Signed-off-by: Doug Flick [MSFT] > --- > NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c > index 705c665c519d..348602c4e1a8 100644 > --- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c > +++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c > @@ -657,15 +657,6 @@ Dhcp6AppendOption ( > return EFI_BUFFER_TOO_SMALL; > } > > - // > - // Verify the PacketCursor is within the packet > - // > - if ( (*PacketCursor < Packet->Dhcp6.Option) > - || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - sizeof (EFI_DHCP6_HEADER)))) > - { > - return EFI_INVALID_PARAMETER; > - } > - I see this exact pattern repeated identically 4 times in this file. After this deletion. If we're cleaning this up, someone should look into making a macro or static helper function. > WriteUnaligned16 ((UINT16 *)*PacketCursor, OptType); > *PacketCursor += DHCP6_SIZE_OF_OPT_CODE; > WriteUnaligned16 ((UINT16 *)*PacketCursor, OptLen); > @@ -929,6 +920,11 @@ Dhcp6AppendIaOption ( > *PacketCursor += sizeof (T2); > } > > + // > + // Update the packet length > + // > + Packet->Length += BytesNeeded; > + That's not just cleanup though, this is a bugfix in its own right? As in, even if the code is currently laid out in a way that doesn't trigger an issue, Dhcp6AppendIaAddrOption () depends on this option being up to date when called. Or am I overreacting? / Leif > // > // Fill all the addresses belong to the Ia > // > @@ -945,11 +941,6 @@ Dhcp6AppendIaOption ( > // > *Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2)); > > - // > - // Update the packet length > - // > - Packet->Length += BytesNeeded; > - > return EFI_SUCCESS; > } > > @@ -957,6 +948,7 @@ Dhcp6AppendIaOption ( > Append the appointed Elapsed time option to Buf, and move Buf to the end. > > @param[in, out] Packet A pointer to the packet, on success Packet->Length > + will be updated. > @param[in, out] PacketCursor The pointer in the packet, on success PacketCursor > will be moved to the end of the option. > @param[in] Instance The pointer to the Dhcp6 instance. > -- > 2.43.0 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115371): https://edk2.groups.io/g/devel/message/115371 Mute This Topic: https://groups.io/mt/104272127/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-