bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

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

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

Package: Emacs
Version: 30.0.50


If I open a file `foo.tex` with a local variable setting of
`TeX-master: "paper.tex"` and that `paper.tex` file has a local
variable setting of `TeX-master: t`, I get the funny behavior that
Emacs first asks me whether to obey the `TeX-master: t` setting of
`paper.tex` before asking me whether to obey the `TeX-master:
"paper.tex"` setting of `foo.tex`, even though it obviously had to use
the `TeX-master: "paper.tex"` setting in order to decide to open the
`paper.tex` file (and ask me about its `TeX-master: t`).

The corresponding backtrace looks as below:

    Debugger entered--Lisp error: (minibuffer-quit)
      #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_57>()
      read-char-from-minibuffer("Please type y, n, ! or i, or C-v/M-v to scroll: " (33 105 121 110 32))
      read-char-choice("Please type y, n, ! or i, or C-v/M-v to scroll: " (33 105 121 110 32))
      hack-local-variables-confirm(((TeX-master . t)) ((TeX-master . t)) nil nil)
      hack-local-variables-filter(((TeX-master . t)) nil)
      hack-local-variables(no-mode)
      run-mode-hooks(latex-mode-hook)
      latex-mode()
      set-auto-mode-0(latex-mode nil)
      set-auto-mode()
      normal-mode(t)
      after-find-file(nil nil)
      find-file-noselect-1(#<buffer paper.tex> ".../paper.tex" t nil ".../paper.tex" (3064599 65026))
      find-file-noselect("paper.tex" t)
      latexenc-find-file-coding-system((insert-file-contents ".../foo.tex" t nil nil nil))
      insert-file-contents(".../foo.tex" t)
      find-file-noselect-1(#<buffer foo.tex> ".../foo.tex" nil nil ".../foo.tex" (3064581 65026))
      find-file-noselect(".../foo.tex")
      command-line-1((".../foo.tex"))
      command-line()
      normal-top-level()

showing that the problem is that `latexenc-find-file-coding-system`
is the function that opens `paper.tex` before the users had a chance to
confirm that they think this is safe.

I suggest the patch below which makes `latexenc-find-file-coding-system`
use `safe-local-variable-p` before using a file-local setting, and also
adds corresponding `safe-local-variable` settings for `TeX-master` and
`tex-main-file`.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tex-master.diff --]
[-- Type: text/x-diff, Size: 2519 bytes --]

diff --git a/lisp/international/latexenc.el b/lisp/international/latexenc.el
index 6e2306449bc..66e3faa37b9 100644
--- a/lisp/international/latexenc.el
+++ b/lisp/international/latexenc.el
@@ -155,14 +155,16 @@ latexenc-find-file-coding-system
               (when (re-search-forward
                      "^%+ *\\(TeX-master\\|tex-main-file\\): *\"\\(.+\\)\""
                      nil t)
-                (let ((file (match-string 2)))
-                  (dolist (ext `("" ,(if (boundp 'TeX-default-extension)
-                                         (concat "." TeX-default-extension)
-                                       "")
-                                 ".tex" ".ltx" ".dtx" ".drv"))
-                    (if (and (null latexenc-main-file) ;Stop at first.
-                             (file-exists-p (concat file ext)))
-                        (setq latexenc-main-file (concat file ext)))))))
+                (let ((var (match-string 1))
+                      (file (match-string 2)))
+                  (when (safe-local-variable-p (intern var) file)
+                    (dolist (ext `("" ,(if (boundp 'TeX-default-extension)
+                                           (concat "." TeX-default-extension)
+                                         "")
+                                   ".tex" ".ltx" ".dtx" ".drv"))
+                      (if (and (null latexenc-main-file) ;Stop at first.
+                               (file-exists-p (concat file ext)))
+                          (setq latexenc-main-file (concat file ext))))))))
             ;; try tex-modes tex-guess-main-file
             (when (and (not latexenc-dont-use-tex-guess-main-file-flag)
                        (not latexenc-main-file))
diff --git a/lisp/textmodes/tex-mode.el b/lisp/textmodes/tex-mode.el
index 02ee1242c72..e7b1522751f 100644
--- a/lisp/textmodes/tex-mode.el
+++ b/lisp/textmodes/tex-mode.el
@@ -89,6 +89,7 @@ tex-main-file
 if the variable is non-nil."
   :type '(choice (const :tag "None" nil)
                  file)
