bug#32201: 27.0.50; setenv should not change match-data

bug#32201: 27.0.50; setenv should not change match-data

[John Shahid]

I have been running into a weird issue lately due to a
'buffer-list-update-hook' that I added in my init.el file.  The hook is
shown below:

    (add-hook 'buffer-list-update-hook
              (lambda ()
                (when (equal major-mode 'go-mode)
                  (unless (boundp 'gopath)
                    (if-let ((root (locate-dominating-file buffer-file-name ".envrc")))
                        (setq-local gopath (expand-file-name root))
                      (setq-local gopath  nil)))
                  (and gopath
                       (save-match-data
                         (setenv "GOPATH" gopath))))))

The idea of this hook is to set some env variable based on the current
buffer.  Changing process-environment isn't sufficient since some minor
modes I'm using start processes in separate buffers.

The problem seems to happen when all of the suddent 'find-file' will
start openning weird files.  For example, if I'm currently viewing
"~/foo/bar.txt" and use 'C-x C-f' the default file name will be
"~/foo/~jvshahid/foo/bar.txt".  After some debugging it turns out that
the following will happen which cause the match-data to be corrupted:

1. find-file calls abbreviate-file-name
2. abbreviate-file-name calls (expand-file-name "~")
3. expand-file-name runs the buffer-list-update-hook (unknown why)
4. the hook will use setenv which messes up the match-data
5. abbreviate-file-name resumes and use the incorrect match-data and return an invalid path

I don't know why '3' is happening.  This usually doesn't happen in a new
Emacs session, only randomly after using Emacs for a while.  May be it
has something to do with file name handlers, the debugger doesn't show
any function calls between 'expand-file-name' and
'run-hooks(buffer-list-update-hook)'.  I also used
'(find-file-name-handler default-directory 'expand-file-name)' but got
back nil.

Usually Emacs gets in a weird state at this point and things like
'buffer-list' starts throwing errors about 'max-lisp-eval-depth'.  I
usually have to set 'buffer-list-update-hook' to 'nil' before being able
to gain control.  It is worth noting that I saw around 1000
*code-conversion-work* buffers in my Emacs session.


In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu)
 of 2018-07-03 built on f388f63bdd5d
Repository revision: 893e62ee7e3630c981adb3efa39ef409500d7657
System Description: Ubuntu 16.04.4 LTS

Recent messages:
Quit [2 times]
[mu4e] Started mu4e with 14071 messages in store
[mu4e] Contacts received: 4758
[mu4e] Found 327 matching messages
Mark saved where search started
[mu4e] Include-related turned off
[mu4e] Found 471 matching messages
Mark set
Quit [2 times]
Making completion list...

Configured using:
 'configure --without-x'

Configured features:
SOUND NOTIFY GNUTLS LIBXML2 ZLIB THREADS

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

Major mode: mu4e-headers

Minor modes in effect:
  hl-line-mode: t
  global-auto-complete-mode: t
  global-display-line-numbers-mode: t
  show-paren-mode: t
  display-time-mode: t
  display-battery-mode: t
  global-auto-revert-mode: t
  xterm-mouse-mode: t
  winner-mode: t
  flx-ido-mode: t
  straight-use-package-mode: t
  straight-package-neutering-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/jvshahid/.emacs.d/straight/build/seq/seq hides /usr/local/share/emacs/27.0.50/lisp/emacs-lisp/seq
/home/jvshahid/.emacs.d/straight/build/cl-lib/cl-lib hides /usr/local/share/emacs/27.0.50/lisp/emacs-lisp/cl-lib

