bug#24206: 25.1; Curly quotes generate invalid strings, leading to a segfault

[Paul Eggert <eggert@cs.ucla.edu>]

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

Eli Zaretskii wrote:

>> The changes were motivated by bug fixes, not style.
>
> That's not what I see.  E.g., this hunk simply replaces valid code by
> an equivalently valid code:
>
>   -	  if (multibyte)
>   -	    {
>   -	      int len;
>   -
>   -	      STRING_CHAR_AND_LENGTH (strp, len);
>   -	      if (len == 1)
>   -		*bufp = *strp;
>   -	      else
>   -		memcpy (bufp, strp, len);
>   -	      strp += len;
>   -	      bufp += len;
>   -	      nchars++;
>   -	    }
>   -	  else
>   -	    *bufp++ = *strp++, nchars++;
>   +	  /* Fall through to copy one char.  */

Some change in this area was needed because the 'multibyte' flag went away. 
While doing that, I noticed that discarding all the code made this 
somewhat-tricky area easier to follow. It's not merely that the old multibyte 
code is unnecessarily long and hard to follow; it's that the old code does 
something fairly-typical (copy a multibyte character) in an unusual way, which 
is too likely to lead the reader into incorrectly thinking that there is 
something actually unusual about the action. Misleading code like this really 
cries out to be rewritten, particularly if the rewriting simply ionvolves 
deleting it.

In short, the main motivation here was clarity, not merely style.

(I hope I don't have to go into such details to defend every code change I 
install! I'm finding it difficult-enough now to find time to improve Emacs.)

> Same here:
>
>   -      else if (strp[0] == '\\' && strp[1] == '[')
>   +      else if (strp[0] == '\\' && strp[1] == '['
>   +	       && (close_bracket
>   +		   = memchr (strp + 2, ']',
>   +			     SDATA (str) + strbytes - (strp + 2))))
> 	  {
>   -	  ptrdiff_t start_idx;
> 	    bool follow_remap = 1;
>
>   -	  strp += 2;		/* skip \[ */
>   -	  start = strp;
>   -	  start_idx = start - SDATA (string);
>   -
>   -	  while ((strp - SDATA (string)
>   -		  < SBYTES (string))
>   -		 && *strp != ']')
>   -	    strp++;
>   -	  length_byte = strp - start;
>   -
>   -	  strp++;		/* skip ] */

This one is not merely a style change. The old code matched \[ even if not 
followed by ], the new code does not. This is an intended improvement. I plead 
guilty to the charge that the new code is also shorter and clearer.

> and here (which, for some reason, loses part of a comment, and IMO
> makes it half a riddle for the uninitiated):
>
>   -	  /* Note the Fwhere_is_internal can GC, so we have to take
>   -	     relocation of string contents into account.  */
>   -	  strp = SDATA (string) + idx;
>   -	  start = SDATA (string) + start_idx;
>   +	  /* Take relocation of string contents into account.  */
>   +	  strp = SDATA (str) + idx;
>   +	  start = strp - length_byte - 1;

The new comment came because I copied it from somewhere else in the interest of 
consistency. You're right, I omitted some commentary in the process. I thought 
the omitted info obvious, but evidently you think otherwise. It's obviously no 
big deal, so I brought it back by applying the attached patch to master.

> What code generated bogus null bytes?

For example, (substitute-command-keys "\\=") generated "\0".

> I'm not saying it isn't fine to make such changes, I'm urging you and
> the others to resist the temptation of doing so unless really
> necessary.  We are operating in the area of diminishing returns, and
> too many times introduce regressions into code that was working
> properly for decades.

This particular code has been buggy for decades in unusual areas. There is no 
harm in simplifying it when fixing the bugs. On the contrary, we should 
encourage bug fixes that simplify code.

> Where's the O(N**2) performance

When the buffer grew slightly, it was reallocated to be slightly bigger and the 
old data was copied to the new; this is an O(N**2) algorithm, where N is the 
final buffer size. The new approach doubles the buffer size instead (actually, 
multiplies it by 1.5, but that's good enough to bring worst-case behavior down 
to O(N)). This sort of thing is standard programming practice when growing a 
buffer whose eventual size is not yet known.

> and why does performance matter in this function anyway?

It usually doesn't, but it might in the worst case, so I figured I might as well 
fix the O(N**2) problem while I was fixing related bugs. This is a good thing to 
do in master.

> Unlike at that time, I now think
> this was a bad move, because Emacs 25.1 will have the disabled
> conversion in it, so by the time we release the code in master, it
> would be an incompatible change.

If that's the main objection, then let's change Emacs 25 to behave similarly. 
This would be a simple and conservative change to Emacs 25. But even if you 
don't want to change Emacs 25 (and thus you want to Emacs 25 to continue to be 
less-compatible with Emacs 24), it's OK to change this minor detail back to the 
way Emacs 24 does things.

> (I also don't see how it is related to the
> original bug report, which AFAIU was about (message "`foo'") that
> still behaves as in the bug report.)

Alan wanted something that he could put into his .emacs that would cause 
(message PERCENTLESS) to output the string PERCENTLESS as-is, assuming 
PERCENTLESS lacks %. This was the point of his original bug report; his original 
example involved ` and ' but he wanted the same behavior for ‘ and ’, a point 
that became clear during the discussion of Bug#23425. In Message #95 of that bug 
report I proposed the change in question, and in Message #104 you said it 
sounded good to you.

This is a contentious area, and unless there's good reason I'd rather let 
sleeping dogs lie and stick with master's current behavior here.

> (Mumbles something about Emacs maintenance being a lonely business...)

But we have all these nice conversations! :-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-src-doc.c-Fsubstitute_command_keys-Clarify-GC-commen.patch --]
[-- Type: text/x-diff; name="0001-src-doc.c-Fsubstitute_command_keys-Clarify-GC-commen.patch", Size: 1092 bytes --]

From 70a5e67e9a072b6f22343fc0c7eed91dfdaf8025 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 17 Aug 2016 10:15:53 -0700
Subject: [PATCH] * src/doc.c (Fsubstitute_command_keys): Clarify GC comments.

---
 src/doc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/doc.c b/src/doc.c
index 4b91831..6376398 100644
--- a/src/doc.c
+++ b/src/doc.c
@@ -821,7 +821,8 @@ Otherwise, return a new string.  */)
 	      goto do_remap;
 	    }
 
-	  /* Take relocation of string contents into account.  */
+	  /* Fwhere_is_internal can GC, so take relocation of string
+	     contents into account.  */
 	  strp = SDATA (str) + idx;
 	  start = strp - length_byte - 1;
 
@@ -936,7 +937,8 @@ Otherwise, return a new string.  */)
 	    bufp += length_byte;
 	    nchars += length;
 
-	    /* Take relocation of string contents into account.  */
+	    /* Some of the previous code can GC, so take relocation of
+	       string contents into account.  */
 	    strp = SDATA (str) + idx;
 
 	    continue;
-- 
2.5.5