[bug#57513] [PATCH] installer: Fix segfault on double logical partition removal.

[bug#57513] [PATCH] installer: Fix segfault on double logical partition removal.

[Josselin Poiret via Guix-patches via]

* gnu/installer/parted.scm (auto-partition!): Avoid removing logical
partitions twice.
---
 gnu/installer/parted.scm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm
index 641a1f45e8..84fdbe24fb 100644
--- a/gnu/installer/parted.scm
+++ b/gnu/installer/parted.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2018, 2019 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2019, 2020, 2022 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2022 Josselin Poiret <dev@jpoiret.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -983,6 +984,11 @@ (define* (auto-partition! disk
     (for-each
      (lambda (partition)
        (and (data-partition? partition)
+            ;; Do not remove logical partitions ourselves, since
+            ;; disk-remove-partition* will remove all the logical partitions
+            ;; residing on an extended partition, which would lead to a
+            ;; double-remove and ensuing SEGFAULT.
+            (not (logical-partition? partition))
             (disk-remove-partition* disk partition)))
      non-boot-partitions)
 

base-commit: 47c11772dfe840a536ed7ec438fe832878f51054
-- 
2.37.2

bug#57513: [PATCH] installer: Fix segfault on double logical partition removal.

[Mathieu Othacehe]

Hey,

> * gnu/installer/parted.scm (auto-partition!): Avoid removing logical
> partitions twice.

I was able to reproduce the issue by creating an extended partition
containing a single logical partition using the manual partitioning tool
then, the automatic one right after.

It resulted in a segfault, which is fixed by your patch, that's a very
nice catch!

Pushed as 4989f6acff3b3fcfbd9dde3e3c2767bd2cd6d49e.

Thanks,

Mathieu

[bug#57513] [PATCH] installer: Fix segfault on double logical partition removal.

[Josselin Poiret via Guix-patches via]

Hey Mathieu,

Mathieu Othacehe <othacehe@gnu.org> writes:

> I was able to reproduce the issue by creating an extended partition
> containing a single logical partition using the manual partitioning tool
> then, the automatic one right after.
>
> It resulted in a segfault, which is fixed by your patch, that's a very
> nice catch!

I have to thank KE0VVT on IRC, who provided a core dump file!  This was
surprisingly easier to debug than I thought, for those interested, I
built the installer using the same Guix commit, and loaded the guile
core dump file in gdb.  I then used `guix build parted
--with-debug-info=parted` and loaded the resulting libparted.so library
using `info sections` to find out where the .text of libparted.so was
loaded in the core file, and `add-symbol-file
/gnu/store/path/to/libparted.so 0xaddress` to load the symbols.  That
way, I could see that ped_disk_remove_partition was invoked for a disk
that had an empty partition list, hence leading me to this double remove
problem!

> Pushed as 4989f6acff3b3fcfbd9dde3e3c2767bd2cd6d49e.
>
> Thanks,
>
> Mathieu

Thank you for reviewing this so fast!

Best,
-- 
Josselin Poiret

[bug#57513] [PATCH] installer: Fix segfault on double logical partition removal.

[Mathieu Othacehe]

Hey,

> I have to thank KE0VVT on IRC, who provided a core dump file!  This was
> surprisingly easier to debug than I thought, for those interested, I
> built the installer using the same Guix commit, and loaded the guile
> core dump file in gdb.  I then used `guix build parted
> --with-debug-info=parted` and loaded the resulting libparted.so library
> using `info sections` to find out where the .text of libparted.so was
> loaded in the core file, and `add-symbol-file
> /gnu/store/path/to/libparted.so 0xaddress` to load the symbols.  That
> way, I could see that ped_disk_remove_partition was invoked for a disk
> that had an empty partition list, hence leading me to this double remove
> problem!

I remember resorting to way less convenient solutions in the past to
achieve something similar. Feel free to add this little memo to the
documentation or as a code comment if you have the opportunity :).

Thanks again,

Mathieu