Features:
(shadow sort mail-extr emacsbug misearch multi-isearch org-mu4e
org-element avl-tree generator org org-macro org-footnote org-pcomplete
pcomplete org-list org-faces org-entities noutline outline easy-mmode
org-version ob-emacs-lisp ob ob-tangle org-src ob-ref ob-lob ob-table
ob-keys ob-exp ob-comint ob-core ob-eval org-compat org-macs
org-loaddefs mu4e desktop frameset mu4e-speedbar speedbar sb-image
ezimage dframe mu4e-main mu4e-view cal-menu calendar cal-loaddefs
browse-url gnus-art mm-uu mml2015 mm-view mml-smime smime dig mailcap
gnus-sum gnus-group gnus-undo gnus-start gnus-cloud nnimap nnmail
mail-source utf7 netrc nnoo parse-time gnus-spec gnus-int gnus-range
gnus-win gnus nnheader wid-edit mu4e-headers mu4e-compose mu4e-context
mu4e-draft mu4e-actions rfc2368 smtpmail auth-source eieio eieio-core
eieio-loaddefs sendmail mu4e-mark mu4e-message flow-fill mu4e-proc
mu4e-utils doc-view jka-compr image-mode mu4e-lists mu4e-vars message
rmc puny format-spec rfc822 mml mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr
mailabbrev mail-utils gmm-utils mailheader hl-line fringe cl mu4e-meta
term/screen term/xterm xterm dired-aux server cl-seq parinfer-ext
paredit parinfer ediff-merg ediff-wind ediff-diff ediff-mult ediff-help
ediff-init ediff-util ediff mode-local parinferlib flycheck json map
find-func rx dash flymake-proc flymake mwheel compile comint regexp-opt
ansi-color warnings thingatpt auto-complete-config auto-complete popup
elec-pair rect dired-x dired dired-loaddefs display-line-numbers
time-date paren time image battery cus-start cus-load autorevert
filenotify xt-mouse edmacro kmacro winner ring flx-ido advice flx ido
seq seq-25 byte-opt edit-indirect-autoloads parinfer-autoloads
ginkgo-mode-autoloads pianobar-autoloads concourse-mode-autoloads
hierarchy-autoloads company-lsp-autoloads company-autoloads
lsp-ui-autoloads dash-functional-autoloads lsp-java-autoloads
lsp-mode-autoloads flx-ido-autoloads flx-autoloads wgrep-autoloads
ac-cider-autoloads request-autoloads helm-autoloads helm-core-autoloads
paredit-autoloads flycheck-clojure-autoloads flycheck-autoloads
cider-autoloads sesman-autoloads seq-autoloads spinner-autoloads
queue-autoloads clojure-mode-autoloads go-rename-autoloads
ace-window-autoloads avy-autoloads dockerfile-mode-autoloads s-autoloads
yasnippet-snippets-autoloads yasnippet-autoloads
go-autocomplete-autoloads auto-complete-autoloads go-eldoc-autoloads
protobuf-mode-autoloads markdown-mode-autoloads go-guru-autoloads
yaml-mode-autoloads etags-select-autoloads magit-autoloads
magit-popup-autoloads git-commit-autoloads with-editor-autoloads
ghub-autoloads dash-autoloads async-autoloads projectile-autoloads
pkg-info-autoloads epl-autoloads go-mode-autoloads debbugs-autoloads
finder-inf popup-autoloads cl-lib-autoloads straight-autoloads info
tool-bar cl-extra help-mode easymenu straight subr-x straight-compat
cl-macs gv cl-loaddefs cl-lib bytecomp byte-compile cconv mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan thai
tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian
slovak czech european ethiopic indian cyrillic chinese composite
charscript charprop case-table epa-hook jka-cmpr-hook help simple abbrev
obarray minibuffer cl-preloaded nadvice loaddefs button faces cus-face
macroexp files text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget hashtable-print-readable backquote inotify
multi-tty make-network-process emacs)

Memory information:
((conses 16 364700 312163)
 (symbols 48 41281 1)
 (miscs 40 121 416)
 (strings 32 120184 49417)
 (string-bytes 1 4054304)
 (vectors 16 40431)
 (vector-slots 8 802992 291932)
 (floats 8 332 1307)
 (intervals 56 4741 8246)
 (buffers 992 17)
 (heap 1024 62832 40993))

bug#32201: Acknowledgement (27.0.50; setenv should not change match-data)

[John Shahid]

I incorrectly attached a snippet of the hook after I fixed it by
wrapping the call to 'setenv' in 'save-match-data'.  The previous
version that exhibit the problem didn't have 'save-match-data'

GNU bug Tracking System <help-debbugs@gnu.org> writes:

> Thank you for filing a new bug report with debbugs.gnu.org.
>
> This is an automatically generated reply to let you know your message
> has been received.
>
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
>
> Your message has been sent to the package maintainer(s):
>  bug-gnu-emacs@gnu.org
>
> If you wish to submit further information on this problem, please
> send it to 32201@debbugs.gnu.org.
>
> Please do not send mail to help-debbugs@gnu.org unless you wish
> to report a problem with the Bug-tracking system.

bug#32201: 27.0.50; setenv should not change match-data

[Noam Postavsky]

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

tags 32201 + patch
quit

John Shahid <jvshahid@gmail.com> writes:
>
>     (add-hook 'buffer-list-update-hook
>               (lambda ()

>                          (setenv "GOPATH" gopath))))))

> The problem seems to happen when all of the suddent 'find-file' will
> start openning weird files.  For example, if I'm currently viewing
> "~/foo/bar.txt" and use 'C-x C-f' the default file name will be
> "~/foo/~jvshahid/foo/bar.txt".  After some debugging it turns out that
> the following will happen which cause the match-data to be corrupted:
>
> 1. find-file calls abbreviate-file-name
> 2. abbreviate-file-name calls (expand-file-name "~")
> 3. expand-file-name runs the buffer-list-update-hook (unknown why)
> 4. the hook will use setenv which messes up the match-data
> 5. abbreviate-file-name resumes and use the incorrect match-data and return an invalid path
>
> I don't know why '3' is happening.

buffer-list-update-hook gets called by get-buffer-create and
kill-buffer; it seems plausible that some file name handlers would use
make-temp-buffer which calls both of those.

Anyway, I don't think setenv should be changed, rather
abbreviate-file-name should save-match-data around the expand-file-name
call.  After all, today you happened to use setenv in a hook, tomorrow
someone will use another match-data modifying function.

Here's the patch (intended for emacs-26):


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

