Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver binding start issue.

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Ard,

This is my mistake.  I usually check the Author field when 
adding Rb/Ab tags because I have seen lots of cases where
the Author field needs updating as well.  I forgot to do this
step in this one.

I agree a CI check against the Author field being a valid
email address and not one of the mailing addresses would be
valuable.  Perhaps in PatchCheck.py?

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Thursday, July 20, 2023 9:42 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> Demeter, Miki <miki.demeter@intel.com>
> Cc: Kasbekar, Saloni <saloni.kasbekar@intel.com>; Nickle Wang
> <nicklew@nvidia.com>; Clark-williams, Zachary <zachary.clark-
> williams@intel.com>; Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
> Subject: Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver
> binding start issue.
> 
> On Thu, 20 Jul 2023 at 03:41, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > Merged: https://github.com/tianocore/edk2/pull/4666
> >
> 
> 
> This commit is now merged as
> 
> commit 6510dcf6f71adbe282bff0ba2b236f1d074f819f
> Author:     devel@edk2.groups.io <devel@edk2.groups.io>
> AuthorDate: Fri Feb 10 04:34:03 2023 -0800
> Commit:     mergify[bot]
> <37929162+mergify[bot]@users.noreply.github.com>
> CommitDate: Thu Jul 20 01:41:02 2023 +0000
> 
> which is a bit unfortunate.
> 
> Given how pedantic our pre-merge CI is, could we perhaps also add a
> check that the author field is sane? I don't think matching it to the
> signoff makes sense in all cases so we could just check that it isn't
> set to this particular value. (I could contribute code that I did not
> author, and my signoff would amount to a statement on my part that
> distributing the code as part of EDK2 complies with the code's license
> terms and those of EDK2 itself) Alternatively, we might have a PR flag
> that allows the maintainer to override this check manually if the
> author field does not match the signoff, and match it strictly to the
> [first] signoff line in other cases.
> 
> As for the committer field: it is quite unfortunate that we do not
> keep track of who merged which PR. Could we do something about that?
> The mergify bot is not a member of the project or a Tianocore
> maintainer so it is not accountable, and its identity should not
> obfuscate the identity of the person who decided to merge the PR.
> 
> 
> 
> 
> 
> > > -----Original Message-----
> > > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Sent: Wednesday, July 19, 2023 5:27 PM
> > > To: Kasbekar, Saloni <saloni.kasbekar@intel.com>; Nickle Wang
> > > <nicklew@nvidia.com>; devel@edk2.groups.io; Clark-williams, Zachary
> > > <zachary.clark-williams@intel.com>
> > > Cc: Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>;
> > > Nick Ramirez <nramirez@nvidia.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver
> > > binding start issue.
> > >
> > > Acked-by: Michael D Kinney <michael.d.kinney@intel.com>
> > >
> > > > -----Original Message-----
> > > > From: Kasbekar, Saloni <saloni.kasbekar@intel.com>
> > > > Sent: Thursday, July 13, 2023 9:13 AM
> > > > To: Nickle Wang <nicklew@nvidia.com>; devel@edk2.groups.io; Clark-
> > > > williams, Zachary <zachary.clark-williams@intel.com>; Kinney,
> Michael
> > > D
> > > > <michael.d.kinney@intel.com>
> > > > Cc: Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy
> > > <igork@ami.com>;
> > > > Nick Ramirez <nramirez@nvidia.com>
> > > > Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> driver
> > > > binding start issue.
> > > >
> > > > Mike,
> > > >
> > > > Would you be able to help us merge the patch?
> > > >
> > > > Thanks,
> > > > Saloni
> > > >
> > > > -----Original Message-----
> > > > From: Nickle Wang <nicklew@nvidia.com>
> > > > Sent: Wednesday, July 12, 2023 11:54 PM
> > > > To: devel@edk2.groups.io; Kasbekar, Saloni
> > > <saloni.kasbekar@intel.com>;
> > > > Clark-williams, Zachary <zachary.clark-williams@intel.com>
> > > > Cc: Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy
> > > <igork@ami.com>;
> > > > Nick Ramirez <nramirez@nvidia.com>
> > > > Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> driver
> > > > binding start issue.
> > > >
> > > > Hi Saloni,
> > > >
> > > > Could you please help to merge this fix since there is no
> objection
> > > > during past weeks?
> > > >
> > > > Thanks,
> > > > Nickle
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Saloni
> > > > > Kasbekar via groups.io
> > > > > Sent: Friday, June 30, 2023 6:28 AM
> > > > > To: Nickle Wang <nicklew@nvidia.com>; devel@edk2.groups.io
> > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> > > > > <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor
> > > > > Kulchytskyy <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
> > > > > Subject: Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> driver
> > > > > binding start issue.
> > > > >
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > Hi Nickle,
> > > > >
> > > > > That makes sense. Thanks for the clarification.
> > > > >
> > > > > Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>
> > > > >
> > > > > Thanks,
> > > > > Saloni
> > > > >
> > > > > -----Original Message-----
> > > > > From: Nickle Wang <nicklew@nvidia.com>
> > > > > Sent: Wednesday, June 28, 2023 3:30 PM
> > > > > To: Kasbekar, Saloni <saloni.kasbekar@intel.com>;
> > > devel@edk2.groups.io
> > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> > > > > <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor
> > > > > Kulchytskyy <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
> > > > > Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> driver
> > > > > binding start issue.
> > > > >
> > > > > Hi Saloni,
> > > > >
> > > > > Thanks for your review.
> > > > >
> > > > > When uninstall fails, per UEFI specification, the protocol will
> be
> > > > > installed again and will be visible to UEFI drivers.
> > > > >
> > > > > Page 190, UEFI spec. 2.10:
> > > > > "If any errors are generated while the protocol interfaces are
> being
> > > > > uninstalled, then the protocols uninstalled prior to the error
> will
> > > be
> > > > > reinstalled with the boot service
> > > > > EFI_BOOT_SERVICES.InstallProtocolInterface() and the status code
> > > > > EFI_INVALID_PARAMETER is returned."
> > > > >
> > > > > In this case, if we do FreePool while driver still can locate
> > > > > gEfiHttpServiceBindingProtocolGuid. Driver will access to the
> memory
> > > > > that is released to system. Memory issue may happen.
> > > > >
> > > > > Regards,
> > > > > Nickle
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Kasbekar, Saloni <saloni.kasbekar@intel.com>
> > > > > > Sent: Thursday, June 29, 2023 3:07 AM
> > > > > > To: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>
> > > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> > > > > > <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor
> > > > > > Kulchytskyy <igork@ami.com>; Nick Ramirez
> <nramirez@nvidia.com>
> > > > > > Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> > > driver
> > > > > > binding start issue.
> > > > > >
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > Hi Nickle,
> > > > > >
> > > > > > We would want to do the FreePool even if the Uninstall fails
> (like
> > > > > > in the case where we failed to install the multiple protocol
> > > > > > interfaces and then went to ON_ERROR). Do you think it's
> better if
> > > > > > we change it to -
> > > > > >
> > > > > >   if (HttpService != NULL) {
> > > > > >     HttpCleanService (HttpService, UsingIpv6);
> > > > > >     Status = gBS->UninstallMultipleProtocolInterfaces (
> > > > > >                     &ControllerHandle,
> > > > > >                     &gEfiHttpServiceBindingProtocolGuid,
> > > > > >                     &HttpService->ServiceBinding,
> > > > > >                     NULL
> > > > > >                     );
> > > > > >     if ((HttpService->Tcp4ChildHandle == NULL) &&
> > > > > > (HttpService->Tcp6ChildHandle == NULL)) {
> > > > > >         FreePool (HttpService);
> > > > > >     }
> > > > > >   }
> > > > > >
> > > > > > Thanks,
> > > > > > Saloni
> > > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > > > > Nickle Wang via groups.io
> > > > > > Sent: Tuesday, June 27, 2023 5:56 PM
> > > > > > To: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>
> > > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> > > > > > <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor
> > > > > > Kulchytskyy <igork@ami.com>; Nick Ramirez
> <nramirez@nvidia.com>
> > > > > > Subject: Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> > > driver
> > > > > > binding start issue.
> > > > > >
> > > > > > May I know if someone can help to review this patch?
> > > > > >
> > > > > > Thanks,
> > > > > > Nickle
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf
> Of
> > > > > > > Nickle Wang via groups.io
> > > > > > > Sent: Friday, February 10, 2023 8:34 PM
> > > > > > > To: devel@edk2.groups.io
> > > > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu
> > > > > > > <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>;
> Igor
> > > > > > > Kulchytskyy <igork@ami.com>; Nick Ramirez
> <nramirez@nvidia.com>
> > > > > > > Subject: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix
> driver
> > > > > > > binding start issue.
> > > > > > >
> > > > > > > External email: Use caution opening links or attachments
> > > > > > >
> > > > > > >
> > > > > > > When failure happens in HttpDxeStart, the error handling
> code
> > > > > > > release the memory buffer but it does not uninstall HTTP
> service
> > > > > > > bindnig protocol. As the result, application can still
> locate
> > > this
> > > > > > > protocol and invoke service binding fucntions in released
> memory
> > > > pool.
> > > > > > >
> > > > > > > Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> > > > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> > > > > > > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > > > > > > Cc: Abner Chang <abner.chang@amd.com>
> > > > > > > Cc: Igor Kulchytskyy <igork@ami.com>
> > > > > > > Cc: Nick Ramirez <nramirez@nvidia.com>
> > > > > > > ---
> > > > > > >  NetworkPkg/HttpDxe/HttpDriver.c | 13 +++++++++++--
> > > > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/NetworkPkg/HttpDxe/HttpDriver.c
> > > > > > > b/NetworkPkg/HttpDxe/HttpDriver.c index
> 5d918d3c4d..f6d1263cad
> > > > > > > 100644
> > > > > > > --- a/NetworkPkg/HttpDxe/HttpDriver.c
> > > > > > > +++ b/NetworkPkg/HttpDxe/HttpDriver.c
> > > > > > > @@ -3,6 +3,7 @@
> > > > > > >
> > > > > > >    Copyright (c) 2015 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > > > >    (C) Copyright 2016 Hewlett Packard Enterprise Development
> > > > > > > LP<BR>
> > > > > > > +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All
> > > rights
> > > > > reserved.
> > > > > > >
> > > > > > >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > > >
> > > > > > > @@ -464,8 +465,16 @@ ON_ERROR:
> > > > > > >
> > > > > > >    if (HttpService != NULL) {
> > > > > > >      HttpCleanService (HttpService, UsingIpv6);
> > > > > > > -    if ((HttpService->Tcp4ChildHandle == NULL) &&
> (HttpService-
> > > > > > > >Tcp6ChildHandle == NULL)) {
> > > > > > > -      FreePool (HttpService);
> > > > > > > +    Status = gBS->UninstallMultipleProtocolInterfaces (
> > > > > > > +                    &ControllerHandle,
> > > > > > > +                    &gEfiHttpServiceBindingProtocolGuid,
> > > > > > > +                    &HttpService->ServiceBinding,
> > > > > > > +                    NULL
> > > > > > > +                    );
> > > > > > > +    if (!EFI_ERROR (Status)) {
> > > > > > > +      if ((HttpService->Tcp4ChildHandle == NULL) &&
> > > (HttpService-
> > > > > > > >Tcp6ChildHandle == NULL)) {
> > > > > > > +        FreePool (HttpService);
> > > > > > > +      }
> > > > > > >      }
> > > > > > >    }
> > > > > > >
> > > > > > > --
> > > > > > > 2.39.1.windows.1
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107107): https://edk2.groups.io/g/devel/message/107107
Mute This Topic: https://groups.io/mt/99821789/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-