GNU bug report logs - #5440
23.1; buffer-file-format encoding temp buffers not reentrant

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 5440 in the body.
You can then email your comments to 5440 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: Kevin Ryde <user42 <at> zip.com.au>
To: bug-gnu-emacs <at> gnu.org
Subject: 23.1; buffer-file-format encoding temp buffers not reentrant
Date: Fri, 22 Jan 2010 10:03:21 +1100

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

The way build_annotations() makes `format-annotate-function' use the
same temp buffer " *Format Temp 0*" on each call is not reentrant.  If
TO-FN from the format-alist does anything that writes another buffer
with a buffer-file-format then the buffer which TO-FN is supposed to be
working on is clobbered.

foo.el below illustrates the problem.  Evaluating it writes a /tmp/x
containing

    THIS IS FORMAT ONE
    this is buffer yyy text

where I expected it to be

    THIS IS FORMAT TWO
    hello world

The `message's emitted by my-one-encode and my-two-encode show they get
the same " *Format Temp 0*" buffer.  The latter is what you get in
/tmp/x if taking away the "/tmp/y" blob within `my-two-encode'.

I expect this wouldn't arise often in practice, but it ought to be
reasonably easy to make build_annotations() (or wherever) do better in
the management of temporary buffers.  I expect it's a matter of finding
the right place in the write crunching that temp buffers used can be
killed.

[foo.el (application/emacs-lisp, attachment)]

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

In GNU Emacs 23.1.1 (i486-pc-linux-gnu, GTK+ Version 2.16.5)
 of 2009-09-14 on raven, modified by Debian
configured using `configure  '--build=i486-linux-gnu' '--host=i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.1/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.1/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_AU
  value of $XMODIFIERS: nil
  locale-coding-system: iso-latin-1-unix
  default-enable-multibyte-characters: t

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 5440 <at> debbugs.gnu.org
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Thu, 21 Jul 2016 00:33:17 -0400

This reproduces on Emacs 25.  But I wonder if this should be a wishlist
instead of a bug.  As the original report notes, decoding a buffer
while encoding another buffer doesn't seem like a normal use-case.

If no one objects in the next few weeks, I can wishlist this.  Making it
a minor-severity bug also seems reasonable to me.

Kevin Ryde <user42 <at> zip.com.au> writes:

> The way build_annotations() makes `format-annotate-function' use the
> same temp buffer " *Format Temp 0*" on each call is not reentrant.  If
> TO-FN from the format-alist does anything that writes another buffer
> with a buffer-file-format then the buffer which TO-FN is supposed to be
> working on is clobbered.
>
> foo.el below illustrates the problem.  Evaluating it writes a /tmp/x
> containing
>
>     THIS IS FORMAT ONE
>     this is buffer yyy text
>
> where I expected it to be
>
>     THIS IS FORMAT TWO
>     hello world
>
> The `message's emitted by my-one-encode and my-two-encode show they get
> the same " *Format Temp 0*" buffer.  The latter is what you get in
> /tmp/x if taking away the "/tmp/y" blob within `my-two-encode'.
>
> I expect this wouldn't arise often in practice, but it ought to be
> reasonably easy to make build_annotations() (or wherever) do better in
> the management of temporary buffers.  I expect it's a matter of finding
> the right place in the write crunching that temp buffers used can be
> killed.
>
>
> (add-to-list 'format-alist '(my-one
>                              "My format."
>                              nil ;; no automatic decode
>                              my-one-decode
>                              my-one-encode
>                              t     ;; encode modifies the region
>                              nil)) ;; write removes from buffer-file-formats
>
> (defun my-one-decode (beg end)
>   end)
>
> (defun my-one-encode (beg end buffer)
>   (message "my-one-encode in %S" (current-buffer))
>
>   (save-excursion
>     (save-restriction
>       (narrow-to-region beg end)
>
>       (insert "THIS IS FORMAT ONE\n")
>
>       (point-max))))
>
>
> (add-to-list 'format-alist '(my-two
>                              "My format."
>                              nil ;; no automatic decode
>                              my-two-decode
>                              my-two-encode
>                              t     ;; encode modifies the region
>                              nil)) ;; write removes from buffer-file-formats
>
> (defun my-two-decode (beg end)
>   (save-excursion
>     (save-restriction
>       (narrow-to-region beg end)
>
>       (goto-char beg)
>       (if (looking-at "THIS IS FORMAT TWO\n")
>           (delete-region (match-beginning 0) (match-end 0)))
>
>       (point-max))))
>
> (defun my-two-encode (beg end buffer)
>   (message "my-two-encode in %S" (current-buffer))
>
>   (save-excursion
>     (save-restriction
>       (narrow-to-region beg end)
>
>       (goto-char (point-min))
>       (insert "THIS IS FORMAT TWO\n")
>
>       (save-current-buffer
>         (find-file "/tmp/y")
>         (format-decode-buffer 'my-one)
>         (erase-buffer)
>         (insert "this is buffer yyy text\n")
>         (save-buffer)
>         (kill-buffer nil))
>
>       (point-max))))
>
>
> (progn
>   (find-file "/tmp/x")
>   (format-decode-buffer 'my-two)
>   (erase-buffer)
>   (insert "hello world\n")
>   (save-buffer)
>   (kill-buffer nil)
>   (find-file "/tmp/x"))
>
>
>
> In GNU Emacs 23.1.1 (i486-pc-linux-gnu, GTK+ Version 2.16.5)
>  of 2009-09-14 on raven, modified by Debian
> configured using `configure  '--build=i486-linux-gnu' '--host=i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.1/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.1/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''
>
> Important settings:
>   value of $LC_ALL: nil
>   value of $LC_COLLATE: nil
>   value of $LC_CTYPE: nil
>   value of $LC_MESSAGES: nil
>   value of $LC_MONETARY: nil
>   value of $LC_NUMERIC: nil
>   value of $LC_TIME: nil
>   value of $LANG: en_AU
>   value of $XMODIFIERS: nil
>   locale-coding-system: iso-latin-1-unix
>   default-enable-multibyte-characters: t

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 5440 <at> debbugs.gnu.org, user42 <at> zip.com.au
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Thu, 21 Jul 2016 17:27:21 +0300

