From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Matt Armstrong Newsgroups: gmane.emacs.bugs Subject: bug#46397: 27.1; Cannot delete buffer pointing to a file in a path that includes a file Date: Mon, 15 Feb 2021 16:49:05 -0800 Message-ID: References: <87h7mllgin.fsf@nexoid.at> <83a6scj745.fsf@gnu.org> <39d0e035-27b6-e2bd-daa2-747dda2c1a35@cs.ucla.edu> <83tuqhg3j9.fsf@gnu.org> <83czx4e5h5.fsf@gnu.org> <83zh06a1yv.fsf@gnu.org> <83wnv99xkz.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23149"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Feb 16 01:50:10 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lBoZ8-0005vE-JL for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 16 Feb 2021 01:50:10 +0100 Original-Received: from localhost ([::1]:41196 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lBoZ7-0007EH-3H for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 15 Feb 2021 19:50:09 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:59922) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lBoZ0-0007E7-H5 for bug-gnu-emacs@gnu.org; Mon, 15 Feb 2021 19:50:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:55668) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lBoZ0-0005Nj-A8 for bug-gnu-emacs@gnu.org; Mon, 15 Feb 2021 19:50:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lBoZ0-0002kc-7q for bug-gnu-emacs@gnu.org; Mon, 15 Feb 2021 19:50:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Matt Armstrong Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 16 Feb 2021 00:50:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 46397 X-GNU-PR-Package: emacs Original-Received: via spool by 46397-submit@debbugs.gnu.org id=B46397.161343656310469 (code B ref 46397); Tue, 16 Feb 2021 00:50:02 +0000 Original-Received: (at 46397) by debbugs.gnu.org; 16 Feb 2021 00:49:23 +0000 Original-Received: from localhost ([127.0.0.1]:38963 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lBoYN-0002in-2y for submit@debbugs.gnu.org; Mon, 15 Feb 2021 19:49:23 -0500 Original-Received: from relay12.mail.gandi.net ([217.70.178.232]:42135) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lBoYK-0002iX-RP for 46397@debbugs.gnu.org; Mon, 15 Feb 2021 19:49:22 -0500 Original-Received: from matts-mbp-2016.lan (24-113-169-116.wavecable.com [24.113.169.116]) (Authenticated sender: matt@rfc20.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 1B7D1200003; Tue, 16 Feb 2021 00:49:10 +0000 (UTC) In-Reply-To: <83wnv99xkz.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:200092 Archived-At: --=-=-= Content-Type: text/plain Eli Zaretskii writes: >> From: Matt Armstrong >> Cc: 46397@debbugs.gnu.org, eggert@cs.ucla.edu, craven@gmx.net >> Date: Sun, 14 Feb 2021 14:16:12 -0800 >> >> When releasing the lock, I have a less clear opinion but I'm thinking >> that warnings are better. A warning is still quite intrusive and >> obvious. Maybe we don't need to decide this part now. > > The problem with warnings is that they can go unnoticed, unless > followed by sit-for. Eli, Paul, Attached is test code for some of the scenarios we are discussing here. My FSF papers are out of date. Mind sending me new ones? I am in the United States. These tests operate by constructing buffers within directories that the test then restricts permissions on, which induces errors from the varous locking calls. Each test is a FIXME that allows us to verify the behavior change as work on bug#46397 progresses. In doing this I discovered that Emacs silently ignores some IO errors when constructing lock files. Paul noticed this a while back and added FIXME. This patch adds a test with a corresponding FIXME. The test isn't complete. It relies on POSIX semantics from the filesystem. It might fail on Windows. As the review progresses I can work on handling that scenario more gracefully. If this kind of a test is a bad idea, feel free to let me know. I like to have test coverage before I do code surgery, but I don't have strong opinions here. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Add-some-test-coverage-for-src-filelock.c-Bug-46397.patch Content-Description: add test coverage >From 99577c5a0b582e6d0c3d9fd495dec9f27d0cb7a2 Mon Sep 17 00:00:00 2001 From: Matt Armstrong Date: Mon, 15 Feb 2021 12:59:08 -0800 Subject: [PATCH] Add some test coverage for src/filelock.c (Bug#46397). * test/src/filelock-tests.el: new file --- test/src/filelock-tests.el | 115 +++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 test/src/filelock-tests.el diff --git a/test/src/filelock-tests.el b/test/src/filelock-tests.el new file mode 100644 index 0000000000..8cdcec09e7 --- /dev/null +++ b/test/src/filelock-tests.el @@ -0,0 +1,115 @@ +;;; filelock-tests.el --- test file locking -*- lexical-binding: t; -*- + +;; Copyright (C) 2021 Free Software Foundation, Inc. + +;; This file is part of GNU Emacs. + +;; GNU Emacs is free software; you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation, either version 3 of the License, or +;; (at your option) any later version. + +;; GNU Emacs is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. + +;; You should have received a copy of the GNU General Public License +;; along with this program. If not, see . + +;;; Commentary: + +;; This file tests code in src/filelock.c and, to some extent, the +;; related code in src/fileio.c. + +;;; Code: + +(require 'ert) + +(defun filelock-tests--call-with-fixture (body) + (let ((test-dir (make-temp-file "filelock-tests-" t))) + (unwind-protect + (let ((file-name (concat (file-name-as-directory test-dir)))) + (with-temp-buffer + (setq-local buffer-file-name file-name) + (setq-local buffer-file-truename file-name) + (unwind-protect + (save-current-buffer + (let ((create-lockfiles t)) + (funcall body))) + (set-buffer-modified-p nil)))) + (delete-directory test-dir t nil)))) + +(defmacro filelock-tests--with-fixture (&rest body) + "Create a test fixture and evaluate BODY there like `progn'. +Create a temporary directory, then create a temporary buffer and +set it the variable `buffer-file-name' and `buffer-file-truename' +to it, let bind `create-lockfiles' to 't' and evaluate BODY." + (declare (indent 0)) + `(filelock-tests--call-with-fixture + (lambda () ,@body))) + +(defun filelock-tests--call-with-directory-modes (modes body) + (let* ((dir (file-name-directory (buffer-file-name))) + (original-modes (file-modes dir))) + (set-file-modes dir modes) + (unwind-protect + (funcall body) + (set-file-modes dir original-modes)))) + +(defmacro filelock-tests--with-directory-modes (modes &rest body) + "Evaluate BODY forms with directory MODES set. +The directory is taken from the buffer local variable +`buffer-file-name'. Afterwards, restore the modes to what they +were." + (declare (indent 1)) + `(filelock-tests--call-with-directory-modes + ,modes (lambda () ,@body))) + +(ert-deftest filelock-tests-lock-buffer-permission-denied () + "Check that locking a buffer in a directory with no write +permissions does not work." + (filelock-tests--with-fixture + (should (not (file-locked-p (buffer-file-name)))) + (filelock-tests--with-directory-modes #o500 + ;; FIXME: (lock-buffer) should raise some sort of + ;; error/warning here that we should verify in test. + ;; Currently lock_file() ignores errors from lock_if_free(). + ;; See the related FIXME in filelock.c. + (lock-buffer) + (should (not (file-locked-p (buffer-file-name))))))) + +(ert-deftest filelock-tests-file-locked-p-permission-denied () + "Check that `file-locked-p' fails if the directory is inaccesible." + (filelock-tests--with-fixture + (filelock-tests--with-directory-modes #o000 + (set-buffer-modified-p t) + ;; FIXME: Bug#46397: `file-locked-p' currently signals errors, + ;; but this prevents the user from killing the buffer. It also + ;; prevents Emacs shutdown. + (should + (equal + (butlast (should-error (file-locked-p (buffer-file-name))) + 2) + '(file-error "Testing file lock")))))) + +(ert-deftest filelock-tests-unlock-permission-denied () + "Check that `unlock-buffer' fails in directories that cannot be +modified." + (filelock-tests--with-fixture + (set-buffer-modified-p t) + (lock-buffer) + (should (file-locked-p (buffer-file-name))) + (filelock-tests--with-directory-modes #o500 + ;; FIXME: Bug#46397: `unlock-buffer' currently signals errors, + ;; but this prevents the user from killing the buffer. It also + ;; prevents Emacs shutdown. + (should + (equal + (butlast (should-error (unlock-buffer)) + 2) + '(file-error "Unlocking file")))))) + +(provide 'filelock-tests) + +;;; filelock-tests.el ends here -- 2.30.0 --=-=-=--