GNU bug report logs - #37095
[PATCH] Save match data in ucs-normalize-region

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 37095 in the body.
You can then email your comments to 37095 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: Akinori MUSHA <knu <at> iDaemons.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Save match data in ucs-normalize-region
Date: Tue, 20 Aug 2019 16:18:06 +0900

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

A patch generated by git format-patch is attached below, which simply
wraps `ucs-normalize-region` with `save-match-data`.

I'm a user of the Emacs Mac port by mituharu was investigating a bug
where dired fails to open a certain local directory on macOS.  The
error was raised at `replace-match` in the `insert-directory`
function:

```
		(when (re-search-forward "^ *\\(total\\)" nil t)
		  (let ((available (get-free-disk-space ".")))
		    (when available
		      ;; Replace "total" with "used", to avoid confusion.
		      (replace-match "total used in directory" nil nil nil 1)
```

And it turned out the match data changed after returning from
`get-free-disk-space` and that was why `replace-match` failed.

Inside of `get-free-disk-space` most platforms uses a generic method
to get the free space, and that part is fine because it is surrounded
by `save-match-data`.  However, the Mac port is one of the few
platforms that implements a native 'file-system-info` function, which
is called if it exists.  Then, the `file-system-info` in `src/mac.c`
calls ENCODE_FILE() on a given directory name, which in the end calls
`ucs-normalize-region` to normalize the filename, where the match data
is clobbered.

https://bitbucket.org/mituharu/emacs-mac/src/df827786d7a7fb0a0e2f27577af67e32d9a888a9/src/mac.c#lines-2337

ENCODE_FILE() is transparently called by many C functions, which means
`ucs-normalize-region` can be called at unpredictable timings, so I
think it should keep match data unchanged.

-- 
Akinori MUSHA / https://akinori.org/

[0001-Save-match-data-in-ucs-normalize-region.patch (text/plain, attachment)]

