bug#61668: Bug in flymake-proc with fix

bug#61668: Bug in flymake-proc with fix

[Camden Narzt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

I’m sorry if this is not the correct place to report a flymake-proc bug, but since flymake is included in the emacs git repo I figured it might be ok.

I’m currently seeing incorrect behaviour from the `flymake-proc--delete-temp-directory` function. The path is parsed and then reassembled incorrectly as the following backtrace extract demonstrates:

flymake-proc--safe-delete-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv")
flymake-proc--delete-temp-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“)

As you can see the `000gn/T/` segment of the path gets duplicated when `flymake-proc--safe-delete-directory` gets called.

This is because in `flymake-proc--delete-temp-directory` when the `suffix` variable is declared it is assumed that `(directory-file-name temporary-file-directory)` is a prefix of the `dir-name` argument, however `(directory-file-name temporary-file-directory)` doesn’t seem to resolve symlinks in the path whereas `dir-name` seems to have symlinks already resolved, so they don’t necessarily match. On my system the difference is:

(directory-file-name temporary-file-directory) → "/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T”
dir-name → "/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“

Note the "/private” prefix on the `dir-name` path. That difference in length causes the suffix to be incorrectly determined and then causes a bunch of errors while any subsequent function call tries to work with a path with the `000gn/T/` segment duplicated which obviously doesn’t exist in the fs.

Changing the suffix variable to be computed as follows fixes the bug:

(substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))


If this can be fixed that’s great, if there’s somewhere else I should report this I’d love to know.

Cheers,

Camden Narzt

bug#61668: Bug in flymake-proc with fix

[Eli Zaretskii]

Adding João.

> Date: Mon, 20 Feb 2023 11:23:02 -0700
> From:  Camden Narzt via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I’m sorry if this is not the correct place to report a flymake-proc bug, but since flymake is included in the emacs git repo I figured it might be ok.
> 
> I’m currently seeing incorrect behaviour from the `flymake-proc--delete-temp-directory` function. The path is parsed and then reassembled incorrectly as the following backtrace extract demonstrates:
> 
> flymake-proc--safe-delete-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv")
> flymake-proc--delete-temp-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“)
> 
> As you can see the `000gn/T/` segment of the path gets duplicated when `flymake-proc--safe-delete-directory` gets called.
> 
> This is because in `flymake-proc--delete-temp-directory` when the `suffix` variable is declared it is assumed that `(directory-file-name temporary-file-directory)` is a prefix of the `dir-name` argument, however `(directory-file-name temporary-file-directory)` doesn’t seem to resolve symlinks in the path whereas `dir-name` seems to have symlinks already resolved, so they don’t necessarily match. On my system the difference is:
> 
> (directory-file-name temporary-file-directory) → "/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T”
> dir-name → "/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“
> 
> Note the "/private” prefix on the `dir-name` path. That difference in length causes the suffix to be incorrectly determined and then causes a bunch of errors while any subsequent function call tries to work with a path with the `000gn/T/` segment duplicated which obviously doesn’t exist in the fs.
> 
> Changing the suffix variable to be computed as follows fixes the bug:
> 
> (substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))
> 
> 
> If this can be fixed that’s great, if there’s somewhere else I should report this I’d love to know.
> 
> Cheers,
> 
> Camden Narzt
> 
> 
> 

bug#61668: Bug in flymake-proc with fix

[João Távora]

Hi Camden,

Flymake-proc is a deprecated part of Flymake.  I haven't used or tested
it in a long time (there were never automated tests for it).  When I
redesigned Flymake years ago, in which flymake-proc became another
backend, I tested some basic use cases, but I didn't change the
underlying code, including the directory calculating parts and helpers.

In other words, this is not "my code" and I never understood it.

So if you think the change makes sense, go for it i.e. provide a patch
that we can push. I think there are very few users of this backend
anyway.

João

