bug#31981: 26.1; flymake: removes temporary file before finishing check

bug#31981: 26.1; flymake: removes temporary file before finishing check

[Enrico Scholz]

flymake gets confused and terminates itself when checking needs a long
time.  It can be reproduced with a makefile like

--- Makefile ---
check-syntax:
	sleep 2
	${MAKE} ${CHK_SOURCES}
----------------

1. start 'emacs -Q'

2. open 'foo.c' in the directory with the makefile above

3. M-x flymake-mode

4. start typing

   --> "Wait" appears in status line

5. continue typing

   --> "!" appears in status line and log reports


  | Warning [flymake-proc foo.c]: deleted file /tmp/YYY/foo_flymake.c
  | Warning [flymake-proc  *flymake output for foo.c*-919402]: Reference to file e: *** [Makefile is out of scope
  | Warning [flymake-proc  *flymake output for foo.c*-131024]: Reference to file e: *** [Makefile is out of scope
  | Warning [flymake-proc foo.c]: process 2803326 exited with code 2
  | Error [flymake-proc foo.c]: :configuration-error: Command (make -s -C ./ CHK_SOURCES=foo_flymake.c SYNTAX_CHECK_MODE=1 check-syntax) errored, but no diagnostics
  | Warning [flymake foo.c]: Disabling backend flymake-proc-legacy-flymake because :configuration-error: Command (make -s -C ./ CHK_SOURCES=foo_flymake.c SYNTAX_CHECK_MODE=1 check-syntax) errored, but no diagnostics




--------------
GNU Emacs 26.1 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 2.24.32)
 of 2018-05-31 built on koji-builder4.intern.sigma-chemnitz.de
Windowing system distributor 'Fedora Project', version 11.0.11906000
System Description:	Fedora release 28 (Twenty Eight)

...

Configured using:
 'configure --build=x86_64-redhat-linux-gnu
 --host=x86_64-redhat-linux-gnu --program-prefix=
 --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
 --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
 --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
 --libexecdir=/usr/libexec --localstatedir=/var
 --sharedstatedir=/var/lib --mandir=/usr/share/man
 --infodir=/usr/share/info --with-dbus --with-gif --with-jpeg --with-png
 --with-rsvg --with-tiff --with-xft --with-xpm --with-x-toolkit=gtk
 --with-gpm=no --with-modules build_alias=x86_64-redhat-linux-gnu
 host_alias=x86_64-redhat-linux-gnu 'CFLAGS=-DMAIL_USE_LOCKF -O2 -g
 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong
 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
 -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection'
 LDFLAGS=-Wl,-z,relro
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND DBUS GSETTINGS NOTIFY ACL
LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK2 X11 MODULES THREADS LCMS2

Important settings:
  value of $LC_COLLATE: C
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

bug#31981: Fix 'flymake-proc-legacy-flymake' temporary file deletion bug

[Joel Rosdahl]

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

I can reproduce this bug on latest master.

The problem is that if a check command takes a long time, the
following can happen:

1. The user modifies the buffer, making flymake-proc start a process A
which reads from a temporary file T containing the buffer content.
2. Before process A has finished, the user modifies the buffer again,
making flymake-proc start a new process B reading from the same
temporary file T.
3. A is marked as obsolete.
4. When A's sentinel detects that A has died, it runs A's cleanup
function which deletes T.
5. B may fail since T is gone.

Attached is a patch which solves the issue, at least for me: simply
don't run the cleanup function for an obsolete process. This should
work well for checks that use flymake-proc-simple-cleanup or one of
the other cleanup functions defined in flymake-proc.el.

I'm not sure if the fix is appropriate for the general case with a
user-supplied custom cleanup function, though. But I see no other
quick fix for the issue since the name of the temporary file is kept
in a single buffer-local variable and there is no way for the cleanup
function to know that it's being called for an obsolete process.

-- Joel

[-- Attachment #2: 0001-Fix-flymake-proc-temporary-file-deletion-bug.patch --]
[-- Type: text/x-patch, Size: 1577 bytes --]

From fef0fe5ed6cec78e87f29444af8d400bcbd01e72 Mon Sep 17 00:00:00 2001
From: Joel Rosdahl <joel@rosdahl.net>
Date: Tue, 27 Aug 2019 22:00:45 +0200
Subject: [PATCH] Fix flymake-proc temporary file deletion bug

* list/progmodes/flymake-proc.el (flymake-proc--process-sentinel):
Don't run cleanup if this is an obsolete process -- deleting the
temporary file will make the the active process fail since it uses the
same file. (Bug#31981)
---
 lisp/progmodes/flymake-proc.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el
index 2d5a47a07978..2095de6117e5 100644
--- a/lisp/progmodes/flymake-proc.el
+++ b/lisp/progmodes/flymake-proc.el
@@ -649,7 +649,12 @@ flymake-proc--process-sentinel
                (flymake-log 3 "Output buffer %s kept alive for debugging"
                             output-buffer))
               (t
-               (when (buffer-live-p source-buffer)
+               (when (and (buffer-live-p source-buffer)
+                          ;; Don't run cleanup if this is an obsolete
+                          ;; process -- deleting the temporary file
+                          ;; will make the the active process fail
+                          ;; since it uses the same file (bug#31981):
+                          (not (process-get proc 'flymake-proc--obsolete)))
                  (with-current-buffer source-buffer
                    (let ((cleanup-f (flymake-proc--get-cleanup-function
                                      (buffer-file-name))))
-- 
2.20.1

bug#31981: Fix 'flymake-proc-legacy-flymake' temporary file deletion bug

[Noam Postavsky]

Joel Rosdahl <joel@rosdahl.net> writes:

> 1. The user modifies the buffer, making flymake-proc start a process A
> which reads from a temporary file T containing the buffer content.
> 2. Before process A has finished, the user modifies the buffer again,
> making flymake-proc start a new process B reading from the same
> temporary file T.

If I'm reading flymake-proc-init-create-temp-buffer-copy correctly, it
actually makes a new temporary file (say T1), but the file name is
stored in a single variable, so Emacs effectively forgets about the
original file T.

> 3. A is marked as obsolete.
> 4. When A's sentinel detects that A has died, it runs A's cleanup
> function which deletes T.

So here it deletes T1, leaving the original T.  And then B will fail,
since it needs T1, not T.

> But I see no other quick fix for the issue since the name of the
> temporary file is kept in a single buffer-local variable and there is
> no way for the cleanup function to know that it's being called for an
> obsolete process.

Would it work to store the filename in the process object instead, with
process-put (and then the sentinel can retreive it with process-get)?

bug#31981: Fix 'flymake-proc-legacy-flymake' temporary file deletion bug

[Joel Rosdahl]

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

On Wed, 28 Aug 2019 at 01:38, Noam Postavsky <npostavs@gmail.com> wrote:
> If I'm reading flymake-proc-init-create-temp-buffer-copy correctly, it
> actually makes a new temporary file (say T1), but the file name is stored in
> a single variable, so Emacs effectively forgets about the original file T.

The create-temp-f parameter to flymake-proc-init-create-temp-buffer-copy is in
practice flymake-proc-create-temp-inplace, which always creates the same result
for a given file-name and prefix. And file-name and prefix are the same for
both modification A and B (since it's the same file). Or am I missing
something?

> So here it deletes T1, leaving the original T. And then B will fail, since it
> needs T1, not T.

Nope, it's T both times, but it's true that only generating a unique filename
wouldn't fix the issue since, as you said, the filename is stored in a single
variable.

> Would it work to store the filename in the process object instead, with
> process-put (and then the sentinel can retreive it with process-get)?

Yes, flymake-proc-create-temp-inplace could create a unique filename and store
it in the process, but that wouldn't help since the cleanup function doesn't
have access to the process (since the process is stored in a single variable as
well). Passing the process or filename(s) to the cleanup function would work,
but it would break the interface to existing custom cleanup functions which
don't take a parameter, so I wrote off that solution earlier.

...unless we can pass the value(s) using dynamic binding to the cleanup
function. I didn't consider this before since lexical binding would prevent
such a solution, but I just learnt to my surprise that global defvar-ed
variables are still dynamically scoped.

Attached is a new proposed solution as sketched above.

-- Joel

[-- Attachment #2: 0001-Fix-flymake-proc-temporary-file-deletion-bug.patch --]
[-- Type: text/x-patch, Size: 3142 bytes --]

From 81f885381343f4f08d0aef1fd0589e76f841bdb8 Mon Sep 17 00:00:00 2001
From: Joel Rosdahl <joel@rosdahl.net>
Date: Wed, 28 Aug 2019 21:00:00 +0200
Subject: [PATCH] Fix flymake-proc temporary file deletion bug

* list/progmodes/flymake-proc.el (flymake-proc-create-temp-inplace):
Include a time string part (hour + minute + second + nanosecond) in the
temporary name to make it unique enough.

* list/progmodes/flymake-proc.el (flymake-proc-legacy-flymake): Store
temporary file names in the process for usage in the sentinel.

* list/progmodes/flymake-proc.el (flymake-proc--process-sentinel): Bind
values of temporary file names dynamically to values stored in the
process so that the cleanup function will delete the correct temporary
file(s).

Fixes bug#31981.
---
 lisp/progmodes/flymake-proc.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el
index 2d5a47a07978..f08ba2f36817 100644
--- a/lisp/progmodes/flymake-proc.el
+++ b/lisp/progmodes/flymake-proc.el
@@ -654,7 +654,14 @@ flymake-proc--process-sentinel
                    (let ((cleanup-f (flymake-proc--get-cleanup-function
                                      (buffer-file-name))))
                      (flymake-log 3 "cleaning up using %s" cleanup-f)
-                     (funcall cleanup-f))))
+                     ;; Make cleanup-f see the temporary file names
+                     ;; created by its corresponding init function
+                     ;; (bug#31981).
+                     (let ((flymake-proc--temp-source-file-name
+                            (process-get proc 'flymake-proc--temp-source-file-name))
+                           (flymake-proc--temp-master-file-name
+                            (process-get proc 'flymake-proc--temp-master-file-name)))
+                       (funcall cleanup-f)))))
                (kill-buffer output-buffer)))))))
 
 (defun flymake-proc--panic (problem explanation)
@@ -824,6 +831,10 @@ flymake-proc-legacy-flymake
                   (process-put proc 'flymake-proc--output-buffer
                                (generate-new-buffer
                                 (format " *flymake output for %s*" (current-buffer))))
+                  (process-put proc 'flymake-proc--temp-source-file-name
+                               flymake-proc--temp-source-file-name)
+                  (process-put proc 'flymake-proc--temp-master-file-name
+                               flymake-proc--temp-master-file-name)
                   (setq flymake-proc--current-process proc)
                   (flymake-log 2 "started process %d, command=%s, dir=%s"
                                (process-id proc) (process-command proc)
@@ -865,6 +876,7 @@ flymake-proc-create-temp-inplace
   (let* ((ext (file-name-extension file-name))
 	 (temp-name (file-truename
 		     (concat (file-name-sans-extension file-name)
+                             "_" (format-time-string "%H%M%S%N")
 			     "_" prefix
 			     (and ext (concat "." ext))))))
     (flymake-log 3 "create-temp-inplace: file=%s temp=%s" file-name temp-name)
-- 
2.20.1

bug#31981: Fix 'flymake-proc-legacy-flymake' temporary file deletion bug

[Noam Postavsky]

Joel Rosdahl <joel@rosdahl.net> writes:

> On Wed, 28 Aug 2019 at 01:38, Noam Postavsky <npostavs@gmail.com> wrote:
>> If I'm reading flymake-proc-init-create-temp-buffer-copy correctly, it
>> actually makes a new temporary file (say T1), but the file name is stored in
>> a single variable, so Emacs effectively forgets about the original file T.
>
> The create-temp-f parameter to flymake-proc-init-create-temp-buffer-copy is in
> practice flymake-proc-create-temp-inplace, which always creates the same result
> for a given file-name and prefix. And file-name and prefix are the same for
> both modification A and B (since it's the same file). Or am I missing
> something?

No, it's just me who missed this.  There's a lot of indirection, and I
didn't read that carefully.

> but I just learnt to my surprise that global defvar-ed
> variables are still dynamically scoped.

Well, they would be unusable otherwise, since global vars don't have any
lexical scope as such.

> +                     ;; Make cleanup-f see the temporary file names
> +                     ;; created by its corresponding init function
> +                     ;; (bug#31981).
> +                     (let ((flymake-proc--temp-source-file-name
> +                            (process-get proc 'flymake-proc--temp-source-file-name))
> +                           (flymake-proc--temp-master-file-name
> +                            (process-get proc 'flymake-proc--temp-master-file-name)))
> +                       (funcall cleanup-f)))))

A little awkward, but that's proably the best we can do without changing
the signature of cleanup-f (which I guess would be much more
troublesome).

bug#31981: Fix 'flymake-proc-legacy-flymake' temporary file deletion bug

[Joel Rosdahl]

On Thu, 29 Aug 2019 at 18:05, Noam Postavsky <npostavs@gmail.com> wrote:
> A little awkward, but that's proably the best we can do without changing
> the signature of cleanup-f (which I guess would be much more
> troublesome).

I agree. Alternative solutions welcome, of course.

-- Joel

bug#31981: Fix 'flymake-proc-legacy-flymake' temporary file deletion bug

[Noam Postavsky]

tags 31981 fixed
close 31981 27.1
quit

Joel Rosdahl <joel@rosdahl.net> writes:

> On Thu, 29 Aug 2019 at 18:05, Noam Postavsky <npostavs@gmail.com> wrote:
>> A little awkward, but that's proably the best we can do without changing
>> the signature of cleanup-f (which I guess would be much more
>> troublesome).
>
> I agree. Alternative solutions welcome, of course.
>
> -- Joel

None seems forthcoming, so I've pushed your patch to master (I coalesced
your ChangeLog entries, by the way).

8d588f09e9 2019-09-07T16:51:24-04:00 "Fix flymake-proc temporary file deletion bug"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=8d588f09e91e315c715cf824a9819a538a85cd9c