unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: "Ludovic Courtès" <ludo@gnu.org>
To: Caleb Ristvedt <caleb.ristvedt@cune.org>
Cc: 42023@debbugs.gnu.org, Christopher Baines <mail@cbaines.net>
Subject: [bug#42023] [PATCH] database: register-items: reduce transaction scope.
Date: Thu, 25 Jun 2020 09:45:14 +0200	[thread overview]
Message-ID: <87eeq3vbat.fsf@gnu.org> (raw)
In-Reply-To: <87v9jg4aiz.fsf@cune.org> (Caleb Ristvedt's message of "Wed, 24 Jun 2020 12:51:48 -0500")

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

Hi,

Caleb Ristvedt <caleb.ristvedt@cune.org> skribis:

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

[...]

>> Two questions:
>>
>>   1. Can another process come and fiddle with TO-REGISTER while we’re
>>      still in ‘reset-timestamps’?  Or can GC happen while we’re in
>>      ‘reset-timestamps’ and delete TO-REGISTER and remove it from the
>>      database?
>
>>
>> I think none of these scenarios can happen, as long as we’ve taken the
>> .lock file for TO-REGISTER before, like ‘finalize-store-file’ does.
>
> The lock file has no bearing on liveness of the path it locks, though
> the liveness of the path it locks *does* count as liveness for the lock
> file and for the .chroot file; see tryToDelete() in nix/libstore/gc.cc.
>
> (Well, semi-liveness; .lock and .chroot files won't show up in a list of
> live paths, but they will still be protected from deletion if their
> associated store item is a temp root)
>
> So general "fiddling" can't happen, but GC can. The responsibility for
> both locking and registering as temporary roots falls on the caller,
> currently, as I believe it should. We may want to document this
> responsibility in the docstring for register-items, though.

Yes, it would be good to add a sentence to document it.

> So currently finalize-store-file is safe from clobbering by another
> process attempting to finalize to the same path, but not safe from
> garbage collection between when the temporary store file is renamed and
> when it is registered. It needs an add-temp-root prior to renaming.

Ah, so we’d need to do that before applying the patch that reduces the
scope of the transaction, right?

>>   2. After the transaction, TO-REGISTER is considered valid.  But are
>>      the effects of the on-going deduplication observable, due to
>>      non-atomicity of some operation?
>
> Subdirectories of store items need to be made writable prior to renaming
> the temp link, so there will necessarily be a window of time during
> which various subdirectories will appear writable. I don't think this
> should cause problems; we already assume that the .lock file is held, so
> we should be the only process attempting to deduplicate it. On a related
> note, we should probably use dynamic-wind for setting and restoring the
> permissions in replace-with-link.

Yes.  Here’s a patch for ‘dynamic-wind’:


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

diff --git a/.dir-locals.el b/.dir-locals.el
index 92979fc5ed..155255a770 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -37,6 +37,7 @@
    (eval . (put 'with-file-lock 'scheme-indent-function 1))
    (eval . (put 'with-file-lock/no-wait 'scheme-indent-function 1))
    (eval . (put 'with-profile-lock 'scheme-indent-function 1))
+   (eval . (put 'with-writable-file 'scheme-indent-function 1))
 
    (eval . (put 'package 'scheme-indent-function 0))
    (eval . (put 'origin 'scheme-indent-function 0))
diff --git a/guix/store/deduplication.scm b/guix/store/deduplication.scm
index 6784ee0b92..af52c03370 100644
--- a/guix/store/deduplication.scm
+++ b/guix/store/deduplication.scm
@@ -94,6 +94,20 @@ LINK-PREFIX."
             (try (tempname-in link-prefix))
             (apply throw args))))))
 
+(define (call-with-writable-file file thunk)
+  (let ((stat (lstat file)))
+    (dynamic-wind
+      (lambda ()
+        (make-file-writable file))
+      thunk
+      (lambda ()
+        (set-file-time file stat)
+        (chmod file (stat:mode stat))))))
+
+(define-syntax-rule (with-writable-file file exp ...)
+  "Make FILE writable for the dynamic extent of EXP..."
+  (call-with-writable-file file (lambda () exp ...)))
+
 ;; There are 3 main kinds of errors we can get from hardlinking: "Too many
 ;; things link to this" (EMLINK), "this link already exists" (EEXIST), and
 ;; "can't fit more stuff in this directory" (ENOSPC).
