From patchwork Thu Jun 11 04:29:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Caleb Ristvedt X-Patchwork-Id: 22639 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 389F927BBE1; Thu, 11 Jun 2020 05:31:10 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,T_DKIM_INVALID, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTP id 3C87D27BBE3 for ; Thu, 11 Jun 2020 05:31:09 +0100 (BST) Received: from localhost ([::1]:39202 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jjErs-0006il-QP for patchwork@mira.cbaines.net; Thu, 11 Jun 2020 00:31:08 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:60338) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jjErm-0006id-Nt for guix-patches@gnu.org; Thu, 11 Jun 2020 00:31:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:52464) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jjErm-0001IQ-CU for guix-patches@gnu.org; Thu, 11 Jun 2020 00:31:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jjErm-0000Om-6x for guix-patches@gnu.org; Thu, 11 Jun 2020 00:31:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#41797] [PATCH] replace with-temporary-store-file Resent-From: Caleb Ristvedt Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Thu, 11 Jun 2020 04:31:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: report 41797 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 41797@debbugs.gnu.org X-Debbugs-Original-To: guix-patches@gnu.org Received: via spool by submit@debbugs.gnu.org id=B.15918498041389 (code B ref -1); Thu, 11 Jun 2020 04:31:02 +0000 Received: (at submit) by debbugs.gnu.org; 11 Jun 2020 04:30:04 +0000 Received: from localhost ([127.0.0.1]:35777 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jjEqp-0000MK-M3 for submit@debbugs.gnu.org; Thu, 11 Jun 2020 00:30:04 -0400 Received: from lists.gnu.org ([209.51.188.17]:41510) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jjEqn-0000LV-Nh for submit@debbugs.gnu.org; Thu, 11 Jun 2020 00:30:02 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:60198) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jjEqn-0006Zn-FE for guix-patches@gnu.org; Thu, 11 Jun 2020 00:30:01 -0400 Received: from mail-il1-x143.google.com ([2607:f8b0:4864:20::143]:42219) by eggs.gnu.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jjEql-0000g5-8K for guix-patches@gnu.org; Thu, 11 Jun 2020 00:30:00 -0400 Received: by mail-il1-x143.google.com with SMTP id j19so35483ilk.9 for ; Wed, 10 Jun 2020 21:29:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cune-org.20150623.gappssmtp.com; s=20150623; h=from:to:subject:date:message-id:user-agent:mime-version; bh=W4S1oMK9uG4Mh+wpZd/HN2ELVd8LpEFmNPTe9SI7wSg=; b=eC2nsM62o7tKUaPtoi57A6Tg4nve51kL7ima/kg2LWRfMeSQbe4u4RPtri1UT54CB7 zee0bH1D+95RwefgpG3ZPSfod4rZhVu7nVHI0zVyx+qz+QsBBoch751ki2ycUr0CViQN yLUzue3NtdAua9pbZ81/h4nohEFc+vkrwPG1n3RglU7Mh+EpE+fG2YaffaCUg5S1NKuc A+xNQP4YXUeAdzgF6btQ5PekdVkVCEuViJ7X1VUNX8yvomePdrKKz7feNdxVv+o1Q+a7 WLqM7OWcWBLo8nzg21jeTfb8oqX4L4UbIpcXbceve7rnoUqj1oIU5aaivz2aKScVAU+M h8Sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:user-agent :mime-version; bh=W4S1oMK9uG4Mh+wpZd/HN2ELVd8LpEFmNPTe9SI7wSg=; b=DQbkDpW3EpZRa976nE/vmYtglVlLFbzbp35lCE3ARt6FroL70yiTxpFh1E3hOV7lA/ /BSeSFlvm/oSHQxycFJFo7+/Qk8OAAHHRZAWq7KW+NulNvZ6wwcibt7lAZ3y2fe5jTRJ G1p9GUOgIcha4g20UYon2zHzreSUVj4TU5c5zrlDQxLaD7FafDF5xSE1UD7YM0h+NBxv rTzo/N86ZgUCdoacYuUE1h57UDstZAesMt3HFFYjJ4TbKcVrw4kwBZ6Z172LFdEo5m08 bKhSE5C8Y7hT1dCWoNdnHrwkKXNaLDCqspcX9dmeauivelyT+9uP6zdKhTd3ZoHsOoQ1 I7GA== X-Gm-Message-State: AOAM530FnC1J9TC2f3E5yi/5+BMAkbURHp5hoy9JBnWuseW3dj451owh h6e87BOvElE+qPeZynBPrfyf9UJZqIk= X-Google-Smtp-Source: ABdhPJyy7Zb9iNfladjcL8iqIVMtFawcWUD2XM0O6lmV0hl/xyDp8St4MlOJ/c2JyhheRLuCh7dQ4A== X-Received: by 2002:a92:506:: with SMTP id q6mr6423193ile.107.1591849791775; Wed, 10 Jun 2020 21:29:51 -0700 (PDT) Received: from GuixPotato ([208.89.170.24]) by smtp.gmail.com with ESMTPSA id d13sm977891ilo.40.2020.06.10.21.29.50 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Jun 2020 21:29:50 -0700 (PDT) From: Caleb Ristvedt Date: Wed, 10 Jun 2020 23:29:45 -0500 Message-ID: <87k10e5io6.fsf@cune.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Received-SPF: pass client-ip=2607:f8b0:4864:20::143; envelope-from=caleb.ristvedt@cune.org; helo=mail-il1-x143.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: "Guix-patches" X-getmail-retrieved-from-mailbox: Patches with-temporary-store-file suffers from race conditions - see attached patch commit message for details. This addresses a problem that it's very likely nobody has run into yet, due to the sheer size of the tempfile namespace, but that could potentially cause serious issues, including store corruption. The new procedure used to resolve this (restore-to-temp-store-file) will be of use in the guile-daemon anyway. - reepca From 8c26304951a2e21fe9b373b9cd543353d08fe003 Mon Sep 17 00:00:00 2001 From: Caleb Ristvedt Date: Wed, 10 Jun 2020 22:24:27 -0500 Subject: [PATCH] nar: fix race conditions inherent to with-temporary-store-file. with-temporary-store-file has a fundamental flaw: it assumes that if a temporary store file exists, is then added as a temporary root, and still exists, then it uniquely belongs to the current process. This is not always the case, because the only criteria used for choosing temporary file names is that they not be currently used and fit a certain template. This means it's entirely possible for another process to choose the same temporary file name if it doesn't exist at the time it chooses. Suppose process A chooses F as its temporary file name in with-temporary-store-file. Suppose before it adds it as a temp root, F gets garbage collected. Suppose process B then happens to, at that point, choose F as its temporary file name. This is completely valid, because F doesn't exist at that point. Then process A will check whether F exists, see that it does, and assume that it uniquely owns that file, when in reality two processes are now simultaneously trying to use it. Process A will then delete the file in question, causing errors in process A, B, or both. The same issue (two processes both believing they've exclusively allocated a temp file) can occur any time between when process A deletes the temp file prior to running the body and when (if) its body actually creates the temp file. In short, allocating a temporary file *name* isn't possible with the mechanisms currently in place - we have no way of telling other users of with-temporary-store-file whether a file name is allocated or not. Allocating a temporary *file*, on the other hand, is possible. This removes with-temporary-store-file and replaces it with with-temporary-restored-file, and adjusts its only user, restore-one-item, accordingly. * guix/serialization.scm (restore-to-temp-store-file): new procedure. * guix/nar.scm (with-temporary-store-file): replaced with with-temporary-restored-file. (with-temporary-restored-file): new macro. (restore-one-item): use with-temporary-restored-file instead of with-temporary-store-file. (temporary-store-file): removed, as it is no longer used. (Local Variables footer): update indenting information for with-temporary-restored-file. --- guix/nar.scm | 38 +++++++------------- guix/serialization.scm | 79 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 27 deletions(-) diff --git a/guix/nar.scm b/guix/nar.scm index eff4becbce..2666d32169 100644 --- a/guix/nar.scm +++ b/guix/nar.scm @@ -128,30 +128,18 @@ held." (force-output lock) (unlock-file lock)))))) -(define (temporary-store-file) - "Return the file name of a temporary file created in the store." - (let* ((template (string-append (%store-prefix) "/guix-XXXXXX")) - (port (mkstemp! template))) - (close-port port) - template)) - -(define-syntax-rule (with-temporary-store-file name body ...) +(define-syntax-rule (with-temporary-restored-file port name body ...) "Evaluate BODY with NAME bound to the file name of a temporary store item -protected from GC." +restored from PORT and protected from GC. Note that the temporary store item +won't be automatically deleted." (with-store store - (let loop ((name (temporary-store-file))) - ;; Add NAME to the current process' roots. (Opening this connection to - ;; the daemon allows us to reuse its code that deals with the - ;; per-process roots file.) - (add-temp-root store name) - - ;; There's a window during which GC could delete NAME. Try again when - ;; that happens. - (if (file-exists? name) - (begin - (delete-file name) - body ...) - (loop (temporary-store-file)))))) + ;; Add NAME to the current process' roots. (Opening this connection to + ;; the daemon allows us to reuse its code that deals with the per-process + ;; roots file.) + (let ((name (restore-to-temp-store-file port + (lambda (root) + (add-temp-root store root))))) + body ...))) (define* (restore-one-item port #:key acl (verify-signature? #t) (lock? #t) @@ -208,9 +196,7 @@ s-expression")) (let-values (((port get-hash) (open-sha256-input-port port))) - (with-temporary-store-file temp - (restore-file port temp) - + (with-temporary-restored-file port temp (let ((magic (read-int port))) (unless (= magic %export-magic) (raise (condition @@ -286,7 +272,7 @@ while the locks are held." (port port) (file #f) (token #f)))))))) ;;; Local Variables: -;;; eval: (put 'with-temporary-store-file 'scheme-indent-function 1) +;;; eval: (put 'with-temporary-restored-file 'scheme-indent-function 2) ;;; End: ;;; nar.scm ends here diff --git a/guix/serialization.scm b/guix/serialization.scm index 836ad06caf..c6b9674104 100644 --- a/guix/serialization.scm +++ b/guix/serialization.scm @@ -18,6 +18,7 @@ (define-module (guix serialization) #:use-module (guix combinators) + #:use-module (guix config) #:use-module (rnrs bytevectors) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) @@ -28,6 +29,7 @@ #:use-module (ice-9 match) #:use-module (ice-9 ftw) #:use-module (system foreign) + #:use-module (rnrs io ports) #:export (write-int read-int write-long-long read-long-long write-padding @@ -51,7 +53,8 @@ write-file write-file-tree fold-archive - restore-file)) + restore-file + restore-to-temp-store-file)) ;;; Comment: ;;; @@ -477,6 +480,80 @@ Restore it as FILE." port file)) +(define* (restore-to-temp-store-file port add-temp-root #:optional name) + "Read a file (possibly a directory structure) in Nar format from PORT. +Restore it as a temporary file and return the temporary file name. +ADD-TEMP-ROOT must be a procedure of one argument (the file name) that +protects that file name from garbage collection. Note that ADD-TEMP-ROOT may +be called multiple times." + (define (tempname) + (let ((template (string-append %store-directory "/guix-temp-file" + (if name + (string-append "-" name) + "") + ".XXXXXX"))) + (close (mkstemp! template)) + (delete-file template) + template)) + + (define (create/top-level create) + (let ((name (tempname))) + (add-temp-root name) + (catch 'system-error + (lambda () + (create name) + name) + (lambda args + (if (= (system-error-errno args) EEXIST) + (mkdir/top-level) + (apply throw args)))))) + + (define (create-output-file name input size type) + (call-with-port (open name (logior O_WRONLY + O_EXCL + O_CREAT)) + (lambda (output) + (dump input output size) + (when (eq? type 'executable) + (chmod output #o755))))) + + (define (create-output-file/top-level input size type) + (create/top-level (cut create-output-file <> input size type))) + + (define mkdir/top-level + (cut create/top-level mkdir)) + + (define (symlink/top-level target) + (create/top-level (cut symlink target <>))) + + (fold-archive (lambda (file type content top-name) + (define-syntax-rule (if-top-level exp1 exp2) + (if top-name + (begin + exp2 + top-name) + exp1)) + + (match type + ('directory + (if-top-level (mkdir/top-level) + (mkdir (string-append top-name file)))) + ('symlink + (if-top-level + (symlink/top-level content) + (symlink content (string-append top-name file)))) + ((or 'regular 'executable) + (match content + ((input . size) + (if-top-level + (create-output-file/top-level input size type) + (create-output-file (string-append top-name + file) + input size type))))))) + #f + port + "")) + ;;; Local Variables: ;;; eval: (put 'call-with-binary-input-file 'scheme-indent-function 1) ;;; End: -- 2.26.2