bug#64821: 30.0.50; write-region errs when overwriting an already open file

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Ihor Radchenko]

Consider the following reproducer:

1. emacs -Q
2. Evaluate the following in scratch buffer

(with-temp-buffer
  (insert "test")
  (write-region nil nil "/tmp/1.txt"))

3. Open /tmp/1.txt
4. Evaluate the above sexp multiple times
5. Observe

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  expand-file-name(nil)
  userlock--check-content-unchanged("/tmp/1.txt")
  userlock--ask-user-about-supersession-threat("/tmp/1.txt")
  write-region(nil nil "/tmp/1.txt")
  (progn (insert "test") (write-region nil nil "/tmp/1.txt"))
  (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer)))
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer))))
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer)))))
  (progn (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer))))))
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  command-execute(eval-last-sexp)

Expected: No error is thrown.

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.8) of 2023-07-21 built on localhost
Repository revision: 7f77120683d6f150e61e11b41d75ba16ee5210a4
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Gentoo Linux


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Mon, 24 Jul 2023 07:12:00 +0000
> 
> 1. emacs -Q
> 2. Evaluate the following in scratch buffer
> 
> (with-temp-buffer
>   (insert "test")
>   (write-region nil nil "/tmp/1.txt"))
> 
> 3. Open /tmp/1.txt
> 4. Evaluate the above sexp multiple times
> 5. Observe
> 
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>   expand-file-name(nil)
>   userlock--check-content-unchanged("/tmp/1.txt")
>   userlock--ask-user-about-supersession-threat("/tmp/1.txt")
>   write-region(nil nil "/tmp/1.txt")
>   (progn (insert "test") (write-region nil nil "/tmp/1.txt"))
>   (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer)))
>   (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer))))
>   (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer)))))
>   (progn (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn (insert "test") (write-region nil nil "/tmp/1.txt")) (and (buffer-name temp-buffer) (kill-buffer temp-buffer))))))
>   elisp--eval-last-sexp(nil)
>   eval-last-sexp(nil)
>   funcall-interactively(eval-last-sexp nil)
>   command-execute(eval-last-sexp)
> 
> Expected: No error is thrown.

Thanks, should be fixed now on the master branch.

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> Expected: No error is thrown.
>
> Thanks, should be fixed now on the master branch.

I confirm the fix. Thanks!

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: 64821@debbugs.gnu.org
> Date: Tue, 25 Jul 2023 08:16:23 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Expected: No error is thrown.
> >
> > Thanks, should be fixed now on the master branch.
> 
> I confirm the fix. Thanks!

Thanks for testing; closing the bug.

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Mattias Engdegård]

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

The change on master (8955853368) causes filelock-tests to fail; see attached log.
This is on macOS with an out-of-tree build.


