bug#35783: Guile-Parted crashes the installer on i686-linux

bug#35783: Guile-Parted crashes the installer on i686-linux

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

Hello,

On i686-linux, the installer crashes when it reaches the partitioning
page with a null-pointer exception (or sometimes with a plain SIGSEGV),
in the call to ‘partition-disk’ in ‘esp-partition?’.  Screenshots
of two different crashes (null-pointer exception and SIGSEGV) attached.

The null-pointer exception happens when starting from a blank target
image (fresh ‘qemu-img create’), while the other crash happens (IIRC)
when the disk already has a partition table.

Mathieu, any ideas?

This is holding the 1.0.1 release (I actually have all the files ready
for upload after hours of builds :-/).  We’ll have to decide whether to
hold off until we have a fix, or whether to skip i686 altogether, or to
ship an install image where only the manual install may be used.

Thanks in advance!

Ludo’.


[-- Attachment #2: i686-crash.png --]
[-- Type: image/png, Size: 141548 bytes --]

[-- Attachment #3: i686-crash-gmp.png --]
[-- Type: image/png, Size: 16810 bytes --]

bug#35783: Guile-Parted crashes the installer on i686-linux

[Jonathan Brielmaier]

On my laptop (Thinkpad X240) as well as in QEMU I'll get the first error
(i686-crash.png). It makes no difference if I do separate home partition
or not. Encrypted partition or not still leads to this error...

bug#35783: Guile-Parted crashes the installer on i686-linux

[Ludovic Courtès]

Ludovic Courtès <ludo@gnu.org> skribis:

> On i686-linux, the installer crashes when it reaches the partitioning
> page with a null-pointer exception (or sometimes with a plain SIGSEGV),
> in the call to ‘partition-disk’ in ‘esp-partition?’.  Screenshots
> of two different crashes (null-pointer exception and SIGSEGV) attached.

Reverting 7d567af46b4e10ffafb1d0f76b524f5781460598 fixes the problem, so
I guess this has to do with the fact that the previous partition objects
(those listed in ‘initial-partitions’) are invalidated or something.

Now we need another way to address
<https://issues.guix.gnu.org/issue/35731>.

Ideas?

Thanks,
Ludo’.

bug#35783: Guile-Parted crashes the installer on i686-linux

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]


Hey Ludo,

> Reverting 7d567af46b4e10ffafb1d0f76b524f5781460598 fixes the problem, so
> I guess this has to do with the fact that the previous partition objects
> (those listed in ‘initial-partitions’) are invalidated or something.

Yes I think that's what happened, even though I don't get why it didn't
cause an issue on x64.

>
> Now we need another way to address
> <https://issues.guix.gnu.org/issue/35731>.

I have a first draft, I'm about to test right now :)

Mathieu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-draft-Fix-esp-user-partition-creation.patch --]
[-- Type: text/x-diff, Size: 2193 bytes --]

From 3c6018a448f38e263aeb5a23fbc88d226a048d67 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Sat, 18 May 2019 11:25:09 +0200
Subject: [PATCH] draft: Fix esp user-partition creation.

---
 gnu/installer/newt/partition.scm |  5 +----
 gnu/installer/parted.scm         | 12 ++++++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gnu/installer/newt/partition.scm b/gnu/installer/newt/partition.scm
index f8e318fa0d..cd9d46316a 100644
--- a/gnu/installer/newt/partition.scm
+++ b/gnu/installer/newt/partition.scm
@@ -752,10 +752,7 @@ by pressing the Exit button.~%~%")))
                             (disk-commit disk)
                             disk)))
                 (scheme (symbol-append method '- (run-scheme-page)))
-                (user-partitions (append
-                                  (auto-partition! disk #:scheme scheme)
-                                  (create-special-user-partitions
-                                   (disk-partitions disk)))))
+                (user-partitions (auto-partition! disk #:scheme scheme)))
            (run-disk-page (list disk) user-partitions
                           #:guided? #t)))
        ((eq? method 'manual)
diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm
index 4ccc0b1f51..ac9098cbde 100644
--- a/gnu/installer/parted.scm
+++ b/gnu/installer/parted.scm
@@ -1001,10 +1001,14 @@ swap partition, a root partition and a home partition."
                     (mount-point "/home")))))))
            (new-partitions* (force-user-partitions-formatting
                              new-partitions)))
-      (create-adjacent-partitions! disk
-                                   new-partitions*
-                                   #:last-partition-end
-                                   (or end-esp-partition 0)))))
+      (append
+       (if esp-partition
+           (partition->user-partition esp-partition)
+           '())
+       (create-adjacent-partitions! disk
+                                    new-partitions*
+                                    #:last-partition-end
+                                    (or end-esp-partition 0))))))
 
 \f
 ;;
-- 
2.17.1

bug#35783: Guile-Parted crashes the installer on i686-linux

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 150 bytes --]


With this almost indentical patch, disk partitioning seems fine with and
without existing esp partition on x64. I'll try to run more tests.

Mathieu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-draft-Fix-esp-user-partition-creation.patch --]
[-- Type: text/x-diff, Size: 2200 bytes --]

From 1e0734c4829725cdee6cab3cb05332ffd2a36a57 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Sat, 18 May 2019 11:25:09 +0200
Subject: [PATCH] draft: Fix esp user-partition creation.

---
 gnu/installer/newt/partition.scm |  5 +----
 gnu/installer/parted.scm         | 12 ++++++++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gnu/installer/newt/partition.scm b/gnu/installer/newt/partition.scm
