Guile-Parted crashes the installer on i686-linux

  • Done
  • quality assurance status badge
Details
3 participants
  • Jonathan Brielmaier
  • Ludovic Courtès
  • Mathieu Othacehe
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
important
L
L
Ludovic Courtès wrote on 17 May 2019 21:46
(address . bug-Guix@gnu.org)
87h89syidd.fsf@gnu.org
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: i686-crash.png
Attachment: i686-crash-gmp.png
J
J
Jonathan Brielmaier wrote on 17 May 2019 22:03
(address . 35783@debbugs.gnu.org)
90464466-424e-9092-7cba-673ebacf2c24@web.de
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...
L
L
Ludovic Courtès wrote on 17 May 2019 22:48
control message for bug #35783
(address . control@debbugs.gnu.org)
87ftpcyfhl.fsf@gnu.org
severity 35783 important
L
L
Ludovic Courtès wrote on 17 May 2019 23:22
Re: bug#35783: Guile-Parted crashes the installer on i686-linux
(address . 35783@debbugs.gnu.org)(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)
877eaoydw8.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (5 lines)
> 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

Ideas?

Thanks,
Ludo’.
M
M
Mathieu Othacehe wrote on 18 May 2019 11:30
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 35783@debbugs.gnu.org)
87h89shzyq.fsf@gmail.com
Hey Ludo,

Toggle quote (4 lines)
> 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.

Toggle quote (4 lines)
>
> 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
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(-)

Toggle diff (41 lines)
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))))))
;;
--
2.17.1
M
M
Mathieu Othacehe wrote on 18 May 2019 12:25
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 35783@debbugs.gnu.org)
87ftpchxel.fsf@gmail.com
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
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(-)

Toggle diff (41 lines)
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))))))
;;
--
2.17.1
L
L
Ludovic Courtès wrote on 18 May 2019 13:50
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 35783@debbugs.gnu.org)
87imu8vv5i.fsf@gnu.org
Hi Mathieu,

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

Toggle quote (3 lines)
> 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.

Toggle quote (5 lines)
>>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.

[...]

Toggle quote (19 lines)
> --- 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’.
L
L
Ludovic Courtès wrote on 18 May 2019 15:44
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 35783@debbugs.gnu.org)
87lfz3vpw4.fsf@gnu.org
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’.
#!/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
L
L
Ludovic Courtès wrote on 19 May 2019 12:09
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 35783-done@debbugs.gnu.org)
87zhniu55q.fsf@gnu.org
Hi,

I went ahead and pushed the patch as
d68de958b60426798ed62797ff7c96c327a672ac.

I’ll build the release from that commit.

Thanks,
Ludo’.
Closed
M
M
Mathieu Othacehe wrote on 19 May 2019 16:24
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 35783@debbugs.gnu.org)
87v9y6cyjp.fsf@gmail.com
Hey Ludo,

Toggle quote (4 lines)
> 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
L
L
Ludovic Courtès wrote on 20 May 2019 10:17
(name . Mathieu Othacehe)(address . m.othacehe@gmail.com)(address . 35783@debbugs.gnu.org)
87tvdpwndy.fsf@gnu.org
Hi Mathieu,

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

Toggle quote (8 lines)
>> 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’.
?