Re: [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue

["Laszlo Ersek" <lersek@redhat.com>]

On 10/9/23 13:28, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function USBMouseDriverBindingStart do have
> 
>     ASSERT (UsbMouseDevice != NULL);
> 
> after AllocateZeroPool, but it is applicable only in DEBUG mode.
> In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
> at this point, the code proceeds to dereference "UsbMouseDevice"
> afterwards which will lead to CRASH.
> 
> Hence, for safety add NULL pointer checks always.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
> index 451d4b934f4c..621d09713b24 100644
> --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
> +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
> @@ -161,6 +161,10 @@ USBMouseDriverBindingStart (
>  
>    UsbMouseDevice = AllocateZeroPool (sizeof (USB_MOUSE_DEV));
>    ASSERT (UsbMouseDevice != NULL);
> +  if (UsbMouseDevice == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto ErrorExit;
> +  }
>  
>    UsbMouseDevice->UsbIo     = UsbIo;
>    UsbMouseDevice->Signature = USB_MOUSE_DEV_SIGNATURE;

With this (good) fix, it's better to remove the ASSERT() altogether, in
my opinion! The assert was a cop-out, and now you are replacing it with
proper error checking (and cleanup, too), so there's no need for the
ASSERT(). It's not an "invariant" that AllocateZeroPool must succeed, so
whenever it fails, it's not an invariant violation (programming error).

With the ASSERT() dropped:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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