bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Mark Lillibridge]

mail-strip-quoted-names in mail/mail-utils.el (at least version 23.1
onwards) contains the following code:

mail-utils.el:187:
	   (with-current-buffer (get-buffer-create " *temp*")
	     (erase-buffer)
	     ...
	     (erase-buffer))

This code erases the buffer " *temp*" even if it is being used by
another piece of code!  This is particularly bad because this is the
first buffer used by with-temp-buffer.

A simple fix is to switch to using with-temp-buffer, which always
creates and destroys a new buffer (patch at end):

	   (with-temp-buffer
	      ...
	      )


    This bug breaks unrmail badly, causing it to discard all messages
after one containing a from line that causes that block of code to be
executed (roughly, from lines that contain nested comments).  This is
because unrmail loads the file to be converted into a buffer created
with with-temp-buffer, which is in turn erased prematurely by
mail-strip-quoted-names.  [There is a longer discussion about this on
the developers mailing list.]

    As this bug causes the permanent loss of e-mail, it should probably
be fixed posthaste, including in version 23.

- Mark

ts-rhel5 [158]% diff mail-utils.el new-mail-utils.el
187,188c187
<          (with-current-buffer (get-buffer-create " *temp*")
<            (erase-buffer)
---
>          (with-temp-buffer
201,202c200
<            (setq address (buffer-string))
<            (erase-buffer))
---
>            (setq address (buffer-string)))

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Glenn Morris]

Version: 23.3

Thanks; applied.

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Stefan Monnier]

close 7746
thanks

> This code erases the buffer " *temp*" even if it is being used by
> another piece of code!  This is particularly bad because this is the
> first buffer used by with-temp-buffer.

Indeed, that's wrong, thanks for spotting it.  I've installed your
suggested patch (see below) into the emacs-23 branch (for Emacs-23.3).

BTW, I'm wondering why the code handles nesting in this way.  Can you
try the second patch below (you may need to hand-apply it since it's
based on the new code I just installed), to confirm that it works just
as well?


        Stefan


--- lisp/mail/mail-utils.el	2010-10-09 00:41:03 +0000
+++ lisp/mail/mail-utils.el	2011-01-02 03:03:38 +0000
@@ -189,8 +189,7 @@
        ;; Detect nested comments.
        (if (string-match "[ \t]*(\\([^)\\]\\|\\\\.\\|\\\\\n\\)*(" address)
 	   ;; Strip nested comments.
-	   (with-current-buffer (get-buffer-create " *temp*")
-	     (erase-buffer)
+	   (with-temp-buffer
 	     (insert address)
 	     (set-syntax-table lisp-mode-syntax-table)
 	     (goto-char 1)
@@ -203,8 +202,7 @@
 				    (forward-sexp 1)
 				  (error (goto-char (point-max))))
 				  (point))))
-	     (setq address (buffer-string))
-	     (erase-buffer))
+	     (setq address (buffer-string)))
 	 ;; Strip non-nested comments an easier way.
 	 (while (setq pos (string-match
 			    ;; This doesn't hack rfc822 nested comments


Second patch to simplify the code:

                            
=== modified file 'lisp/mail/mail-utils.el'
--- lisp/mail/mail-utils.el	2011-01-02 03:16:03 +0000
+++ lisp/mail/mail-utils.el	2011-01-02 03:19:53 +0000
@@ -186,30 +186,13 @@
 	       (mapconcat 'identity (rfc822-addresses address) ", "))
       (let (pos)
 
-       ;; Detect nested comments.
-       (if (string-match "[ \t]*(\\([^)\\]\\|\\\\.\\|\\\\\n\\)*(" address)
-	   ;; Strip nested comments.
-	   (with-temp-buffer
-	     (insert address)
-	     (set-syntax-table lisp-mode-syntax-table)
-	     (goto-char 1)
-	     (while (search-forward "(" nil t)
-	       (forward-char -1)
-	       (skip-chars-backward " \t")
-	       (delete-region (point)
-			      (save-excursion
-				(condition-case ()
-				    (forward-sexp 1)
-				  (error (goto-char (point-max))))
-				  (point))))
-	     (setq address (buffer-string)))
-	 ;; Strip non-nested comments an easier way.
+        ;; Strip comments.
 	 (while (setq pos (string-match
 			    ;; This doesn't hack rfc822 nested comments
 			    ;;  `(xyzzy (foo) whinge)' properly.  Big deal.
-			    "[ \t]*(\\([^)\\]\\|\\\\.\\|\\\\\n\\)*)"
+                          "[ \t]*(\\([^()\\]\\|\\\\.\\|\\\\\n\\)*)"
 			    address))
-	   (setq address (replace-match "" nil nil address 0))))
+          (setq address (replace-match "" nil nil address 0)))
 
        ;; strip surrounding whitespace
        (string-match "\\`[ \t\n]*" address)

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Mark Lillibridge]

>  close 7746
>  thanks
>  
>  > This code erases the buffer " *temp*" even if it is being used by
>  > another piece of code!  This is particularly bad because this is the
>  > first buffer used by with-temp-buffer.
>  
>  Indeed, that's wrong, thanks for spotting it.  I've installed your
>  suggested patch (see below) into the emacs-23 branch (for Emacs-23.3).
>  
>  BTW, I'm wondering why the code handles nesting in this way.  Can you
>  try the second patch below (you may need to hand-apply it since it's
>  based on the new code I just installed), to confirm that it works just
>  as well?

    I looked up rfc822 comments on the web and found at
http://www.w3.org/Protocols/rfc822/3_Lexical.html:

comment     =  "(" *(ctext / quoted-pair / comment) ")"

ctext       =  <any CHAR excluding "(",     ; => may be folded
                ")", "\" & CR, & including
                linear-white-space>

quoted-pair =  "\" CHAR                     ; may quote any char


After some thought, I figured out why you your code doesn't work:

  it turns "( \(  )" into "( \"


You might be able to fix this problem using subgroups, but it's going to
be fairly tricky code.

- Mark

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Stefan Monnier]

> After some thought, I figured out why you your code doesn't work:
>   it turns "( \(  )" into "( \"

Hmm... not in my test:

       (let ((address "(sadffds \\( adf)") pos)
         (while (setq pos (string-match
                          ;; This doesn't hack rfc822 nested comments
                          ;;  `(xyzzy (foo) whinge)' properly.  Big deal.
                          "[ \t]*(\\([^()\\]\\|\\\\.\\|\\\\\n\\)*)"
                          address))
           (setq address (replace-match "" nil nil address 0)))
         address)

returns "".


        Stefan

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Mark Lillibridge]

    Opps.  My bad.  On closer look, I can't see any way to trip up the
code.  I'm it running against my e-mail corpus now.

- Mark

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Mark Lillibridge]

    I ran the change against my e-mail corpus and my sanity checker
didn't find any problems for what it's worth.

- Mark

bug#7746: SERIOUS BUG: mail-strip-quoted-names bug causing unrmail to lose mail

[Stefan Monnier]

>     I ran the change against my e-mail corpus and my sanity checker
> didn't find any problems for what it's worth.

Thanks for confirming.
I've installed that change on the trunk (i.e. for Emacs-24).


        Stefan