> From: Andrew Hyatt <ahyatt <at> gmail.com>
> Date: Thu, 21 Jul 2016 00:33:17 -0400
> Cc: 5440 <at> debbugs.gnu.org
> 
> This reproduces on Emacs 25.  But I wonder if this should be a wishlist
> instead of a bug.  As the original report notes, decoding a buffer
> while encoding another buffer doesn't seem like a normal use-case.
> 
> If no one objects in the next few weeks, I can wishlist this.  Making it
> a minor-severity bug also seems reasonable to me.

I agree.  I also suggest to amend the documentation to make this issue
more explicitly mentioned.  The ELisp manual already says

 -- Variable: format-alist
     This list contains one format definition for each defined file
     format.            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     ^^^^^^

"Only one format definition per format."  But I think it won't do any
harm to specifically warn about violating that.

Thanks.

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 5440 <at> debbugs.gnu.org, user42 <at> zip.com.au
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Sun, 24 Jul 2016 01:06:05 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Andrew Hyatt <ahyatt <at> gmail.com>
>> Date: Thu, 21 Jul 2016 00:33:17 -0400
>> Cc: 5440 <at> debbugs.gnu.org
>> 
>> This reproduces on Emacs 25.  But I wonder if this should be a wishlist
>> instead of a bug.  As the original report notes, decoding a buffer
>> while encoding another buffer doesn't seem like a normal use-case.
>> 
>> If no one objects in the next few weeks, I can wishlist this.  Making it
>> a minor-severity bug also seems reasonable to me.
>
> I agree.  I also suggest to amend the documentation to make this issue
> more explicitly mentioned.  The ELisp manual already says
>
>  -- Variable: format-alist
>      This list contains one format definition for each defined file
>      format.            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      ^^^^^^
>
> "Only one format definition per format."  But I think it won't do any
> harm to specifically warn about violating that.
>
> Thanks.

I think the original bug report had one format definition for each
defined file format, but was manipulating a file in one format while in
another format function.  How about I change the documentation to just
warn not to manipulate other files in FROM-FN here?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 5440 <at> debbugs.gnu.org, user42 <at> zip.com.au
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Sun, 24 Jul 2016 17:25:37 +0300

