bug#42029: `gnus-registry-spool-action' gets field beyond message headers

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[tomotaka.suwa]

Hi,

I've been suffering from `mail-source-crash-box' on getting new mail.

After some debug and investigation, I noticed that
`mail-extract-address-components' was failing by invalid addresses.

The issue happened in `gnus-registry-spool-action' and invalid addresses
are passed by calling `message-fetch-field' on the buffer not narrowed
to message headers.

Below snippet reproduce the root issue:

(with-temp-buffer
  (save-excursion
    ;; mail header
    (insert "From: from@bar.com\n"
            "To: to@bar.com\n"
            "Subject: test\n")
    (newline)
    ;; mail body
    (insert "message\n"
            "Cc: >,@ <foo@bar.com>\n")) ;; by incorrect decode
  (gnus-registry-spool-action 1 "test"))

In stead of `message-fetch-field', calling `message-field-value' would
solve the problem since it ensures the buffer is narrowed at first.

diff -u "d:/msys64/mingw64/share/emacs/26.3/lisp/gnus/gnus-registry.el.orig" "d:/msys64/mingw64/share/emacs/26.3/lisp/gnus/gnus-registry.el"
--- d:/msys64/mingw64/share/emacs/26.3/lisp/gnus/gnus-registry.el.orig	2020-06-24 11:10:49.458397900 +0900
+++ d:/msys64/mingw64/share/emacs/26.3/lisp/gnus/gnus-registry.el	2020-06-23 11:08:23.170050000 +0900
@@ -405,10 +405,10 @@
   (let ((to (gnus-group-guess-full-name-from-command-method group))
           (recipients (or recipients
                           (gnus-registry-sort-addresses
-                           (or (message-fetch-field "cc") "")
-                           (or (message-fetch-field "to") ""))))
-          (subject (or subject (message-fetch-field "subject")))
-          (sender (or sender (message-fetch-field "from"))))
+                           (or (message-field-value "cc") "")
+                           (or (message-field-value "to") ""))))
+          (subject (or subject (message-field-value "subject")))
+          (sender (or sender (message-field-value "from"))))
       (when (and (stringp id) (string-match "\r$" id))
         (setq id (substring id 0 -1)))
       (gnus-message 7 "Gnus registry: article %s spooled to %s"

Diff finished.  Wed Jun 24 11:13:17 2020

Gnus v5.13
GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2020-04-04

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Eric Abrahamsen]

tomotaka.suwa@gmail.com writes:

> Hi,
>
> I've been suffering from `mail-source-crash-box' on getting new mail.
>
> After some debug and investigation, I noticed that
> `mail-extract-address-components' was failing by invalid addresses.
>
> The issue happened in `gnus-registry-spool-action' and invalid addresses
> are passed by calling `message-fetch-field' on the buffer not narrowed
> to message headers.
>
> Below snippet reproduce the root issue:
>
> (with-temp-buffer
>   (save-excursion
>     ;; mail header
>     (insert "From: from@bar.com\n"
>             "To: to@bar.com\n"
>             "Subject: test\n")
>     (newline)
>     ;; mail body
>     (insert "message\n"
>             "Cc: >,@ <foo@bar.com>\n")) ;; by incorrect decode
>   (gnus-registry-spool-action 1 "test"))
>
> In stead of `message-fetch-field', calling `message-field-value' would
> solve the problem since it ensures the buffer is narrowed at first.

Thanks for this report. It might be simpler to wrap the whole thing in a
single save-restriction+narrow-to-headers, since the function gets
called four times. What do you think?

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Tomotaka SUWA]

> Thanks for this report. It might be simpler to wrap the whole thing in a
> single save-restriction+narrow-to-headers, since the function gets
> called four times. What do you think?

I wrote the patch paying attention to minimize side effects since I'm not
familiar with that functionality. So if `gnus-registry-spool-action' is
interested in only mail headers, your proposal is much better.

-- 
Tomotaka SUWA

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Eric Abrahamsen]

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

Tomotaka SUWA <tomotaka.suwa@gmail.com> writes:

>> Thanks for this report. It might be simpler to wrap the whole thing in a
>> single save-restriction+narrow-to-headers, since the function gets
>> called four times. What do you think?
>
> I wrote the patch paying attention to minimize side effects since I'm not
> familiar with that functionality. So if `gnus-registry-spool-action' is
> interested in only mail headers, your proposal is much better.