From a0eec7f2e672804f3a7a30e55c821ba2dac213b7 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 18 Jul 2018 18:45:47 -0400
Subject: [PATCH v1] Add save-match-data to abbreviate-file-name (Bug#32201)

* lisp/files.el (abbreviate-file-name): Save match-data around
expand-file-name; it is not guaranteed to preserve match-data, and may
well do so depending on what file handlers and hooks are in effect.
---
 lisp/files.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index fb8c34bcae..4eb1560a20 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1929,7 +1929,7 @@ abbreviate-file-name
 			 (save-match-data
 			   (string-match "^[a-zA-`]:/$" filename))))
                (equal (get 'abbreviated-home-dir 'home)
-                      (expand-file-name "~")))
+                      (save-match-data (expand-file-name "~"))))
 	  (setq filename
 		(concat "~"
 			(match-string 1 filename)
-- 
2.11.0

bug#32201: 27.0.50; setenv should not change match-data

[John Shahid]

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


Noam Postavsky <npostavs@gmail.com> writes:

[...]

> Anyway, I don't think setenv should be changed, rather
> abbreviate-file-name should save-match-data around the expand-file-name
> call.  After all, today you happened to use setenv in a hook, tomorrow
> someone will use another match-data modifying function.

I agree, but I also think that there is no reason for `setenv' to use
`string-match' instead of `string-match-p'.  It doesn't seem to be using
the match data anyway.  I attached the patch that replaces
`string-match' with `string-match-p' in `setenv' and `setenv-internal'.
WDYT ?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-destroying-match-data-in-setenv.patch --]
[-- Type: text/x-diff, Size: 1410 bytes --]

From 7b592252f698ae07a2010302c74b42026fbbca5e Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid@gmail.com>
Date: Wed, 18 Jul 2018 20:18:19 -0400
Subject: [PATCH] Avoid destroying match data in 'setenv'

* lisp/env.el (setenv,setenv-internal): Replace string-match with
  string-match-p
---
 lisp/env.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/env.el b/lisp/env.el
index e47eb57836..7007ba33e5 100644
--- a/lisp/env.el
+++ b/lisp/env.el
@@ -113,11 +113,11 @@ setenv-internal
 	     (not keep-empty)
 	     env
 	     (stringp (car env))
-	     (string-match pattern (car env)))
+             (string-match-p pattern (car env)))
 	(cdr env)
       ;; Try to find existing entry for VARIABLE in ENV.
       (while (and scan (stringp (car scan)))
-	(when (string-match pattern (car scan))
+        (when (string-match-p pattern (car scan))
 	  (if value
 	      (setcar scan (concat variable "=" value))
 	    (if keep-empty
@@ -184,7 +184,7 @@ setenv
       (setq variable (encode-coding-string variable locale-coding-system)))
   (if (and value (multibyte-string-p value))
       (setq value (encode-coding-string value locale-coding-system)))
-  (if (string-match "=" variable)
+  (if (string-match-p "=" variable)
       (error "Environment variable name `%s' contains `='" variable))
   (if (string-equal "TZ" variable)
       (set-time-zone-rule value))
-- 
2.18.0

bug#32201: 27.0.50; setenv should not change match-data

[Noam Postavsky]

John Shahid <jvshahid@gmail.com> writes:

>> Anyway, I don't think setenv should be changed, rather
>> abbreviate-file-name should save-match-data around the expand-file-name
>> call.  After all, today you happened to use setenv in a hook, tomorrow
>> someone will use another match-data modifying function.
>
> I agree, but I also think that there is no reason for `setenv' to use
> `string-match' instead of `string-match-p'.  It doesn't seem to be using
> the match data anyway.  I attached the patch that replaces
> `string-match' with `string-match-p' in `setenv' and `setenv-internal'.
> WDYT ?

Seems harmless enough.  I'll wait a couple of days, and then push my
abbreviate-file-name patch to emacs-26 and this patch to master.

bug#32201: 27.0.50; setenv should not change match-data

[Noam Postavsky]

tags 32201 fixed
close 32201 26.2
quit

Noam Postavsky <npostavs@gmail.com> writes:

> John Shahid <jvshahid@gmail.com> writes:
>
>>> Anyway, I don't think setenv should be changed, rather
>>> abbreviate-file-name should save-match-data around the expand-file-name
>>> call.  After all, today you happened to use setenv in a hook, tomorrow
>>> someone will use another match-data modifying function.
>>
>> I agree, but I also think that there is no reason for `setenv' to use
>> `string-match' instead of `string-match-p'.  It doesn't seem to be using
>> the match data anyway.  I attached the patch that replaces
>> `string-match' with `string-match-p' in `setenv' and `setenv-internal'.
>> WDYT ?
>
> Seems harmless enough.  I'll wait a couple of days, and then push my
> abbreviate-file-name patch to emacs-26 and this patch to master.

Done.

[1: 59e8533286]: 2018-07-21 21:07:07 -0400
  Add save-match-data to abbreviate-file-name (Bug#32201)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=59e8533286cc8b5abc80b0966ef4b9fb676fbdfe

[2: b7ca3d5d93]: 2018-07-21 21:10:20 -0400
  Avoid destroying match data in 'setenv' (Bug#32201)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b7ca3d5d932bad6900296679ab87f7d0d64d1de9