@@ -120,20 +134,14 @@ Note: TARGET, TO-REPLACE, and SWAP-DIRECTORY must be on the same file system."
   ;; If we couldn't create TEMP-LINK, that's OK: just don't do the
   ;; replacement, which means TO-REPLACE won't be deduplicated.
   (when temp-link
-    (let* ((parent (dirname to-replace))
-           (stat   (stat parent)))
-      (make-file-writable parent)
+    (with-writable-file (dirname to-replace)
       (catch 'system-error
         (lambda ()
           (rename-file temp-link to-replace))
         (lambda args
           (delete-file temp-link)
           (unless (= EMLINK (system-error-errno args))
-            (apply throw args))))
-
-      ;; Restore PARENT's mtime and permissions.
-      (set-file-time parent stat)
-      (chmod parent (stat:mode stat)))))
+            (apply throw args)))))))
 
 (define* (deduplicate path hash #:key (store %store-directory))
   "Check if a store item with sha256 hash HASH already exists.  If so,

[-- Attachment #3: Type: text/plain, Size: 6064 bytes --]


> Also, replace-with-link doesn't check whether the "containing directory"
> is the store like optimisePath_() does, so in theory if another process
> tried to make a permanent change to the store's permissions it could be
> clobbered when replace-with-link restores the permissions. I don't know
> of any instance where this could happen currently, but it's something to
> keep in mind.

I guess we should also avoid changing permissions on /gnu/store, it
would be wiser.

> And, of course, there's the inherent visible change of deduplication -
> possible modification of inode number, but I don't see how that could
> cause problems with any reasonable programs.

Yes, that’s fine.

>> I think the ‘replace-with-link’ dance is atomic, so we should be fine.
>>
>> Thoughts?
>
> replace-with-link is atomic, but it can fail if the "canonical" link in
> .links is deleted by the garbage collector between when its existence is
> checked in 'deduplicate' and when temp link creation in
> replace-with-link happens. Then ENOENT would be returned, and
> 'deduplicate' wouldn't catch that exception, nor would optimisePath_()
> in nix/libstore/optimise-store.cc.
>
> The proper behavior there, in my opinion, would be to retry the
> deduplication. Attached is a patch that makes 'deduplicate' do that.
>
> Also, while I'm perusing optimisePath_(), there's a minor bug in which
> EMLINK generated by renaming the temp link may still be treated as a
> fatal error. This is because errno may be clobbered by unlink() prior to
> being checked (according to the errno man page it can still be modified
> even if the call succeeds).

Indeed, good catch!

> From 461064da9e169df3dd939b734bb2860598d113f4 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Wed, 24 Jun 2020 00:56:50 -0500
> Subject: [PATCH 1/2] deduplication: retry on ENOENT.
>
> It's possible for the garbage collector to remove the "canonical" link after
> it's been detected as existing by 'deduplicate'.  This would cause an ENOENT
> error when replace-with-link attempts to create the temporary link.  This
> changes it so that it will properly handle that by retrying.
>
> * guix/store/deduplication.scm (deduplicate): retry on ENOENT.
> ---
>  guix/store/deduplication.scm | 64 +++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/guix/store/deduplication.scm b/guix/store/deduplication.scm
> index 6784ee0b92..b6d94e49c2 100644
> --- a/guix/store/deduplication.scm
> +++ b/guix/store/deduplication.scm
> @@ -161,26 +161,44 @@ under STORE."
>                    (scandir* path))
>          (let ((link-file (string-append links-directory "/"
>                                          (bytevector->nix-base32-string hash))))
> -          (if (file-exists? link-file)
> -              (replace-with-link link-file path
> -                                 #:swap-directory links-directory)
> -              (catch 'system-error
> -                (lambda ()
> -                  (link path link-file))
> -                (lambda args
> -                  (let ((errno (system-error-errno args)))
> -                    (cond ((= errno EEXIST)
> -                           ;; Someone else put an entry for PATH in
> -                           ;; LINKS-DIRECTORY before we could.  Let's use it.
> -                           (replace-with-link path link-file
> -                                              #:swap-directory links-directory))
> -                          ((= errno ENOSPC)
> -                           ;; There's not enough room in the directory index for
> -                           ;; more entries in .links, but that's fine: we can
> -                           ;; just stop.
> -                           #f)
> -                          ((= errno EMLINK)
> -                           ;; PATH has reached the maximum number of links, but
> -                           ;; that's OK: we just can't deduplicate it more.
> -                           #f)
> -                          (else (apply throw args)))))))))))
> +          (let retry ()
> +            (if (file-exists? link-file)
> +                (catch 'system-error
> +                  (lambda ()
> +                    (replace-with-link link-file path
> +                                       #:swap-directory links-directory))
> +                  (lambda args
> +                    (if (and (= (system-error-errno args) ENOENT)
> +                             ;; ENOENT can be produced because:
> +                             ;; - LINK-FILE has missing directory components
> +                             ;; - LINKS-DIRECTORY has missing directory
> +                             ;;   components
> +                             ;; - PATH has missing directory components
> +                             ;;
> +                             ;; the last two are errors, the first just
> +                             ;; requires a retry.
> +                             (file-exists? (dirname path))
> +                             (file-exists? links-directory))
> +                        (retry)
> +                        (apply throw args))))

I feel like there are TOCTTOU issues here and redundant ‘stat’ calls,
plus the risk of catching a ‘system-error’ coming from “somewhere else.”

What about baking this logic directly in ‘replace-with-link’, and
replacing ‘file-exists?’ calls by 'system-error handling?

> From a14ff0a9dab4d73680befaf9d244d6cce0a73ab3 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Wed, 24 Jun 2020 01:00:40 -0500
> Subject: [PATCH 2/2] .dir-locals.el: fix call-with-{retrying-}transaction
>  indenting.
>
> * .dir-locals.el (call-with-transaction, call-with-retrying-transaction):
>   change scheme-indent-function property from 2 to 1.

LGTM!

Thanks for your quick feedback and thorough analyses!

Ludo’.

  reply	other threads:[~2020-06-25  7:46 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 16:36 [bug#42023] [PATCH] database: register-items: reduce transaction scope Christopher Baines
2020-06-23 22:15 ` Ludovic Courtès
2020-06-24 17:51   ` Caleb Ristvedt
2020-06-25  7:45     ` Ludovic Courtès [this message]
2020-07-26 15:31       ` Ludovic Courtès
2020-08-09  4:13         ` Caleb Ristvedt
2020-09-14  8:58           ` Ludovic Courtès
2020-09-15 20:29             ` [bug#42023] [PATCH] Retry deduplication on ENOENT Caleb Ristvedt
2020-09-16 20:37               ` Ludovic Courtès

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87eeq3vbat.fsf@gnu.org \
    --to=ludo@gnu.org \
    --cc=42023@debbugs.gnu.org \
    --cc=caleb.ristvedt@cune.org \
    --cc=mail@cbaines.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).