Okay! Would you be willing to give the attached diff a quick test?

Thanks,
Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: protect-registry-spool-action.diff --]
[-- Type: text/x-patch, Size: 2354 bytes --]

diff --git a/lisp/gnus/gnus-registry.el b/lisp/gnus/gnus-registry.el
index f306889a7f..a36095c1ec 100644
--- a/lisp/gnus/gnus-registry.el
+++ b/lisp/gnus/gnus-registry.el
@@ -449,19 +449,21 @@ gnus-registry-action
      to subject sender recipients)))
 
 (defun gnus-registry-spool-action (id group &optional subject sender recipients)
-  (let ((to (gnus-group-guess-full-name-from-command-method group))
-        (recipients (or recipients
-                        (gnus-registry-sort-addresses
-                         (or (message-fetch-field "cc") "")
-                         (or (message-fetch-field "to") ""))))
-        (subject (or subject (message-fetch-field "subject")))
-        (sender (or sender (message-fetch-field "from"))))
-    (when (and (stringp id) (string-match "\r$" id))
-      (setq id (substring id 0 -1)))
-    (gnus-message 7 "Gnus registry: article %s spooled to %s"
-                  id
-                  to)
-    (gnus-registry-handle-action id nil to subject sender recipients)))
+  (save-excursion
+    (message-narrow-to-headers-or-head)
+    (let ((to (gnus-group-guess-full-name-from-command-method group))
+          (recipients (or recipients
+                          (gnus-registry-sort-addresses
+                           (or (message-fetch-field "cc") "")
+                           (or (message-fetch-field "to") ""))))
+          (subject (or subject (message-fetch-field "subject")))
+          (sender (or sender (message-fetch-field "from"))))
+      (when (and (stringp id) (string-match "\r$" id))
+	(setq id (substring id 0 -1)))
+      (gnus-message 7 "Gnus registry: article %s spooled to %s"
+                    id
+                    to)
+      (gnus-registry-handle-action id nil to subject sender recipients))))
 
 (defun gnus-registry-handle-action (id from to subject sender
                                        &optional recipients)
@@ -1064,7 +1066,7 @@ gnus-registry-get-article-marks
 Uses process/prefix conventions.  For multiple articles,
 only the last one's marks are returned."
   (interactive (gnus-summary-work-articles 1))
-  (let* ((article (last articles))
+  (let* ((article (car (last articles)))
          (id (gnus-registry-fetch-message-id-fast article))
          (marks (when id (gnus-registry-get-id-key id 'mark))))
     (when (called-interactively-p 'any)

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Basil L. Contovounesios]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> +  (save-excursion
> +    (message-narrow-to-headers-or-head)

Shouldn't this additionally or instead be wrapped in save-restriction?

Thanks,

-- 
Basil

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Eric Abrahamsen]

On 06/26/20 22:24 PM, Basil L. Contovounesios wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> +  (save-excursion
>> +    (message-narrow-to-headers-or-head)
>
> Shouldn't this additionally or instead be wrapped in save-restriction?

Bleagh, you're right, sorry about that. It was supposed to be
`save-restriction'.

Thanks!
Eric

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Tomotaka SUWA]

> Okay! Would you be willing to give the attached diff a quick test?

Sure. I gave a test with the `save-restriction' version and it
worked well.

By the way, does the change of `gnus-registry-get-article-marks'
have any relation to this issue?

Thanks,

-- 
Tomotaka SUWA

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Lars Ingebrigtsen]

Tomotaka SUWA <tomotaka.suwa@gmail.com> writes:

>> Okay! Would you be willing to give the attached diff a quick test?
>
> Sure. I gave a test with the `save-restriction' version and it
> worked well.

Thanks for testing; I've now applied Eric's patch (with the additional
fix from Basil) to Emacs 28.

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

bug#42029: `gnus-registry-spool-action' gets field beyond message headers

[Eric Abrahamsen]

On 07/19/20 02:20 AM, Lars Ingebrigtsen wrote:
> Tomotaka SUWA <tomotaka.suwa@gmail.com> writes:
>
>>> Okay! Would you be willing to give the attached diff a quick test?
>>
>> Sure. I gave a test with the `save-restriction' version and it
>> worked well.
>
> Thanks for testing; I've now applied Eric's patch (with the additional
> fix from Basil) to Emacs 28.

Thanks! I've had almost no time for coding recently...