[-- Attachment #2: filelock-tests.log --]
[-- Type: application/octet-stream, Size: 2747 bytes --]

  GEN      src/filelock-tests.log
Running 6 tests (2023-07-27 17:03:57+0200, selector `(not (or (tag :unstable) (tag :nativecomp)))')
(Shell command succeeded with no output)
Test filelock-tests-detect-external-change backtrace:
  userlock--check-content-unchanged("/private/var/folders/qy/zstv16390
  userlock--ask-user-about-supersession-threat("/private/var/folders/q
  insert("bar")
  (let ((create-lockfiles cl)) (write-region "foo" nil (buffer-file-na
  (save-current-buffer (let ((create-lockfiles cl)) (write-region "foo
  (unwind-protect (save-current-buffer (let ((create-lockfiles cl)) (w
  (progn (progn (setq buffer-file-name name) (setq buffer-file-truenam
  (unwind-protect (progn (progn (setq buffer-file-name name) (setq buf
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (let ((name (concat (file-name-as-directory temp-dir) "userfile")) (
  (progn (let ((name (concat (file-name-as-directory temp-dir) "userfi
  (unwind-protect (progn (let ((name (concat (file-name-as-directory t
  (let* ((coding-system-for-write nil) (temp-file (file-name-as-direct
  (let ((cl (car tail))) (let* ((coding-system-for-write nil) (temp-fi
  (while tail (let ((cl (car tail))) (let* ((coding-system-for-write n
  (let ((tail '(t nil))) (while tail (let ((cl (car tail))) (let* ((co
  (closure (t) nil (let* ((fn-101 #'not) (args-102 (condition-case err
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name filelock-tests-detect-external-change
  ert-run-or-rerun-test(#s(ert--stats :selector (not ...) :tests [... 
  ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
  ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
  ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
  eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
  command-line-1(("-L" ":../../emacs/test" "-l" "ert" "-l" "src/filelo
  command-line()
  normal-top-level()
Test filelock-tests-detect-external-change condition:
    (wrong-type-argument stringp nil)
   FAILED  1/6  filelock-tests-detect-external-change (0.024295 sec) at ../../emacs/test/src/filelock-tests.el:173
   passed  2/6  filelock-tests-file-locked-p-spoiled (0.001322 sec)
   passed  3/6  filelock-tests-kill-buffer-spoiled (0.000714 sec)
   passed  4/6  filelock-tests-lock-spoiled (0.000609 sec)
   passed  5/6  filelock-tests-lock-unlock-no-errors (0.001055 sec)
   passed  6/6  filelock-tests-unlock-spoiled (0.000917 sec)

Ran 6 tests, 5 results as expected, 1 unexpected (2023-07-27 17:03:58+0200, 0.274700 sec)

1 unexpected results:
   FAILED  filelock-tests-detect-external-change

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

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Thu, 27 Jul 2023 17:07:26 +0200
> Cc: Ihor Radchenko <yantar92@gmail.com>,
>  64821@debbugs.gnu.org
> 
> The change on master (8955853368) causes filelock-tests to fail; see attached log.
> This is on macOS with an out-of-tree build.

Sorry, I cannot reproduce this on any system to which I have access.
Would you mind running the failing test under Edebug and telling which
part in userlock.el signals an error, and why?  Is that this line:

          (with-current-buffer (get-file-buffer (file-truename filename))
            (set-visited-file-modtime))

If so, does it mean file-truename returns nil?

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Mattias Engdegård]

27 juli 2023 kl. 18.31 skrev Eli Zaretskii <eliz@gnu.org>:

> Is that this line:
> 
>          (with-current-buffer (get-file-buffer (file-truename filename))
>            (set-visited-file-modtime))

Right, and it's probably because ERT creates temporary files in /var/something but on macOS, /var is a symlink to /private/var. In the lines above, filename (and file-truename) is the "/private/var/..." version.
Thus get-file-buffer won't return anything because buffer-file-name contains the non-true name, "/var/...".

I have no idea if this is the right solution (probably not) but it makes filelock-tests all pass:

--- a/test/src/filelock-tests.el
+++ b/test/src/filelock-tests.el
@@ -38,8 +38,8 @@ filelock-tests--fixture
 Finally, delete the buffer and the test directory."
   (declare (debug (body)))
   `(ert-with-temp-directory temp-dir
-     (let ((name (concat (file-name-as-directory temp-dir)
-                         "userfile"))
+     (let ((name (file-truename (concat (file-name-as-directory temp-dir)
+                                        "userfile")))
            (create-lockfiles t))
        (with-temp-buffer
          (setq buffer-file-name name

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Thu, 27 Jul 2023 19:47:46 +0200
> Cc: yantar92@gmail.com,
>  64821@debbugs.gnu.org
> 
> 27 juli 2023 kl. 18.31 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Is that this line:
> > 
> >          (with-current-buffer (get-file-buffer (file-truename filename))
> >            (set-visited-file-modtime))
> 
> Right, and it's probably because ERT creates temporary files in /var/something but on macOS, /var is a symlink to /private/var. In the lines above, filename (and file-truename) is the "/private/var/..." version.
> Thus get-file-buffer won't return anything because buffer-file-name contains the non-true name, "/var/...".
> 
> I have no idea if this is the right solution (probably not) but it makes filelock-tests all pass:

Thanks, but I think we should make userlock.el robust in the face of
such applications.  How about the patch below instead?

diff --git a/lisp/userlock.el b/lisp/userlock.el
index 96de17d..92fea11 100644
--- a/lisp/userlock.el
+++ b/lisp/userlock.el
@@ -141,8 +141,10 @@ userlock--check-content-unchanged
           ;; modtime in that buffer, to cater to use case where the
           ;; file is about to be written to from some buffer that
           ;; doesn't visit any file, like a temporary buffer.
-          (with-current-buffer (get-file-buffer (file-truename filename))
-            (set-visited-file-modtime))
+          (let ((buf (get-file-buffer (file-truename filename))))
+            (when buf
+              (with-current-buffer buf
+                (set-visited-file-modtime))))
           'unchanged)))))
 
 ;;;###autoload

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Mattias Engdegård]

27 juli 2023 kl. 21.02 skrev Eli Zaretskii <eliz@gnu.org>:

> Thanks, but I think we should make userlock.el robust in the face of
> such applications.  How about the patch below instead?

That prevent the error but the test then fails since the modtime isn't actually changed.

Maybe you can set temporary-file-directory to a directory name with a symlink component and reproduce the problem on your system?

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Thu, 27 Jul 2023 21:49:12 +0200
> Cc: yantar92@gmail.com,
>  64821@debbugs.gnu.org
> 
> 27 juli 2023 kl. 21.02 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Thanks, but I think we should make userlock.el robust in the face of
> > such applications.  How about the patch below instead?
> 
> That prevent the error but the test then fails since the modtime isn't actually changed.

That's okay: it just means we need to install your suggested patch to
the test as well.  Here, done.  Can you test and see that the test now
succeeds for you?

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Mattias Engdegård]

28 juli 2023 kl. 08.40 skrev Eli Zaretskii <eliz@gnu.org>:

> That's okay: it just means we need to install your suggested patch to
> the test as well.  Here, done.  Can you test and see that the test now
> succeeds for you?

Yes, it succeeds. Thank you!

bug#64821: 30.0.50; write-region errs when overwriting an already open file

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 28 Jul 2023 11:01:12 +0200
> Cc: yantar92@gmail.com,
>  64821@debbugs.gnu.org
> 
> 28 juli 2023 kl. 08.40 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > That's okay: it just means we need to install your suggested patch to
> > the test as well.  Here, done.  Can you test and see that the test now
> > succeeds for you?
> 
> Yes, it succeeds. Thank you!

Great, thanks for testing.