On Tue, Feb 21, 2023 at 12:35 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Adding João.
>
> > Date: Mon, 20 Feb 2023 11:23:02 -0700
> > From:  Camden Narzt via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >
> > I’m sorry if this is not the correct place to report a flymake-proc bug, but since flymake is included in the emacs git repo I figured it might be ok.
> >
> > I’m currently seeing incorrect behaviour from the `flymake-proc--delete-temp-directory` function. The path is parsed and then reassembled incorrectly as the following backtrace extract demonstrates:
> >
> > flymake-proc--safe-delete-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv")
> > flymake-proc--delete-temp-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“)
> >
> > As you can see the `000gn/T/` segment of the path gets duplicated when `flymake-proc--safe-delete-directory` gets called.
> >
> > This is because in `flymake-proc--delete-temp-directory` when the `suffix` variable is declared it is assumed that `(directory-file-name temporary-file-directory)` is a prefix of the `dir-name` argument, however `(directory-file-name temporary-file-directory)` doesn’t seem to resolve symlinks in the path whereas `dir-name` seems to have symlinks already resolved, so they don’t necessarily match. On my system the difference is:
> >
> > (directory-file-name temporary-file-directory) → "/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T”
> > dir-name → "/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“
> >
> > Note the "/private” prefix on the `dir-name` path. That difference in length causes the suffix to be incorrectly determined and then causes a bunch of errors while any subsequent function call tries to work with a path with the `000gn/T/` segment duplicated which obviously doesn’t exist in the fs.
> >
> > Changing the suffix variable to be computed as follows fixes the bug:
> >
> > (substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))
> >
> >
> > If this can be fixed that’s great, if there’s somewhere else I should report this I’d love to know.
> >
> > Cheers,
> >
> > Camden Narzt
> >
> >
> >



-- 
João Távora

bug#61668: Bug in flymake-proc with fix

