storaged-project / blivet

A python module for configuration of block devices
GNU Lesser General Public License v2.1
99 stars 85 forks source link

part_type_uuid: guard against pyparted type_uuid being None #1265

Closed AdamWill closed 1 month ago

AdamWill commented 1 month ago

It's possible for the pyparted partition's type_uuid to be None in several ways: see all the NULL returns in https://github.com/dcantrell/pyparted/blob/d5870bb7a5d512c17c92a4e4fe7e9a8e7c8c3528/src/pydisk.c#L1481 In the specific case we hit, anaconda since 41.26 will try to get the part_type_uuid of a partition on an msdos-labeled disk when looking for Windows installations; as msdos-labeled disks do not support partition type UUIDs, pyparted's call to ped_partition_get_type_uuid gets a "msdos disk labels do not support partition type-uuids" error from parted and returns NULL, so pyparted tries to forward that error somehow then return NULL itself (though the error gets swallowed somewhere along the line, I had to patch pyparted to find out what was going on).

That means we try to do UUID(bytes=None), which blows up:

TypeError: one of the hex, bytes, bytes_le, fields, or int arguments must be given

due to a check at the start of UUID.__init__ that the value of exactly one of those args is not None.

To avoid this, let's check both that our pyparted supports type_uuid (which I think is the point of the existing condition here) and that the current pyparted partition's type_uuid is truth-y before trying to get a UUID. Arguably anaconda shouldn't even try to get a type UUID for a device on an msdos-labeled disk, but it seems reasonable to make blivet not blow up if it or something else does.

StorageGhoul commented 1 month ago

Can one of the admins verify this patch?

AdamWill commented 1 month ago

Specifically we hit this block in pyparted and return NULL at line 1512.

The anaconda code is this newly-added block which tries to find Windows devices. We could also/instead make that only bother with devices on GPT-labeled disks, something like this:

[adamw@xps13a anaconda ((anaconda-41.26) *%)]$ git diff
diff --git a/pyanaconda/modules/storage/devicetree/viewer.py b/pyanaconda/modules/storage/devicetree/viewer.py
index 13c60f97e9..5ae599080b 100644
--- a/pyanaconda/modules/storage/devicetree/viewer.py
+++ b/pyanaconda/modules/storage/devicetree/viewer.py
@@ -523,6 +523,8 @@ class DeviceTreeViewer(ABC):
         for blivet_device in self.storage.devicetree.devices:
             if not isinstance(blivet_device, PartitionDevice):
                 continue
+            if blivet_device.disk.format.label_type != "gpt":
+                continue

             device = self._get_device(blivet_device.name)
             if str(device.part_type_uuid) == EFI_PARTITION_TYPE:

...but I don't know if anaconda team wants to go with that or not, @KKoukiou ?

AdamWill commented 1 month ago

Beta blocker bug report - https://bugzilla.redhat.com/show_bug.cgi?id=2300115 .

Now I look, I notice this exact condition is already present twice in partition.py (in __repr__ and dict), but the second part of the condition was missing in this case.

Also, those two then go on to catch potential TypeError and ValueError exceptions from UUID, but this one catches AttributeError...in the source, I can see plenty of cases of UUID.__init__ directly raising TypeError and ValueError, but none of it directly raising AttributeError. The catching of AttributeError comes from 4fd51e692a6da9eaf7389d5cf43fc8a74b2bc1ef , when this code was originally introduced, and is not explained; @berrange wrote it, maybe he remembers why he thought it might raise AttributeError? I guess we should also have this invocation catch ValueError and TypeError like the other two, I'll extend the PR to do that.

vojtechtrefny commented 1 month ago

Jenkins, ok to test.