Comment 5 for bug 1891718

Revision history for this message
dann frazier (dannf) wrote :

TLDR; I've convinced myself that the first patch can be safely dropped, so I've re-uploaded without it.

I attempted to demonstrate the problem I mentioned in Comment #3 that 0001-Fix-the-error-path-in-set_disk_and_part_name.patch addresses. Basically, what would happen if I passed in a device that set_disk_and_part_name() did not know about, causing it to incorrectly report no error while leaving dev->disk_name/dev->part_name uninitialized in the caller. Turns out, the answer is "nothing awful". I used the following test on a system where nvme0n1 was a nvme-subsys device, which focal's set_disk_and_part_name() would not recognize:

#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <sys/types.h>
#include <efivar/efiboot-creator.h>

extern int efi_set_verbose(int verbosity, FILE *errlog);

int main() {
  int err;
  efi_set_verbose(1, stderr);
  err = efi_generate_file_device_path_from_esp(NULL, 0,
            "/dev/nvme0n1", 1,
            "\\EFI\\debian\\grub.efi",
            EFIBOOT_ABBREV_EDD10,
            0x80);
}

Even though neither name was set, the pointer value happens to be zeroed out:

linux.c:387 device_get(): dev->disk_name: (null)
linux.c:388 device_get(): dev->part_name: (null)

And everything between that and the next error check seems to handle a NULL pointer well, so we safely bail at the next check:

linux.c:392 device_get(): readlink of /sys/block/(null)/device failed

That's great if they *are* zero'd out - but is that guaranteed? Yes - the dev is allocated using calloc, which always sets allocated memory to 0.