bug#28671: 25.2; double transfer-encoding in rmailedit for complex mime messages

bug#28671: 25.2; double transfer-encoding in rmailedit for complex mime messages

[Ken Olum]

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

When rmail-edit-current-message gets a mime message which is too complex
for it to handle (i.e., anything but one "text/plain" part), it just
gives you the raw message to edit.  If you do so, rmail-cease-edit then
applies the transfer-encoding even though the message wasn't decoded,
and so on.  It also in some circumstances it also moves the marker for
the beginning of the following message.

The attached patch modifies rmail-cease-edit to check the
rmail-old-mime-state flag set by rmail-edit-current-message when it is
editing the raw message.  If set, rmail-cease-edit inserts the
already-encoded body without further processing.  It also deletes the
old body after inserting the new, rather than before, to avoid moving
the next message's marker.

                                        Ken

In GNU Emacs 25.2.2 (x86_64-unknown-linux-gnu, X toolkit, Xaw scroll bars)
 of 2017-05-29 built on olum.org
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:	Ubuntu 16.04.3 LTS

Configured features:
XPM JPEG TIFF GIF PNG SOUND NOTIFY FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS
LUCID X11


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rmailedit.patch --]
[-- Type: text/x-diff, Size: 2657 bytes --]

--- lisp/mail/rmailedit.el	2017-07-06 10:27:25.654906559 -0400
+++ lisp/mail/rmailedit.new.el	2017-09-26 12:00:45.498641338 -0400
@@ -312,26 +312,34 @@
             (data-buffer (current-buffer))
             (start (copy-marker (point) nil)) ; new body will be between
             (end (copy-marker (point) t)))    ; these two markers
-        (with-current-buffer rmail-view-buffer
-          (encode-coding-region headers-end (point-max) coding-system
-                                data-buffer))
-        (delete-region end (point-max))
+        (if mime-state
+            ;; Message is already in encoded state
+            (insert-buffer-substring rmail-view-buffer headers-end
+                                     (with-current-buffer rmail-view-buffer
+                                       (point-max)))
+          (with-current-buffer rmail-view-buffer
+            (encode-coding-region headers-end (point-max) coding-system
+                                  data-buffer)))
 	;; Apply to the mbox buffer any changes in header fields
 	;; that the user made while editing in the view buffer.
 	(rmail-edit-update-headers (rmail-edit-diff-headers
 				    rmail-old-headers new-headers))
 	;; Re-apply content-transfer-encoding, if any, on the message body.
