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 9D10ED8027F for ; Thu, 20 Jul 2023 16:42:19 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=9JEmvFEmpVZwOn3IXuDGnVAwD2OirgTaeWkH61fCRfk=; c=relaxed/simple; d=groups.io; h=X-Received:X-Received:X-Received:X-Received:X-Received:X-Gm-Message-State:X-Google-Smtp-Source:X-Received:MIME-Version:References:In-Reply-To:From:Date:X-Gmail-Original-Message-ID:Message-ID:Subject:To:Cc:Precedence:List-Unsubscribe:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:Content-Type; s=20140610; t=1689871338; v=1; b=k7yvOdoYQbUGDRzXk1q+EzusyzY8mV0XlnAWILzVPVR1BTzw9ICT8KCegszqYQ2lHiq1V8dl eWoBPTMfOyObVVxggGONSxvqt1kl0mKfo8UkJRCiOQDjB2yWMY/c6nh9z8s/oLGn/O5VVMmbsjH zm0lMe4BqzWdjGjfTiqy//eU= X-Received: by 127.0.0.2 with SMTP id Jz4vYY7687511xTrLpbApNu8; Thu, 20 Jul 2023 09:42:18 -0700 X-Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.2064.1689871337633561930 for ; Thu, 20 Jul 2023 09:42:17 -0700 X-Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id ED6A161B75 for ; Thu, 20 Jul 2023 16:42:16 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5AB9EC433CD for ; Thu, 20 Jul 2023 16:42:16 +0000 (UTC) X-Received: by mail-lf1-f42.google.com with SMTP id 2adb3069b0e04-4fbc0314a7bso1559661e87.2 for ; Thu, 20 Jul 2023 09:42:16 -0700 (PDT) X-Gm-Message-State: IR3DrvWaop4DAzS58q6PNFPox7686176AA= X-Google-Smtp-Source: APBJJlG1P0TLjjoV0QAzhcL8S7X2uzaL7RFtcPme/zpwH+yVjyP3288ZLxj6WiPUBYQPppF3O1ljm4I40ma4LcKhZP0= X-Received: by 2002:a05:6512:e89:b0:4fb:cc99:4e90 with SMTP id bi9-20020a0565120e8900b004fbcc994e90mr2851964lfb.37.1689871334307; Thu, 20 Jul 2023 09:42:14 -0700 (PDT) MIME-Version: 1.0 References: <174276A45CE2816B.5513@groups.io> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 20 Jul 2023 18:42:03 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver binding start issue. To: devel@edk2.groups.io, michael.d.kinney@intel.com, Leif Lindholm , "Demeter, Miki" Cc: "Kasbekar, Saloni" , Nickle Wang , "Clark-williams, Zachary" , Abner Chang , Igor Kulchytskyy , Nick Ramirez Precedence: Bulk List-Unsubscribe: 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,ardb@kernel.org Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=k7yvOdoY; 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=kernel.org (policy=none) On Thu, 20 Jul 2023 at 03:41, Michael D Kinney wrote: > > Merged: https://github.com/tianocore/edk2/pull/4666 > This commit is now merged as commit 6510dcf6f71adbe282bff0ba2b236f1d074f819f Author: 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 > > Sent: Wednesday, July 19, 2023 5:27 PM > > To: Kasbekar, Saloni ; Nickle Wang > > ; devel@edk2.groups.io; Clark-williams, Zachary > > > > Cc: Abner Chang ; Igor Kulchytskyy ; > > Nick Ramirez ; Kinney, Michael D > > > > Subject: RE: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix driver > > binding start issue. > > > > Acked-by: Michael D Kinney > > > > > -----Original Message----- > > > From: Kasbekar, Saloni > > > Sent: Thursday, July 13, 2023 9:13 AM > > > To: Nickle Wang ; devel@edk2.groups.io; Clark- > > > williams, Zachary ; Kinney, Michael > > D > > > > > > Cc: Abner Chang ; Igor Kulchytskyy > > ; > > > Nick Ramirez > > > 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 > > > Sent: Wednesday, July 12, 2023 11:54 PM > > > To: devel@edk2.groups.io; Kasbekar, Saloni > > ; > > > Clark-williams, Zachary > > > Cc: Abner Chang ; Igor Kulchytskyy > > ; > > > Nick Ramirez > > > 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 On Behalf Of > > Saloni > > > > Kasbekar via groups.io > > > > Sent: Friday, June 30, 2023 6:28 AM > > > > To: Nickle Wang ; devel@edk2.groups.io > > > > Cc: Maciej Rabeda ; Siyuan Fu > > > > ; Abner Chang ; Igor > > > > Kulchytskyy ; Nick Ramirez > > > > 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 > > > > > > > > Thanks, > > > > Saloni > > > > > > > > -----Original Message----- > > > > From: Nickle Wang > > > > Sent: Wednesday, June 28, 2023 3:30 PM > > > > To: Kasbekar, Saloni ; > > devel@edk2.groups.io > > > > Cc: Maciej Rabeda ; Siyuan Fu > > > > ; Abner Chang ; Igor > > > > Kulchytskyy ; Nick Ramirez > > > > 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 > > > > > Sent: Thursday, June 29, 2023 3:07 AM > > > > > To: devel@edk2.groups.io; Nickle Wang > > > > > Cc: Maciej Rabeda ; Siyuan Fu > > > > > ; Abner Chang ; Igor > > > > > Kulchytskyy ; Nick Ramirez > > > > > 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 On Behalf Of > > > > > Nickle Wang via groups.io > > > > > Sent: Tuesday, June 27, 2023 5:56 PM > > > > > To: devel@edk2.groups.io; Nickle Wang > > > > > Cc: Maciej Rabeda ; Siyuan Fu > > > > > ; Abner Chang ; Igor > > > > > Kulchytskyy ; Nick Ramirez > > > > > 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 On Behalf Of > > > > > > Nickle Wang via groups.io > > > > > > Sent: Friday, February 10, 2023 8:34 PM > > > > > > To: devel@edk2.groups.io > > > > > > Cc: Maciej Rabeda ; Siyuan Fu > > > > > > ; Abner Chang ; Igor > > > > > > Kulchytskyy ; Nick Ramirez > > > > > > 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 > > > > > > Cc: Maciej Rabeda > > > > > > Cc: Siyuan Fu > > > > > > Cc: Abner Chang > > > > > > Cc: Igor Kulchytskyy > > > > > > Cc: Nick Ramirez > > > > > > --- > > > > > > 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.
> > > > > > (C) Copyright 2016 Hewlett Packard Enterprise Development > > > > > > LP
> > > > > > + 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 (#107106): https://edk2.groups.io/g/devel/message/107106 Mute This Topic: https://groups.io/mt/99821789/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-