[Camden Narzt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi João,

Yeah I don’t use flymake-proc directly, I think lsp-mode is using it or something. Anyway I’ve attached a patch.

[-- Attachment #2: flymake-proc.patch --]
[-- Type: application/octet-stream, Size: 579 bytes --]

--- flymake-proc.el.old	2023-02-21 09:12:10
+++ flymake-proc.el.new	2023-02-21 09:11:43
@@ -904,7 +904,7 @@
 (defun flymake-proc--delete-temp-directory (dir-name)
   "Attempt to delete temp dir DIR-NAME, do not fail on error."
   (let* ((temp-dir    temporary-file-directory)
-         (suffix      (substring dir-name (1+ (length (directory-file-name temp-dir))))))
+         (suffix      (substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))))
 
     (while (> (length suffix) 0)
       (setq suffix (directory-file-name suffix))

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



Cheers,

Camden


> On Feb 21, 2023, at 5:42 AM, João Távora <joaotavora@gmail.com> wrote:
> 
> Hi Camden,
> 
> Flymake-proc is a deprecated part of Flymake.  I haven't used or tested
> it in a long time (there were never automated tests for it).  When I
> redesigned Flymake years ago, in which flymake-proc became another
> backend, I tested some basic use cases, but I didn't change the
> underlying code, including the directory calculating parts and helpers.
> 
> In other words, this is not "my code" and I never understood it.
> 
> So if you think the change makes sense, go for it i.e. provide a patch
> that we can push. I think there are very few users of this backend
> anyway.
> 
> João
> 
> On Tue, Feb 21, 2023 at 12:35 PM Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>> Adding João.
>> 
>>> Date: Mon, 20 Feb 2023 11:23:02 -0700
>>> From:  Camden Narzt via "Bug reports for GNU Emacs,
>>> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>> 
>>> I’m sorry if this is not the correct place to report a flymake-proc bug, but since flymake is included in the emacs git repo I figured it might be ok.
>>> 
>>> I’m currently seeing incorrect behaviour from the `flymake-proc--delete-temp-directory` function. The path is parsed and then reassembled incorrectly as the following backtrace extract demonstrates:
>>> 
>>> flymake-proc--safe-delete-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv")
>>> flymake-proc--delete-temp-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“)
>>> 
>>> As you can see the `000gn/T/` segment of the path gets duplicated when `flymake-proc--safe-delete-directory` gets called.
>>> 
>>> This is because in `flymake-proc--delete-temp-directory` when the `suffix` variable is declared it is assumed that `(directory-file-name temporary-file-directory)` is a prefix of the `dir-name` argument, however `(directory-file-name temporary-file-directory)` doesn’t seem to resolve symlinks in the path whereas `dir-name` seems to have symlinks already resolved, so they don’t necessarily match. On my system the difference is:
>>> 
>>> (directory-file-name temporary-file-directory) → "/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T”
>>> dir-name → "/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“
>>> 
>>> Note the "/private” prefix on the `dir-name` path. That difference in length causes the suffix to be incorrectly determined and then causes a bunch of errors while any subsequent function call tries to work with a path with the `000gn/T/` segment duplicated which obviously doesn’t exist in the fs.
>>> 
>>> Changing the suffix variable to be computed as follows fixes the bug:
>>> 
>>> (substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))
>>> 
>>> 
>>> If this can be fixed that’s great, if there’s somewhere else I should report this I’d love to know.
>>> 
>>> Cheers,
>>> 
>>> Camden Narzt
>>> 
>>> 
>>> 
> 
> 
> 
> -- 
> João Távora

bug#61668: Bug in flymake-proc with fix

[Stefan Kangas]

tags 61668 + patch
thanks

Camden Narzt via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Yeah I don’t use flymake-proc directly, I think lsp-mode is using it
> or something. Anyway I’ve attached a patch.

I would like to install this patch.

Could you please send the patch as an attachment instead?  We prefer
that patches are created with a command like `git format-patch -1'.
Please also include the bug number of this bug in the commit message, if
possible, like so: Bug#61668.

That would make it easier for us to review and install this patch.

Thanks in advance.

bug#61668: Bug in flymake-proc with fix

[Camden Narzt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

I’ll try my best.

I’ve attached a patch I made by:
cloning https://git.savannah.gnu.org/git/emacs.git 
making the change
committing the change with a message mentioning the bug number
Running `git format-patch -1` to create the patch

Hopefully it is useful to you.

Cheers,

Camden



> On Oct 8, 2023, at 10:35 AM, Stefan Kangas <stefankangas@gmail.com> wrote:
> 
> tags 61668 + patch
> thanks
> 
> Camden Narzt via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
> 
>> Yeah I don’t use flymake-proc directly, I think lsp-mode is using it
>> or something. Anyway I’ve attached a patch.
> 
> I would like to install this patch.
> 
> Could you please send the patch as an attachment instead?  We prefer
> that patches are created with a command like `git format-patch -1'.
> Please also include the bug number of this bug in the commit message, if
> possible, like so: Bug#61668.
> 
> That would make it easier for us to review and install this patch.
> 
> Thanks in advance.


[-- Attachment #2.1: Type: text/html, Size: 746 bytes --]

[-- Attachment #2.2: 0001-Fix-for-Bug-61668-resolve-symlinks-in-temp-dir-path.patch --]
[-- Type: application/octet-stream, Size: 1093 bytes --]

From 5e787db2d3fd11bd23be8882804c99d45d464c9d Mon Sep 17 00:00:00 2001
From: Camden Narzt <c.narzt@me.com>
Date: Sun, 8 Oct 2023 13:47:56 -0600
Subject: [PATCH] Fix for Bug#61668: resolve symlinks in temp dir path

---
 lisp/progmodes/flymake-proc.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/progmodes/flymake-proc.el b/lisp/progmodes/flymake-proc.el
index 2e9c28a92d5..2e4995ed7fc 100644
--- a/lisp/progmodes/flymake-proc.el
+++ b/lisp/progmodes/flymake-proc.el
@@ -904,7 +904,7 @@ can also be executed interactively independently of
 (defun flymake-proc--delete-temp-directory (dir-name)
   "Attempt to delete temp dir DIR-NAME, do not fail on error."
   (let* ((temp-dir    (file-truename temporary-file-directory))
-	 (suffix      (substring dir-name (1+ (length (directory-file-name temp-dir))))))
+	 (suffix      (substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))))
 
     (while (> (length suffix) 0)
       (setq suffix (directory-file-name suffix))
-- 
2.39.3 (Apple Git-145)


[-- Attachment #2.3: Type: text/html, Size: 1160 bytes --]

bug#61668: Bug in flymake-proc with fix

[Stefan Kangas]

Camden Narzt via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> I’m currently seeing incorrect behaviour from the `flymake-proc--delete-temp-directory` function. The path is parsed and then reassembled incorrectly as the following backtrace extract demonstrates:
>
> flymake-proc--safe-delete-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv")
> flymake-proc--delete-temp-directory("/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“)
>
> As you can see the `000gn/T/` segment of the path gets duplicated when `flymake-proc--safe-delete-directory` gets called.
>
> This is because in `flymake-proc--delete-temp-directory` when the `suffix` variable is declared it is assumed that `(directory-file-name temporary-file-directory)` is a prefix of the `dir-name` argument, however `(directory-file-name temporary-file-directory)` doesn’t seem to resolve symlinks in the path whereas `dir-name` seems to have symlinks already resolved, so they don’t necessarily match. On my system the difference is:
>
> (directory-file-name temporary-file-directory) → "/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T”
> dir-name → "/private/var/folders/p7/03_g5t611499lmjqhwc5tljr0000gn/T/Users/camdennarzt/Developer/Java/getargv.java/src/main/java/cam/narzt/getargv/“
>
> Note the "/private” prefix on the `dir-name` path. That difference in length causes the suffix to be incorrectly determined and then causes a bunch of errors while any subsequent function call tries to work with a path with the `000gn/T/` segment duplicated which obviously doesn’t exist in the fs.
>
> Changing the suffix variable to be computed as follows fixes the bug:
>
> (substring dir-name (1+ (length (file-truename (expand-file-name (directory-file-name temp-dir))))))

Taking a closer look, I'm not yet sure about the proposed fix.  First,
it seems like you're using macOS, which you did not mention in your bug
report.  Which version of macOS are you using?

Reducing the code down to something more minimal, I see this on my
macOS 12.7 machine.  The original code produces something like this:

    (let* ((temp-dir (file-truename temporary-file-directory))
           (dir (directory-file-name temp-dir)))
      (cons dir (length dir)))
    => ("/private/var/folders/pj/rhx0gxy15tv3vx6l3mdy0qvm0000gn/T" . 56)

This is your fix---i.e. (file-truename (expand-file-name ...))---and I
get the exact same result here:

    (let* ((temp-dir (file-truename temporary-file-directory))
           (dir (file-truename (expand-file-name
                                (directory-file-name temp-dir)))))
      (cons dir (length dir)))
    => ("/private/var/folders/pj/rhx0gxy15tv3vx6l3mdy0qvm0000gn/T" . 56)

I'm also seeing the same result if I skip the call to
`expand-file-name':

    (let* ((temp-dir (file-truename temporary-file-directory))
           (dir (file-truename (directory-file-name temp-dir))))
      (cons dir (length dir)))
    => ("/private/var/folders/pj/rhx0gxy15tv3vx6l3mdy0qvm0000gn/T" . 56)

What do the above forms evaluate to on your machine?

Thanks.

bug#61668: Bug in flymake-proc with fix

[Stefan Kangas]

unarchive 46203
forcemerge 46203 61668
thanks

[ Please in the future use "Reply to all" so that the discussion is
 recorded in the bug tracker. ]

Camden Narzt <c.narzt@me.com> writes:

> I looked at why you weren’t seeing the bug I reported, and honestly I
> don’t know, it looks like Lars Ingebrigtsen fixed this already in
> 9225599c. I don’t know why I was seeing the behaviour I was seeing.
>
> Cheers,
>
> Camden

Ah, that makes sense.  Thanks.

I'm merging this bug with #46203 then, which should also close this bug.