bug#35996: User account password got locked when booting old generation

["Ludovic Courtès" <ludo@gnu.org>]

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

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

> It appears your patch fixes the issue.  I admire the speed at which
> you write patches. :)  Thank you!

Awesome!  I must say that I’m really glad you’re putting this much
energy into reproducing issues and investigating—it’s rare for people
who report bug to dig this deep, but it’s super helpful and motivating!

I’ve pushed the whole series:

  d088d5c484 accounts: Call 'fdatasync' when writing databases.
  ed8570dce3 accounts: Close database before renaming it.
  70a7a1b5dc nar: Really lock store files.
  d497b6ab39 activation: Lock /etc/.pwd.lock before accessing databases.
  5f0cf1df71 syscalls: 'with-lock-file' catches ENOSYS.
  89ceb86ad4 syscalls: 'with-file-lock' expands to a call to 'call-with-file-lock'.
  b7178c22bf syscalls: Add 'with-file-lock' macro.

The actual fix is ed8570dce3, AIUI.

> I created a new working generation and then a new unbootable
> generation with broken udevd args, both with all your patches.  I
> rebooted the broken and then the working generation repeatedly twelve
> times.  I waited varying amounts of time before doing Ctrl+Alt+Del in
> the broken generation.  /etc/shadow is still in good health.

Good.

> On Tue, Jun 04, 2019 at 11:21:05PM +0200, Ludovic Courtès wrote:
>> Indeed, ‘shepherd’ calls ‘disable-reboot-on-ctrl-alt-del’ (which
>> disables “hard” reboots upon ctrl-alt-del and instead notifies it) after
>> it has loaded its config file.
>
> Is there a good reason shepherd calls disable-reboot-on-ctrl-alt-del
> at the end?  I get recovering journal messages unless on the previous
> boot I waited for the whole GDM to start (I can login on the TTY
> before GDM has fully started), which takes a long time during which
> users could change their mind and decide they do not want to boot.
> (The Macbook is not fast anyway and Guix is even slower when booting
> compared to Debian.)

I agree.

The attached patch for Shepherd moves everything before loading the
config file.  I think it will have the desired effect, though I’m not
entirely sure the signal handler would run at the right time etc.

You can test it on the metal if you want (you need to add the patch to
the ‘shepherd’ package), but I’ll see if I can test in a VM.

Thank you!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3264 bytes --]

diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index 8b2cc1d..769085a 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -198,34 +198,6 @@ socket file at FILE-NAME upon exit of PROC.  Return the values of PROC."
       ;; Start the 'root' service.
       (start root-service)
 
-      ;; This _must_ succeed.  (We could also put the `catch' around
-      ;; `main', but it is often useful to get the backtrace, and
-      ;; `caught-error' does not do this yet.)
-      (catch #t
-        (lambda ()
-          (load-in-user-module (or config-file (default-config-file))))
-        (lambda (key . args)
-          (caught-error key args)
-          (quit 1)))
-      ;; Start what was started last time.
-      (and persistency
-           (catch 'system-error
-             (lambda ()
-               (start-in-order (read (open-input-file
-                                      persistency-state-file))))
-             (lambda (key . args)
-               (apply format #f (gettext (cadr args)) (caddr args))
-               (quit 1))))
-
-      (when (provided? 'threads)
-        ;; XXX: This terrible hack allows us to make sure that signal handlers
-        ;; get a chance to run in a timely fashion.  Without it, after an EINTR,
-        ;; we could restart the accept(2) call below before the corresponding
-        ;; async has been queued.  See the thread at
-        ;; <https://lists.gnu.org/archive/html/guile-devel/2013-07/msg00004.html>.
-        (sigaction SIGALRM (lambda _ (alarm 1)))
-        (alarm 1))
-
       (when (= 1 (getpid))
         ;; When running as PID 1, disable hard reboots upon ctrl-alt-del.
         ;; Instead, the kernel will send us SIGINT so that we can gracefully
@@ -259,6 +231,34 @@ socket file at FILE-NAME upon exit of PROC.  Return the values of PROC."
         (lambda _
           (stop root-service)))
 
+      ;; This _must_ succeed.  (We could also put the `catch' around
+      ;; `main', but it is often useful to get the backtrace, and
+      ;; `caught-error' does not do this yet.)
+      (catch #t
+        (lambda ()
+          (load-in-user-module (or config-file (default-config-file))))
+        (lambda (key . args)
+          (caught-error key args)
+          (quit 1)))
+      ;; Start what was started last time.
+      (and persistency
+           (catch 'system-error
+             (lambda ()
+               (start-in-order (read (open-input-file
+                                      persistency-state-file))))
+             (lambda (key . args)
+               (apply format #f (gettext (cadr args)) (caddr args))
+               (quit 1))))
+
+      (when (provided? 'threads)
+        ;; XXX: This terrible hack allows us to make sure that signal handlers
+        ;; get a chance to run in a timely fashion.  Without it, after an EINTR,
+        ;; we could restart the accept(2) call below before the corresponding
+        ;; async has been queued.  See the thread at
+        ;; <https://lists.gnu.org/archive/html/guile-devel/2013-07/msg00004.html>.
+        (sigaction SIGALRM (lambda _ (alarm 1)))
+        (alarm 1))
+
       ;; Ignore SIGPIPE so that we don't die if a client closes the connection
       ;; prematurely.
       (sigaction SIGPIPE SIG_IGN)