> From: Andrew Hyatt <ahyatt <at> gmail.com>
> Cc: user42 <at> zip.com.au,  5440 <at> debbugs.gnu.org
> Date: Sun, 24 Jul 2016 01:06:05 -0400
> 
> >  -- Variable: format-alist
> >      This list contains one format definition for each defined file
> >      format.            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >      ^^^^^^
> >
> > "Only one format definition per format."  But I think it won't do any
> > harm to specifically warn about violating that.
> >
> > Thanks.
> 
> I think the original bug report had one format definition for each
> defined file format, but was manipulating a file in one format while in
> another format function.

That's more than one format in disguise.

> How about I change the documentation to just warn not to manipulate
> other files in FROM-FN here?

Can you show a proposed patch?

Thanks.

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 5440 <at> debbugs.gnu.org, user42 <at> zip.com.au
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Mon, 25 Jul 2016 00:07:50 -0400

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Andrew Hyatt <ahyatt <at> gmail.com>
>> Cc: user42 <at> zip.com.au,  5440 <at> debbugs.gnu.org
>> Date: Sun, 24 Jul 2016 01:06:05 -0400
>> 
>> >  -- Variable: format-alist
>> >      This list contains one format definition for each defined file
>> >      format.            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >      ^^^^^^
>> >
>> > "Only one format definition per format."  But I think it won't do any
>> > harm to specifically warn about violating that.
>> >
>> > Thanks.
>> 
>> I think the original bug report had one format definition for each
>> defined file format, but was manipulating a file in one format while in
>> another format function.
>
> That's more than one format in disguise.
>
>> How about I change the documentation to just warn not to manipulate
>> other files in FROM-FN here?
>
> Can you show a proposed patch?
>
> Thanks.

Attached.

[0001-Warn-against-unintentional-recursion-in-formatting.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 5440 <at> debbugs.gnu.org, user42 <at> zip.com.au
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Mon, 25 Jul 2016 19:35:41 +0300

> From: Andrew Hyatt <ahyatt <at> gmail.com>
> Cc: user42 <at> zip.com.au,  5440 <at> debbugs.gnu.org
> Date: Mon, 25 Jul 2016 00:07:50 -0400
> 
> diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
> index f3650a4..5763380 100644
> --- a/doc/lispref/files.texi
> +++ b/doc/lispref/files.texi

Thanks.  A couple of comments:

> @@ -3238,7 +3238,9 @@ Format Conversion Round-Trip
>  
>  One responsibility of @var{from-fn} is to make sure that the beginning
>  of the file no longer matches @var{regexp}.  Otherwise it is likely to
> -get called again.
> +get called again.  Also, @var{from-fn} must not involve other buffers or
> +files other than the one being decoded

One of these 2 "others" should be deleted, I think.

>                                         or else formatting may happen
> +during formatting, leading to incorrect results.

I would say something like

  otherwise the internal buffer used for formatting might be
  overwritten.

> +@var{to-fn} must not involve other buffers or files other than the one
> +being encoded, or else formatting may happen during formatting,
> +leading to incorrect results.

Same here.

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 5440 <at> debbugs.gnu.org, user42 <at> zip.com.au
Subject: Re: bug#5440: 23.1;
 buffer-file-format encoding temp buffers not reentrant
Date: Mon, 25 Jul 2016 21:13:14 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Andrew Hyatt <ahyatt <at> gmail.com>
>> Cc: user42 <at> zip.com.au,  5440 <at> debbugs.gnu.org
>> Date: Mon, 25 Jul 2016 00:07:50 -0400
>> 
>> diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
>> index f3650a4..5763380 100644
>> --- a/doc/lispref/files.texi
>> +++ b/doc/lispref/files.texi
>
> Thanks.  A couple of comments:
>
>> @@ -3238,7 +3238,9 @@ Format Conversion Round-Trip
>>  
>>  One responsibility of @var{from-fn} is to make sure that the beginning
>>  of the file no longer matches @var{regexp}.  Otherwise it is likely to
>> -get called again.
>> +get called again.  Also, @var{from-fn} must not involve other buffers or
>> +files other than the one being decoded
>
> One of these 2 "others" should be deleted, I think.

Done

>
>>                                         or else formatting may happen
>> +during formatting, leading to incorrect results.
>
> I would say something like
>
>   otherwise the internal buffer used for formatting might be
>   overwritten.

Done

>
>> +@var{to-fn} must not involve other buffers or files other than the one
>> +being encoded, or else formatting may happen during formatting,
>> +leading to incorrect results.
>
> Same here.

Done.

Thanks for the suggestions.  I'll submit this and close out the bug.

Previous Next

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