+  :safe #'stringp
   :group 'tex-file)
 
 ;;;###autoload
@@ -2213,6 +2214,10 @@ tex-guess-main-file
 			  header-re (+ (point) 10000) t))))
 	    (throw 'found (expand-file-name buffer-file-name))))))))
 
+(unless (get 'TeX-master 'safe-local-variable) ;Don't override AUCTeX's setting.
+  (put 'TeX-master 'safe-local-variable
+       (lambda (x) (or (booleanp x) (stringp x)))))
+
 (defun tex-main-file ()
   "Return the relative name of the main file."
   (let* ((file (or tex-main-file

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

[Arash Esbati]

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

> diff --git a/lisp/textmodes/tex-mode.el b/lisp/textmodes/tex-mode.el
> index 02ee1242c72..e7b1522751f 100644
> --- a/lisp/textmodes/tex-mode.el
> +++ b/lisp/textmodes/tex-mode.el
> @@ -89,6 +89,7 @@ tex-main-file
>  if the variable is non-nil."
>    :type '(choice (const :tag "None" nil)
>                   file)
> +  :safe #'stringp
>    :group 'tex-file)
>  
>  ;;;###autoload
> @@ -2213,6 +2214,10 @@ tex-guess-main-file
>  			  header-re (+ (point) 10000) t))))
>  	    (throw 'found (expand-file-name buffer-file-name))))))))
>  
> +(unless (get 'TeX-master 'safe-local-variable) ;Don't override AUCTeX's setting.
> +  (put 'TeX-master 'safe-local-variable
> +       (lambda (x) (or (booleanp x) (stringp x)))))
> +

Is there a reason why leave out the values `dwim' and `shared'?  tex.el
defines `TeX-master' like this:

  (defcustom TeX-master t
    :safe (lambda (x)
            (or (stringp x)
                (member x (quote (t nil shared dwim))))))

Best, Arash

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca
> Date: Mon, 15 Apr 2024 18:42:36 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> If I open a file `foo.tex` with a local variable setting of
> `TeX-master: "paper.tex"` and that `paper.tex` file has a local
> variable setting of `TeX-master: t`, I get the funny behavior that
> Emacs first asks me whether to obey the `TeX-master: t` setting of
> `paper.tex` before asking me whether to obey the `TeX-master:
> "paper.tex"` setting of `foo.tex`, even though it obviously had to use
> the `TeX-master: "paper.tex"` setting in order to decide to open the
> `paper.tex` file (and ask me about its `TeX-master: t`).
> 
> The corresponding backtrace looks as below:
> 
>     Debugger entered--Lisp error: (minibuffer-quit)
>       #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_57>()
>       read-char-from-minibuffer("Please type y, n, ! or i, or C-v/M-v to scroll: " (33 105 121 110 32))
>       read-char-choice("Please type y, n, ! or i, or C-v/M-v to scroll: " (33 105 121 110 32))
>       hack-local-variables-confirm(((TeX-master . t)) ((TeX-master . t)) nil nil)
>       hack-local-variables-filter(((TeX-master . t)) nil)
>       hack-local-variables(no-mode)
>       run-mode-hooks(latex-mode-hook)
>       latex-mode()
>       set-auto-mode-0(latex-mode nil)
>       set-auto-mode()
>       normal-mode(t)
>       after-find-file(nil nil)
>       find-file-noselect-1(#<buffer paper.tex> ".../paper.tex" t nil ".../paper.tex" (3064599 65026))
>       find-file-noselect("paper.tex" t)
>       latexenc-find-file-coding-system((insert-file-contents ".../foo.tex" t nil nil nil))
>       insert-file-contents(".../foo.tex" t)
>       find-file-noselect-1(#<buffer foo.tex> ".../foo.tex" nil nil ".../foo.tex" (3064581 65026))
>       find-file-noselect(".../foo.tex")
>       command-line-1((".../foo.tex"))
>       command-line()
>       normal-top-level()
> 
> showing that the problem is that `latexenc-find-file-coding-system`
> is the function that opens `paper.tex` before the users had a chance to
> confirm that they think this is safe.

Is it correct for latexenc-find-file-coding-system to use
find-file-noselect for this purpose?  Why does it call
insert-file-contents with 2nd arg non-nil, if all it needs is to find
and process the encoding spec there?

Alternatively, we could disable local-variable processing when calling
latexenc-find-file-coding-system.  WDYT?

> I suggest the patch below which makes `latexenc-find-file-coding-system`
> use `safe-local-variable-p` before using a file-local setting, and also
> adds corresponding `safe-local-variable` settings for `TeX-master` and
> `tex-main-file`.

Is it really guaranteed that safe local variables will never cause
similar problems?  That they are safe doesn't mean they must be
processed at this point.

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

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

> Is it correct for latexenc-find-file-coding-system to use
> find-file-noselect for this purpose?

That's another question.  My question is instead whether it's safe for
`latexenc-find-file-coding-system` to use the `TeX-master` setting in
foo.tex`.  I can imagine cases where that setting could be dangerous
because it tricks the users into accessing a file they shouldn't access.

> Is it really guaranteed that safe local variables will never cause
> similar problems?

No, but at least it gives the users a standard way to control whether to
consider it safe or not.


        Stefan

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

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

> Is there a reason why leave out the values `dwim' and `shared'?

Because `tex-mode.el` doesn't use/support those values.
Not a strong reason, admittedly.


        Stefan

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 70409@debbugs.gnu.org
> Date: Tue, 16 Apr 2024 08:53:13 -0400
> 
> > Is it correct for latexenc-find-file-coding-system to use
> > find-file-noselect for this purpose?
> 
> That's another question.  My question is instead whether it's safe for
> `latexenc-find-file-coding-system` to use the `TeX-master` setting in
> foo.tex`.  I can imagine cases where that setting could be dangerous
> because it tricks the users into accessing a file they shouldn't access.

But wouldn't processing the file in a "more literal" manner solve that
problem as well?  We only process file-local variables when we turn on
the proper major mode, so avoiding to turn on a mode will also solve
the problem you have, including unsafe variables.  And finding the
telltale signature of the file's encoding doesn't need any major
mode's help, does it?

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

[Arash Esbati]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Because `tex-mode.el` doesn't use/support those values.
> Not a strong reason, admittedly.

Thanks, then it's not necessary to cater for them, I think.

Best, Arash

bug#70409: 30.0.50; `latexenc-find-file-coding-system` uses `TeX-master` before we know it's safe

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

>> > Is it correct for latexenc-find-file-coding-system to use
>> > find-file-noselect for this purpose?
>> That's another question.  My question is instead whether it's safe for
>> `latexenc-find-file-coding-system` to use the `TeX-master` setting in
>> foo.tex`.  I can imagine cases where that setting could be dangerous
>> because it tricks the users into accessing a file they shouldn't access.
> But wouldn't processing the file in a "more literal" manner solve that
> problem as well?

Could be.  I'm just uncomfortable with the idea that we make use of the
`TeX-master: "paper.tex"` local-variable setting before we even bother
to check whether it obeys the usual `safe-local-variable-p` checks (and
even if `enable-local-variables` is nil, AFAICT).


        Stefan