bug#58732: installer: finalizers & device destroy segfault

[Mathieu Othacehe]

Hey,

I made some progress on that one. I think, this is what's going on:

1. Two new PedDevice A and B are malloc'ed by the libparted when opening
the installer partitioning page.

2. They are added to the %devices weak hash table by pointer->device!
and their respective finalizers are registered.

3. The partitioning ends and A goes out of scope. It is eventually
removed from %devices but it does not mean its finalizer will be run
immediately.

4. The partitioning is restarted using the installer menu. B is still in
the %devices hash table. However, A is now gone and is added again to
the %devices hash table by the pointer->device! procedure. Another
finalizer is registered for A.

That's because set-pointer-finalizer! does not *set* a finalizer it
*adds* one.

5. The partitioning ends and both A and B goes out of scope. They are
removed from %devices and their finalizers are called. The A finalizer
is called twice resulting in a double free.

This race condition is created by the fact that there is a time window
where the device is removed from the %devices hash table but its
finalizer is not immediately called.

If set-pointer-finalizer! actually called scm_i_set_finalizer instead of
scm_i_add_finalizer the A finalizer would be set twice but called only
once. Do you think it would be an option?

I attached the instrumentation patches (good old printf's) as well as
the syslog I based my analysis upon.

Thanks,

Mathieu

diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm
index 82375d29e3..381e1b3ce7 100644
--- a/gnu/installer/parted.scm
+++ b/gnu/installer/parted.scm
@@ -1502,6 +1502,7 @@ (define (user-partitions->configuration user-partitions)
 
 (define (init-parted)
   "Initialize libparted support."
+  (%parted-syslog-port (syslog-port))
   (probe-all-devices!)
   ;; Remove all logical devices, otherwise "device-is-busy?" will report true
   ;; on all devices containaing active logical volumes.

diff -aur parted/libparted/arch/linux.c tmp/parted-3.5/libparted/arch/linux.c
--- parted/libparted/arch/linux.c       2022-11-04 10:14:33.551737324 +0100
+++ tmp/parted-3.5/libparted/arch/linux.c       2022-04-18 20:38:45.000000000 
+0200
@@ -17,7 +17,6 @@
 
 #define PROC_DEVICES_BUFSIZ 16384
 
-#include <syslog.h>
 #include <config.h>
 #include <arch/linux.h>
 #include <linux/blkpg.h>
@@ -44,7 +43,6 @@
 #include <assert.h>
 #include <sys/sysmacros.h>
 #ifdef ENABLE_DEVICE_MAPPER
-
 #include <libdevmapper.h>
 #endif
 
@@ -89,8 +87,6 @@
 #define WR_MODE (O_WRONLY)
 #define RW_MODE (O_RDWR)
 
-int syslog_init;
-
 struct hd_geometry {
         unsigned char heads;
         unsigned char sectors;
@@ -1600,11 +1596,6 @@
                                 _("ped_device_new()  Unsupported device 
type"));
                 goto error_free_arch_specific;
         }
-        if (!syslog_init) {
-                openlog("parted", LOG_PID, LOG_USER);
-                syslog_init = 1;
-        }
-        syslog(LOG_INFO, "parted: new: %p\n", dev);
         return dev;
 
 error_free_arch_specific:
@@ -1620,8 +1611,6 @@
 static void
 linux_destroy (PedDevice* dev)
 {
-        syslog(LOG_INFO, "parted: destroy: %p\n", dev);
-
         LinuxSpecific *arch_specific = LINUX_SPECIFIC(dev);
         void *p = arch_specific->dmtype;
 

diff --git a/parted/device.scm b/parted/device.scm
index 9f688dd..36d83f4 100644
--- a/parted/device.scm
+++ b/parted/device.scm
@@ -23,7 +23,7 @@
   #:use-module (parted geom)
   #:use-module (parted natmath)
   #:use-module (parted structs)
-  #:export (parted-syslog-port
+  #:export (%parted-syslog-port
             probe-all-devices!
             get-device
             get-device-next
@@ -44,8 +44,8 @@
             device-get-minimum-alignment
             device-get-optimum-alignment))
 
-(define parted-syslog-port
-  (make-parameter #f))
+(define %parted-syslog-port
+  (make-parameter #t))
 
 ;; Record all devices, so that pointer finalizers are only set once,
 ;; even if get-device returns an already known pointer.  Use the
@@ -58,22 +58,22 @@
 (define (pointer->device! pointer)
   ;; Check if a finalizer is already registered for this pointer.
   (format (%parted-syslog-port)
-          "guile-parted: pointer->device!: ~a" pointer)
+          "guile-parted: pointer->device!: ~a~%" pointer)
 
   (format (%parted-syslog-port)
-          "guile-parted: hash begin")
+          "guile-parted: hash begin~%")
   (hash-for-each (lambda (k v)
                    (format (%parted-syslog-port)
-                           "guile-parted: hash: ~a -> ~a" k v))
+                           "guile-parted: hash: ~a -> ~a~%" k v))
                  %devices)
   (format (%parted-syslog-port)
-          "guile-parted: hash end")
+          "guile-parted: hash end~%")
 
   (or (hash-ref %devices pointer)
       (let ((device (pointer->device pointer)))
 
         (format (%parted-syslog-port)
-          "guile-parted: finalizer!: ~a" pointer)
+          "guile-parted: finalizer!: ~a~%" pointer)
 
         ;; Contrary to its name, this "adds" a finalizer.
         (set-pointer-finalizer! pointer %device-destroy)

messages
Description: Binary data