Re: [edk2-devel] [PATCH 19/35] NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces calls

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Laszlo,

On 9/17/19 9:49 PM, Laszlo Ersek wrote:
> Both the "ControllerHandle" parameter of CloseProtocol()

Maybe worth adding "of type EFI_CLOSE_PROTOCOL"

> and the "Handle"
> parameter of UninstallMultipleProtocolInterfaces() 

"of type EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES"

have type EFI_HANDLE,
> not (EFI_HANDLE*).
> 
> This patch fixes actual bugs. The issues have been dormant likely because
> they are on error paths. (Or, in case of TlsAuthConfigDxe, because the
> driver is unloaded likely very infrequently.)
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     build-tested only
> 
>  NetworkPkg/DnsDxe/DnsDriver.c                  | 4 ++--
>  NetworkPkg/IScsiDxe/IScsiConfig.c              | 2 +-
>  NetworkPkg/Ip4Dxe/Ip4Driver.c                  | 2 +-
>  NetworkPkg/Ip6Dxe/Ip6Driver.c                  | 2 +-
>  NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c            | 2 +-
>  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +-
>  6 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/NetworkPkg/DnsDxe/DnsDriver.c b/NetworkPkg/DnsDxe/DnsDriver.c
> index 94d072159a4d..ad007da8b7d6 100644
> --- a/NetworkPkg/DnsDxe/DnsDriver.c
> +++ b/NetworkPkg/DnsDxe/DnsDriver.c
> @@ -1145,7 +1145,7 @@ Dns4ServiceBindingCreateChild (
>             DnsSb->ConnectUdp->UdpHandle,
>             &gEfiUdp4ProtocolGuid,
>             gDns4DriverBinding.DriverBindingHandle,
> -           ChildHandle
> +           *ChildHandle

EFI_CLOSE_PROTOCOL, OK.

>             );
>  
>       gBS->UninstallMultipleProtocolInterfaces (
> @@ -1388,7 +1388,7 @@ Dns6ServiceBindingCreateChild (
>             DnsSb->ConnectUdp->UdpHandle,
>             &gEfiUdp6ProtocolGuid,
>             gDns6DriverBinding.DriverBindingHandle,
> -           ChildHandle
> +           *ChildHandle

EFI_CLOSE_PROTOCOL, OK.

>             );
>  
>       gBS->UninstallMultipleProtocolInterfaces (
> diff --git a/NetworkPkg/IScsiDxe/IScsiConfig.c b/NetworkPkg/IScsiDxe/IScsiConfig.c
> index b876da7f5ccd..d773849fd3b0 100644
> --- a/NetworkPkg/IScsiDxe/IScsiConfig.c
> +++ b/NetworkPkg/IScsiDxe/IScsiConfig.c
> @@ -3852,7 +3852,7 @@ IScsiConfigFormInit (
>                                       );
>    if (CallbackInfo->RegisteredHandle == NULL) {
>      gBS->UninstallMultipleProtocolInterfaces (
> -           &CallbackInfo->DriverHandle,
> +           CallbackInfo->DriverHandle,

EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.

>             &gEfiDevicePathProtocolGuid,
>             &mIScsiHiiVendorDevicePath,
>             &gEfiHiiConfigAccessProtocolGuid,
> diff --git a/NetworkPkg/Ip4Dxe/Ip4Driver.c b/NetworkPkg/Ip4Dxe/Ip4Driver.c
> index ebd4dec1dfe4..62be8b681a18 100644
> --- a/NetworkPkg/Ip4Dxe/Ip4Driver.c
> +++ b/NetworkPkg/Ip4Dxe/Ip4Driver.c
> @@ -891,7 +891,7 @@ Ip4ServiceBindingCreateChild (
>                    );
>    if (EFI_ERROR (Status)) {
>      gBS->UninstallMultipleProtocolInterfaces (
> -           ChildHandle,
> +           *ChildHandle,

EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.

>             &gEfiIp4ProtocolGuid,
>             &IpInstance->Ip4Proto,
>             NULL
> diff --git a/NetworkPkg/Ip6Dxe/Ip6Driver.c b/NetworkPkg/Ip6Dxe/Ip6Driver.c
> index 7dc9e45af7b6..63d8428dbced 100644
> --- a/NetworkPkg/Ip6Dxe/Ip6Driver.c
> +++ b/NetworkPkg/Ip6Dxe/Ip6Driver.c
> @@ -888,7 +888,7 @@ Ip6ServiceBindingCreateChild (
>                    );
>    if (EFI_ERROR (Status)) {
>      gBS->UninstallMultipleProtocolInterfaces (
> -           ChildHandle,
> +           *ChildHandle,

EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.

>             &gEfiIp6ProtocolGuid,
>             &IpInstance->Ip6Proto,
>             NULL
> diff --git a/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c b/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
> index ae9e65544a86..06c4e202d3ef 100644
> --- a/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
> +++ b/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
> @@ -592,7 +592,7 @@ Mtftp4ServiceBindingCreateChild (
>             MtftpSb->ConnectUdp->UdpHandle,
>             &gEfiUdp4ProtocolGuid,
>             gMtftp4DriverBinding.DriverBindingHandle,
> -           ChildHandle
> +           *ChildHandle

EFI_CLOSE_PROTOCOL, OK.

>             );
>      goto ON_ERROR;
>    }
> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c
> index 18ee763002b4..c0870ab9979c 100644
> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c
> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c
> @@ -39,7 +39,7 @@ TlsAuthConfigDxeUnload (
>    ASSERT (PrivateData->Signature == TLS_AUTH_CONFIG_PRIVATE_DATA_SIGNATURE);
>  
>    gBS->UninstallMultipleProtocolInterfaces (
> -         &ImageHandle,
> +         ImageHandle,

EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.

>           &gEfiCallerIdGuid,
>           PrivateData,
>           NULL
> 

I'd have split this patch in 2 for easier review (one fixing
EFI_CLOSE_PROTOCOL, another fixing
EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES).

As it or split:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Regards,

Phil.