[signature.asc (application/pgp-signature, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> mouse.gnus.org>
To: Akinori MUSHA <knu <at> iDaemons.org>
Cc: 37095 <at> debbugs.gnu.org
Subject: Re: bug#37095: [PATCH] Save match data in ucs-normalize-region
Date: Fri, 23 Aug 2019 05:51:29 +0200

Akinori MUSHA <knu <at> iDaemons.org> writes:

> A patch generated by git format-patch is attached below, which simply
> wraps `ucs-normalize-region` with `save-match-data`.
>
> I'm a user of the Emacs Mac port by mituharu was investigating a bug
> where dired fails to open a certain local directory on macOS.  The
> error was raised at `replace-match` in the `insert-directory`
> function:
>
> ```
> 		(when (re-search-forward "^ *\\(total\\)" nil t)
> 		  (let ((available (get-free-disk-space ".")))
> 		    (when available
> 		      ;; Replace "total" with "used", to avoid confusion.
> 		      (replace-match "total used in directory" nil nil nil 1)
> ```
>
> And it turned out the match data changed after returning from
> `get-free-disk-space` and that was why `replace-match` failed.

You don't say what Emacs version you're reporting this bug for, but the
following patch was applied in February 2018 to the Emacs trunk, so I
think this problem has been fixed by now:

diff --git a/lisp/files.el b/lisp/files.el
index 91aa95d631..75d3b7b1e7 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6473,9 +6473,10 @@ get-free-disk-space
 space (normally, the number of free 1KB blocks).
 
 If DIR's free space cannot be obtained, this function returns nil."
-  (let ((avail (nth 2 (file-system-info dir))))
-    (if avail
-	(format "%.0f" (/ avail 1024)))))
+  (save-match-data
+    (let ((avail (nth 2 (file-system-info dir))))
+      (if avail
+	  (format "%.0f" (/ avail 1024))))))
 
 ;; The following expression replaces `dired-move-to-filename-regexp'.
 (defvar directory-listing-before-filename-regexp


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
To: Lars Ingebrigtsen <larsi <at> mouse.gnus.org>
Cc: Akinori MUSHA <knu <at> iDaemons.org>, 37095 <at> debbugs.gnu.org
Subject: Re: bug#37095: [PATCH] Save match data in ucs-normalize-region
Date: Fri, 23 Aug 2019 14:06:12 +0900

On Fri, 23 Aug 2019 12:51:29 +0900,
Lars Ingebrigtsen wrote:
> 
> Akinori MUSHA <knu <at> iDaemons.org> writes:
> 
> > A patch generated by git format-patch is attached below, which simply
> > wraps `ucs-normalize-region` with `save-match-data`.
> >
> > I'm a user of the Emacs Mac port by mituharu was investigating a bug
> > where dired fails to open a certain local directory on macOS.  The
> > error was raised at `replace-match` in the `insert-directory`
> > function:
> >
> > ```
> > 		(when (re-search-forward "^ *\\(total\\)" nil t)
> > 		  (let ((available (get-free-disk-space ".")))
> > 		    (when available
> > 		      ;; Replace "total" with "used", to avoid confusion.
> > 		      (replace-match "total used in directory" nil nil nil 1)
> > ```
> >
> > And it turned out the match data changed after returning from
> > `get-free-disk-space` and that was why `replace-match` failed.
> 
> You don't say what Emacs version you're reporting this bug for, but the
> following patch was applied in February 2018 to the Emacs trunk, so I
> think this problem has been fixed by now:

For the Mac port, the "work" branch already contains a similar change:

  https://bitbucket.org/mituharu/emacs-mac/commits/b651c3a6bab6795202e2ebcd4396d665909cc210

It will shortly be included in the next release based on Emacs 26.3 RC1.

				     YAMAMOTO Mitsuharu
				mituharu <at> math.s.chiba-u.ac.jp

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

From: Akinori MUSHA <knu <at> idaemons.org>
To: YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>
Cc: Lars Ingebrigtsen <larsi <at> mouse.gnus.org>, 37095 <at> debbugs.gnu.org
Subject: Re: bug#37095: [PATCH] Save match data in ucs-normalize-region
Date: Fri, 23 Aug 2019 16:21:58 +0900

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

Thanks you both for the information.

I don't insist if you don't think this way, but I think this bug was partly
due to ucs-normalize-region being called only if the user is on a certain
platform/filesystem and the file name actually needs normalization, so it
would be better to fix such a function not to break a globally shared state
like match data so that there's no room for more bugs of the same kind.

Just my two yen.

2019-08-23日(金) 14:06 YAMAMOTO Mitsuharu <mituharu <at> math.s.chiba-u.ac.jp>:

> On Fri, 23 Aug 2019 12:51:29 +0900,
> Lars Ingebrigtsen wrote:
> >
> > Akinori MUSHA <knu <at> iDaemons.org> writes:
> >
> > > A patch generated by git format-patch is attached below, which simply
> > > wraps `ucs-normalize-region` with `save-match-data`.
> > >
> > > I'm a user of the Emacs Mac port by mituharu was investigating a bug
> > > where dired fails to open a certain local directory on macOS.  The
> > > error was raised at `replace-match` in the `insert-directory`
> > > function:
> > >
> > > ```
> > >             (when (re-search-forward "^ *\\(total\\)" nil t)
> > >               (let ((available (get-free-disk-space ".")))
> > >                 (when available
> > >                   ;; Replace "total" with "used", to avoid confusion.
> > >                   (replace-match "total used in directory" nil nil nil
> 1)
> > > ```
> > >
> > > And it turned out the match data changed after returning from
> > > `get-free-disk-space` and that was why `replace-match` failed.
> >
> > You don't say what Emacs version you're reporting this bug for, but the
> > following patch was applied in February 2018 to the Emacs trunk, so I
> > think this problem has been fixed by now:
>
> For the Mac port, the "work" branch already contains a similar change:
>
>
> https://bitbucket.org/mituharu/emacs-mac/commits/b651c3a6bab6795202e2ebcd4396d665909cc210
>
> It will shortly be included in the next release based on Emacs 26.3 RC1.
>
>                                      YAMAMOTO Mitsuharu
>                                 mituharu <at> math.s.chiba-u.ac.jp
>
>

[Message part 2 (text/html, inline)]

Previous Next

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