-	(cond
-	 ((string= character-coding "quoted-printable")
-	  (mail-quote-printable-region start (point-max)))
-	 ((and (string= character-coding "base64") is-text-message)
-	  (base64-encode-region start (point-max)))
-	 ((and (eq character-coding 'uuencode) is-text-message)
-	  (error "uuencoded messages are not supported")))
+        (unless mime-state              ; if set, already transfer-encoded
+          (cond
+           ((string= character-coding "quoted-printable")
+            (mail-quote-printable-region start end))
+           ((and (string= character-coding "base64") is-text-message)
+            (base64-encode-region start end))
+           ((and (eq character-coding 'uuencode) is-text-message)
+            (error "uuencoded messages are not supported"))))
         ;; After encoding, make sure buffer ends with a blank line so as not to
         ;; run this message together with the following one.
-        (goto-char (point-max))
-        (rmail-ensure-blank-line))
+        (goto-char end)
+        (rmail-ensure-blank-line)
+        ;; Delete previous body.  This must be after all insertions at the end,
+        ;; so the marker for the beginning of the next message isn't messed up.
+        (delete-region end (point-max)))
       (rmail-set-attribute rmail-edited-attr-index t))
 ;;;??? BROKEN perhaps.
 ;;;    (if (boundp 'rmail-summary-vector)

bug#28671: 25.2; double transfer-encoding in rmailedit for complex mime messages

[Eli Zaretskii]

> From: Ken Olum <kdo@cosmos.phy.tufts.edu>
> Date: Mon, 02 Oct 2017 11:37:46 -0400
> Cc: kdo@cosmos.phy.tufts.edu
> 
> When rmail-edit-current-message gets a mime message which is too complex
> for it to handle (i.e., anything but one "text/plain" part), it just
> gives you the raw message to edit.  If you do so, rmail-cease-edit then
> applies the transfer-encoding even though the message wasn't decoded,
> and so on.  It also in some circumstances it also moves the marker for
> the beginning of the following message.
> 
> The attached patch modifies rmail-cease-edit to check the
> rmail-old-mime-state flag set by rmail-edit-current-message when it is
> editing the raw message.  If set, rmail-cease-edit inserts the
> already-encoded body without further processing.  It also deletes the
> old body after inserting the new, rather than before, to avoid moving
> the next message's marker.

Thanks.  Could you please send a small mbox file which could be used
to reproduce the problem and test the solution?

A couple of minor comments to the patch:

>  	;; Re-apply content-transfer-encoding, if any, on the message body.
> -	(cond
> -	 ((string= character-coding "quoted-printable")
> -	  (mail-quote-printable-region start (point-max)))
> -	 ((and (string= character-coding "base64") is-text-message)
> -	  (base64-encode-region start (point-max)))
> -	 ((and (eq character-coding 'uuencode) is-text-message)
> -	  (error "uuencoded messages are not supported")))
> +        (unless mime-state              ; if set, already transfer-encoded
> +          (cond
> +           ((string= character-coding "quoted-printable")
> +            (mail-quote-printable-region start end))
> +           ((and (string= character-coding "base64") is-text-message)
> +            (base64-encode-region start end))
> +           ((and (eq character-coding 'uuencode) is-text-message)
> +            (error "uuencoded messages are not supported"))))

You could easily make the change less intrusive by simply adding the
mime-state condition as the first alternative in 'cond' with no
action, right?

Also, please add a commit log message for the changes formatted as a
ChangeLog entry (e.g., using "C-x 4 a").

bug#28671: 25.2; double transfer-encoding in rmailedit for complex mime messages

[Ken Olum]

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

Here is a test file.  Edit the first message and make some change (e.g.,
to the subject header).  Then the body will end up base64 encoded
twice.  If you then go to the next message, the headers will have extra
lines at the top due to marker problems.  The patch should fix these
problems.

Here's a new patch with the simplified code as you suggested, and
here's a change log entry:

----------------------------------------------------------------------
2017-10-02  Ken Olum  <kdo@cosmos.phy.tufts.edu>

	Fix problems when editing raw message (Bug#28671)

	* lisp/mail/rmailedit.el (rmail-cease-edit): if rmail-old-mime-state
	is set, meaning that we are editing the raw message, do not
	encode it again.  Delete old body after, not before, inserting
	new, to avoid moving marker at beginning of next message.
----------------------------------------------------------------------

                                        Ken


[-- Attachment #2: rmail-test --]
[-- Type: application/octet-stream, Size: 1055 bytes --]

From kdo@cosmos.phy.tufts.edu Thu Aug 31 10:15:14 2017
MIME-Version: 1.0
From: <kdo@cosmos.phy.tufts.edu>
To: <kdo@cosmos.phy.tufts.edu>
Date: Thu, 31 Aug 2017 10:10:40 -0400
Subject: Test message
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: base64
Message-ID: <f1d709b386bb42a6a0a6ebc15e3a2803@PPE20132.MBTA.COM>
X-RMAIL-ATTRIBUTES: --E-----

VGhpcyBpcyBhIHRlc3QK

From kdo@cosmos.phy.tufts.edu Mon Oct 02 13:20:17 2017
Return-path: <kdo@cosmos.phy.tufts.edu>
Envelope-to: kdo@cosmos.phy.tufts.edu
Delivery-date: Mon, 02 Oct 2017 13:20:17 -0400
Received: from kdo by cosmos.phy.tufts.edu ([local]:local)
	with local id 1dz4O9-0005RP-4a - Using Exim-4.84 (MandrivaLinux) MTA 
	(return-path <kdo@cosmos.phy.tufts.edu>); Mon, 02 Oct 2017 13:20:17 -0400
From: Ken Olum <kdo@cosmos.phy.tufts.edu>
To: kdo@cosmos.phy.tufts.edu
Subject: second test message
Date: Mon, 02 Oct 2017 13:20:17 -0400
Message-ID: <q52efqlxy9a.fsf@cosmos.phy.tufts.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
X-RMAIL-ATTRIBUTES: --------



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: rmailedit.patch --]
[-- Type: text/x-diff, Size: 2403 bytes --]

--- lisp/mail/rmailedit.old.el	2017-07-06 10:27:25.654906559 -0400
+++ lisp/mail/rmailedit.el	2017-10-02 13:44:25.050239587 -0400
@@ -312,26 +312,34 @@
             (data-buffer (current-buffer))
             (start (copy-marker (point) nil)) ; new body will be between
             (end (copy-marker (point) t)))    ; these two markers
-        (with-current-buffer rmail-view-buffer
-          (encode-coding-region headers-end (point-max) coding-system
-                                data-buffer))
-        (delete-region end (point-max))
+        (if mime-state
+            ;; Message is already in encoded state
+            (insert-buffer-substring rmail-view-buffer headers-end
+                                     (with-current-buffer rmail-view-buffer
+                                       (point-max)))
+          (with-current-buffer rmail-view-buffer
+            (encode-coding-region headers-end (point-max) coding-system
+                                  data-buffer)))
 	;; Apply to the mbox buffer any changes in header fields
 	;; that the user made while editing in the view buffer.
-	(rmail-edit-update-headers (rmail-edit-diff-headers
+        (rmail-edit-update-headers (rmail-edit-diff-headers
 				    rmail-old-headers new-headers))
 	;; Re-apply content-transfer-encoding, if any, on the message body.
 	(cond
+	 (mime-state)		    ; if set, already transfer-encoded
 	 ((string= character-coding "quoted-printable")
-	  (mail-quote-printable-region start (point-max)))
+	  (mail-quote-printable-region start end))
 	 ((and (string= character-coding "base64") is-text-message)
-	  (base64-encode-region start (point-max)))
+	  (base64-encode-region start end))
 	 ((and (eq character-coding 'uuencode) is-text-message)
 	  (error "uuencoded messages are not supported")))
         ;; After encoding, make sure buffer ends with a blank line so as not to
         ;; run this message together with the following one.
-        (goto-char (point-max))
-        (rmail-ensure-blank-line))
+        (goto-char end)
+        (rmail-ensure-blank-line)
+        ;; Delete previous body.  This must be after all insertions at the end,
+        ;; so the marker for the beginning of the next message isn't messed up.
+        (delete-region end (point-max)))
       (rmail-set-attribute rmail-edited-attr-index t))
 ;;;??? BROKEN perhaps.
 ;;;    (if (boundp 'rmail-summary-vector)

bug#28671: 25.2; double transfer-encoding in rmailedit for complex mime messages

[Eli Zaretskii]

> From: Ken Olum <kdo@cosmos.phy.tufts.edu>
> Cc: 28671@debbugs.gnu.org, kdo@cosmos.phy.tufts.edu
> Date: Mon, 02 Oct 2017 13:47:20 -0400
> 
> Here is a test file.  Edit the first message and make some change (e.g.,
> to the subject header).  Then the body will end up base64 encoded
> twice.  If you then go to the next message, the headers will have extra
> lines at the top due to marker problems.  The patch should fix these
> problems.
> 
> Here's a new patch with the simplified code as you suggested, and
> here's a change log entry:

Thanks, pushed to the release branch.