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: Fri, 05 Mar 2021 14:19:53 -0800 Message-ID: <87y2f1meeu.fsf@mdeb> References: <83sg5r276b.fsf@gnu.org> <838s7j14xc.fsf@gnu.org> <837dmq95ee.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="12075"; 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 Fri Mar 05 23:21:25 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 1lIIp2-0002zb-Qk for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 05 Mar 2021 23:21:24 +0100 Original-Received: from localhost ([::1]:57912 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lIIp1-0006Wa-Bh for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 05 Mar 2021 17:21:23 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:45568) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lIIog-0006WH-DE for bug-gnu-emacs@gnu.org; Fri, 05 Mar 2021 17:21:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:52314) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lIIog-0007ym-67 for bug-gnu-emacs@gnu.org; Fri, 05 Mar 2021 17:21:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lIIog-0006GZ-1Z for bug-gnu-emacs@gnu.org; Fri, 05 Mar 2021 17:21: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: Fri, 05 Mar 2021 22:21:01 +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.161498280923999 (code B ref 46397); Fri, 05 Mar 2021 22:21:01 +0000 Original-Received: (at 46397) by debbugs.gnu.org; 5 Mar 2021 22:20:09 +0000 Original-Received: from localhost ([127.0.0.1]:35627 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lIInn-0006Ez-Mp for submit@debbugs.gnu.org; Fri, 05 Mar 2021 17:20:08 -0500 Original-Received: from relay9-d.mail.gandi.net ([217.70.183.199]:38449) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lIInl-0006EK-GL for 46397@debbugs.gnu.org; Fri, 05 Mar 2021 17:20:06 -0500 X-Originating-IP: 24.113.169.116 Original-Received: from mdeb (24-113-169-116.wavecable.com [24.113.169.116]) (Authenticated sender: matt@rfc20.org) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id BC471FF805; Fri, 5 Mar 2021 22:19:57 +0000 (UTC) In-Reply-To: <837dmq95ee.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:201581 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: Wed, 24 Feb 2021 09:37:49 -0800 >> >> I still like my original idea of calling display-warning for all unlock >> errors, essentially turning "unlock" into a best effort function at the >> API level. I think display-warning is intrusive enough that users are >> unlikely to simply not notice the problem, and there are worse things >> than leaving lock files around. > > OK, you've convinced me: let's try the warning approach. Can you > present a patch for that, please? Thanks Eli, will do. I have made progress with that idea, but I'd rather get the test in first. See patch attached below. > As for the tests you posted: too many of them rely on Posix file > modes, and thus will probably either fail or be unable to provide > meaningful testing on MS-Windows. Can we please augment that by tests > that create unlocking problems by, e.g., running a shell command to > remove or rename or otherwise sabotage the lock file, so that the new > functionality could be meaningfully tested on Windows as well? I have not been able to come up with a way to achieve what you ask for. I have no access to an MS-Windows system to test with, though I have been testing with small FAT and NTFS file systems in a ramdisk under GNU/Linux. It may be possible to take some approach that works on MS-Windows systems, but I'm not in a good position to write that code. Another issue is that the lock file is typicaly a symbolink link, and operating systems typically ignore file modes on symbolic links, so it is hard to put the lock file itself into an "invalid" state -- i.e. where simply attempting to access or delete it generates a file system level error. This is why I resorted to modifying file modes on the containing directory -- attemps to remove or access files in such a directory do reliably generate errors (on POSIX systems). What I've done is make the tests skip themselves when `set-file-mode' on the test's temporary directory appears to not work. When I test this on an ext2 file system the tests complete. When I test this on a FAT and NTFS file system (set up as a ramdisk on GNU/Linux), the tests skip themselves. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Add-some-test-coverage-for-src-filelock.c-Bug-46397.patch >From e05d400c177333cff0f558aff1948bcd55130c17 Mon Sep 17 00:00:00 2001 From: Matt Armstrong Date: Mon, 15 Feb 2021 12:59:08 -0800 Subject: [PATCH 1/2] Add some test coverage for src/filelock.c (Bug#46397). * test/src/filelock-tests.el: new file --- test/src/filelock-tests.el | 183 +++++++++++++++++++++++++++++++++++++ 1 file changed, 183 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..e568050c42 --- /dev/null +++ b/test/src/filelock-tests.el @@ -0,0 +1,183 @@ +;;; 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. + +;; Note: many of these tests set file modes to unusual values, with +;; the aim of exercising Emacs' error handling code. These tests +;; require POSIX file system semantics, which are not available on all +;; file systems (e.g. FAT, NTFS). The tests skip themselves when the +;; file system does not support the given file modes. + +;;; Code: + +(require 'cl-lib) +(require 'ert) + +(defvar filelock-tests-temporary-file-directory nil + "The directory for writing temporary files in filelock tests. +This is used by the function +`filelock-tests--make-temp-directory' to override the value of +the variable `temporary-file-directory'.") + +(defun filelock-tests--make-temp-directory () + "Create and return the name of a temporary directory. +The caller should delete the directory." + (let ((temporary-file-directory (or filelock-tests-temporary-file-directory + temporary-file-directory))) + (make-temp-file "test" t))) + +(defun filelock-tests--fixture (test-function) + "Call TEST-FUNCTION under a test fixture. +Create a test directory and a buffer whose `buffer-file-name' and +`buffer-file-truename' are a file within it. Call TEST-FUNCTION, +passing it directory name. Finally, delete the buffer and the +test directory. + +This function attempts to delete the test directory even if the +test leaves the buffer locked or the test directory with strange +permissions." + (let* ((temp-dir (filelock-tests--make-temp-directory)) + (name (concat (file-name-as-directory temp-dir) + "file")) + (create-lockfiles t)) + (unwind-protect + (with-temp-buffer + (setq buffer-file-name name) + (setq buffer-file-truename name) + (unwind-protect + (save-current-buffer + (funcall test-function temp-dir)) + + ;; Here begins somewhat delicate cleanup logic. It must + ;; be performed in this order. + ;; + ;; First, give Emacs permission to modify the test + ;; directory. The test may have left the diretory's modes + ;; in an unusual state. + ;; + ;; Second, mark the buffer unmodified. This prevents + ;; prompts from `kill-buffer' about saving unmodified + ;; buffers (when this test is run interactively). + ;; + ;; Third, delete the temp buffer (by way of + ;; `with-temp-buffer' above). Emacs may also delete the + ;; lock file from the test directory at this point. + ;; + ;; Fourth, delete the test directory itself. + + (set-file-modes temp-dir (logior #o700 (file-modes temp-dir))) + (set-buffer-modified-p nil))) + (delete-directory temp-dir t nil)))) + +(defun filelock-tests--make-file-inaccessible (filename) + "Make file named FILENAME inaccessible. +Returns non-nil if the operation succeeds, otherwise nil. Note: +not all file systems support this operation." + (set-file-modes filename #o000) + (equal #o000 (file-modes filename))) + +(ert-deftest filelock-tests-lock-unlock-no-errors () + "Check that locking and unlocking works without error." + (filelock-tests--fixture + (lambda (_) + (should (not (file-locked-p (buffer-file-name)))) + (insert "this locks the buffer's file") + (should (file-locked-p (buffer-file-name))) + (unlock-buffer) + (should (not (file-locked-p (buffer-file-name))))))) + +(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--fixture + (lambda (temp-dir) + (should (not (file-locked-p (buffer-file-name)))) + + (let ((original-modes (file-modes temp-dir))) + (skip-unless (filelock-tests--make-file-inaccessible temp-dir)) + ;; FIXME: file system errors when locking a file are ignored; + ;; should they be? + (insert "this locks the current buffer's file") + (set-file-modes temp-dir original-modes)) + + (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--fixture + (lambda (temp-dir) + (skip-unless (filelock-tests--make-file-inaccessible temp-dir)) + + (let ((err (should-error (file-locked-p (buffer-file-name))))) + (should (equal (cl-subseq err 0 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--fixture + (lambda (temp-dir) + (insert "this locks the current buffer's file") + (should (file-locked-p (buffer-file-name))) + + (skip-unless (filelock-tests--make-file-inaccessible temp-dir)) + + ;; FIXME: Unlocking buffers should not signal errors related to + ;; their lock files (bug#46397). + (let ((err (should-error (unlock-buffer)))) + (should (equal (cl-subseq err 0 2) + '(file-error "Unlocking file"))))))) + +(defun filelock-tests--yes (&rest _) + "Return t." + t) + +(ert-deftest filelock-tests-kill-buffer-permission-denied () + "Check that `unlock-buffer' fails in directories that cannot be +modified." + (filelock-tests--fixture + (lambda (temp-dir) + (insert "this should lock the buffer") + (should (file-locked-p (buffer-file-name))) + + (skip-unless (filelock-tests--make-file-inaccessible temp-dir)) + + ;; Kill the current buffer even if it is modified. Use advice to + ;; fake a "yes" answer for the "Buffer modified; kill anyway?" + ;; prompt. Leave the buffer modified so `kill-buffer' will + ;; attempt to unlock the buffer's file. + ;; + ;; FIXME: Killing buffers should not signal errors related to + ;; their lock files (bug#46397). + (let ((err (unwind-protect + (progn + (advice-add 'yes-or-no-p + :override + 'filelock-tests--yes) + (should-error (kill-buffer))) + (advice-remove 'yes-or-no-p 'filelock-tests--yes)))) + (should (equal (cl-subseq err 0 2) + '(file-error "Unlocking file"))))))) + +(provide 'filelock-tests) +;;; filelock-tests.el ends here -- 2.30.1 --=-=-=--