index f8e318fa0d..cd9d46316a 100644
--- a/gnu/installer/newt/partition.scm
+++ b/gnu/installer/newt/partition.scm
@@ -752,10 +752,7 @@ by pressing the Exit button.~%~%")))
                             (disk-commit disk)
                             disk)))
                 (scheme (symbol-append method '- (run-scheme-page)))
-                (user-partitions (append
-                                  (auto-partition! disk #:scheme scheme)
-                                  (create-special-user-partitions
-                                   (disk-partitions disk)))))
+                (user-partitions (auto-partition! disk #:scheme scheme)))
            (run-disk-page (list disk) user-partitions
                           #:guided? #t)))
        ((eq? method 'manual)
diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm
index 4ccc0b1f51..196fa99cf4 100644
--- a/gnu/installer/parted.scm
+++ b/gnu/installer/parted.scm
@@ -1001,10 +1001,14 @@ swap partition, a root partition and a home partition."
                     (mount-point "/home")))))))
            (new-partitions* (force-user-partitions-formatting
                              new-partitions)))
-      (create-adjacent-partitions! disk
-                                   new-partitions*
-                                   #:last-partition-end
-                                   (or end-esp-partition 0)))))
+      (append
+       (if esp-partition
+           (list (partition->user-partition esp-partition))
+           '())
+       (create-adjacent-partitions! disk
+                                    new-partitions*
+                                    #:last-partition-end
+                                    (or end-esp-partition 0))))))
 
 \f
 ;;
-- 
2.17.1

bug#35783: Guile-Parted crashes the installer on i686-linux

[Ludovic Courtès]

Hi Mathieu,

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> With this almost indentical patch, disk partitioning seems fine with and
> without existing esp partition on x64. I'll try to run more tests.

I was fiddling with this and had arrived to a similar patch, we’re in
perfect symbiosis.  :-)

I’ve done some testing both in an EFI and a non-EFI setup with QEMU, and
it seems to work well; I’ll do some more testing as well.

>From 1e0734c4829725cdee6cab3cb05332ffd2a36a57 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <m.othacehe@gmail.com>
> Date: Sat, 18 May 2019 11:25:09 +0200
> Subject: [PATCH] draft: Fix esp user-partition creation.

[...]

> --- a/gnu/installer/parted.scm
> +++ b/gnu/installer/parted.scm
> @@ -1001,10 +1001,14 @@ swap partition, a root partition and a home partition."
>                      (mount-point "/home")))))))
>             (new-partitions* (force-user-partitions-formatting
>                               new-partitions)))
> -      (create-adjacent-partitions! disk
> -                                   new-partitions*
> -                                   #:last-partition-end
> -                                   (or end-esp-partition 0)))))
> +      (append
> +       (if esp-partition
> +           (list (partition->user-partition esp-partition))
> +           '())
> +       (create-adjacent-partitions! disk
> +                                    new-partitions*
> +                                    #:last-partition-end
> +                                    (or end-esp-partition 0))))))

Perhaps add something like this to the docstring of ‘auto-partition!’:

  Return the complete list of partitions on DISK, including the ESP when it
  exists.

Longer-term it would be good to audit Guile-Parted: it probably
shouldn’t be possible for Guile-Parted to refer to “defunct” Parted
objects.

Thank you for the quick response!

Ludo’.

bug#35783: Guile-Parted crashes the installer on i686-linux

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

It seems to be working well for me, including non-EFI i686.

I use the attached script to test an full install from the ISO, with or
without UEFI (though you cannot boot the install UEFI system because the
EFI variables set by GRUB are not saved.)

Ludo’.


[-- Attachment #2: the script --]
[-- Type: text/plain, Size: 449 bytes --]

#!/bin/sh
set -e
set -x
ISO="$(./pre-inst-env guix system disk-image --file-system-type=iso9660 gnu/system/install.scm)"
qemu-img create -f qcow2  /tmp/t.img 10G

#cp "$(guix build ovmf)/share/firmware/ovmf_x64.bin" /tmp
#chmod +w /tmp/ovmf_x64.bin
EFI_OPTS="-bios $(guix build ovmf)/share/firmware/ovmf_x64.bin"

exec qemu-system-x86_64 -enable-kvm -hda /tmp/t.img -cdrom "$ISO" -m 1024 -boot d -net user -net nic,model=virtio -no-reboot $EFI_OPTS

bug#35783: Guile-Parted crashes the installer on i686-linux

[Ludovic Courtès]

Hi,

I went ahead and pushed the patch as
d68de958b60426798ed62797ff7c96c327a672ac.

I’ll build the release from that commit.

Thanks,
Ludo’.

bug#35783: Guile-Parted crashes the installer on i686-linux

[Mathieu Othacehe]

Hey Ludo,

> Longer-term it would be good to audit Guile-Parted: it probably
> shouldn’t be possible for Guile-Parted to refer to “defunct” Parted
> objects.

Yup, with hindsight I realize that keeping Guile-Parted so low-level was
a mistake. With a few more abstractions (gnu installer parted) could be
less complicated.

Anyway, thanks for pushing this patch.

Mathieu

bug#35783: Guile-Parted crashes the installer on i686-linux

[Ludovic Courtès]

Hi Mathieu,

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

>> Longer-term it would be good to audit Guile-Parted: it probably
>> shouldn’t be possible for Guile-Parted to refer to “defunct” Parted
>> objects.
>
> Yup, with hindsight I realize that keeping Guile-Parted so low-level was
> a mistake. With a few more abstractions (gnu installer parted) could be
> less complicated.

I don’t know; as a rule of thumb, I think it’s good to make bindings a
direct mapping to the underlying library, and to build abstractions on
top of that.

That said, my point was more that it shouldn’t be possible to get a
null-pointer exception or a SIGSEGV when using Guile-Parted, even if you
make a mistake.  In this case, it seems that the underlying C object had
been reclaimed somehow; the bindings should protect against that.

Thoughts?

Ludo’.