> If this was one of the packages I maintain, I would ask
> you to break any "Additionally" out as a separate commit.

Thats fair enough. I can go ahead and do that.

> 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.

That's a good suggestion

> 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?

Ultimately, I think you're right. This issue was originally pointed on the original mail thread but it was merged in before it could be corrected. Should I create a bugzilla for this and then attach that to the change?

Douglas Flick
________________________________
From: Leif Lindholm <quic_llindhol@quicinc.com>
Sent: Monday, February 12, 2024 11:16:44 AM
To: Douglas Flick [MSFT] <doug.edk2@gmail.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Doug Flick <dougflick@microsoft.com>; Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams <zachary.clark-williams@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [EXTERNAL] Re: [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

[You don't often get email from quic_llindhol@quicinc.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

On Fri, Feb 09, 2024 at 19:04:57 -0800, Douglas Flick [MSFT] wrote:
> From: Doug Flick <dougflick@microsoft.com>
>
> 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 <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
> ---
>  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 (#115372): https://edk2.groups.io/g/devel/message/115372
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]
-=-=-=-=-=-=-=-=-=-=-=-