fix possible freeing null pointer and error status return

[alex-kalin]

If BlockIo not supported by current file system, null pointer will be freed in DriverBindingStart function.

If device_error ocured in disk read, udf driver perform access to null pointer, because grub_udf_read_icb after call grub_disk_read return grub_errno.

[pbatard]

I'll just point out that just like free(NULL) is perfectly valid, FreePool(NULL) is valid too. Obviously, functions that can (and should) do nothing if a NULL pointer is passed to them have long been designed to do so.

So your first patch is superfluous.

On the other hand, your second one is indeed very much needed.

It looks like had wrongly assumed that calls to grub_disk_read() were always issued like:

if (grub_disk_read(...) != 0) {
    // Handle error
    ...
}

But in fact, in most of the driver source I see, they are instead issue like:

grub_disk_read(...);
if (grub_errno) {
    // Handle error
    ...
}

So not setting grub_errno through grub_error() is indeed an issue.

I'll apply your patch when I get a chance. Thanks a lot for figuring this issue!

[alex-kalin]

Yes, you are right, free(null) is possible, but on some old motherboards bios print "assert" on the screen or in a serial port. This is mistake of bios developer, but it is so annoying.

[pbatard]

but on some old motherboards bios print "assert" on the screen or in a serial port.

Hmm, I'm looking at the FreePool() implementation from the EDK2.

Ultimately, FreePool() calls CoreInternalFreePool() (in MdeModulePkg/Core/Dxe/Mem/Pool.c) which handles NULL as expected and returns EFI_INVALID_PARAMETER then.

I guess the issue is that FreePool() in MdeModulePkg/Library/DxeCoreMemoryAllocationLib/MemoryAllocationLib.c is coded as follows:

VOID
EFIAPI
FreePool (
  IN VOID   *Buffer
  )
{
  EFI_STATUS    Status;

  Status = CoreFreePool (Buffer);
  ASSERT_EFI_ERROR (Status);
}

So it will indeed raise an assert if the people who designed the UEFI firmware decided to enable them on production hardware (which, IMO, they shouldn't do, even if it's just on the serial console, but that's another story).

I guess I need to reflect on whether I want to alter the calls I use in EfiFs to check for NULL then, to avoid those asserts. I'll let you know about this when I apply the pull request.