GNU bug report logs - #32201
27.0.50; setenv should not change match-data

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32201 in the body.
You can then email your comments to 32201 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: John Shahid <jvshahid <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; setenv should not change match-data
Date: Wed, 18 Jul 2018 19:18:33 +0000

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))

Message #8 received at 32201 <at> debbugs.gnu.org (full text, mbox):

From: John Shahid <jvshahid <at> gmail.com>
To: 32201 <at> debbugs.gnu.org
Subject: Re: bug#32201: Acknowledgement (27.0.50;
 setenv should not change match-data)
Date: Wed, 18 Jul 2018 19:25:26 +0000

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 <at> 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 <at> gnu.org
>
> If you wish to submit further information on this problem, please
> send it to 32201 <at> debbugs.gnu.org.
>
> Please do not send mail to help-debbugs <at> gnu.org unless you wish
> to report a problem with the Bug-tracking system.

Message #11 received at 32201 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: John Shahid <jvshahid <at> gmail.com>
Cc: 32201 <at> debbugs.gnu.org
Subject: Re: bug#32201: 27.0.50; setenv should not change match-data
Date: Wed, 18 Jul 2018 18:52:18 -0400

[Message part 1 (text/plain, inline)]

tags 32201 + patch
quit

John Shahid <jvshahid <at> 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):

[v1-0001-Add-save-match-data-to-abbreviate-file-name-Bug-3.patch (text/x-diff, inline)]

From a0eec7f2e672804f3a7a30e55c821ba2dac213b7 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> 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

Message #16 received at 32201 <at> debbugs.gnu.org (full text, mbox):

From: John Shahid <jvshahid <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 32201 <at> debbugs.gnu.org
Subject: Re: bug#32201: 27.0.50; setenv should not change match-data
Date: Thu, 19 Jul 2018 00:22:49 +0000

[Message part 1 (text/plain, inline)]

Noam Postavsky <npostavs <at> 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 ?

[0001-Avoid-destroying-match-data-in-setenv.patch (text/x-diff, inline)]

From 7b592252f698ae07a2010302c74b42026fbbca5e Mon Sep 17 00:00:00 2001
From: John Shahid <jvshahid <at> 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

Message #19 received at 32201 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: John Shahid <jvshahid <at> gmail.com>
Cc: 32201 <at> debbugs.gnu.org
Subject: Re: bug#32201: 27.0.50; setenv should not change match-data
Date: Wed, 18 Jul 2018 21:56:49 -0400

John Shahid <jvshahid <at> 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.

Message #22 received at 32201 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: John Shahid <jvshahid <at> gmail.com>
Cc: 32201 <at> debbugs.gnu.org
Subject: Re: bug#32201: 27.0.50; setenv should not change match-data
Date: Sat, 21 Jul 2018 21:30:02 -0400

tags 32201 fixed
close 32201 26.2
quit

Noam Postavsky <npostavs <at> gmail.com> writes:

> John Shahid <jvshahid <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.