bug#45128: [PATCH] rmail-spam-filter can lose mail

[emacs-f@media.mit.edu]

[See after description for patch and reproducer.]

I just started using rmail-spam-filter to flush a couple of trolls,
and discovered that it's got bugs which will cause mail to be spuriously
deleted and/or overlooked.  The bugs probably date back to the package's
introduction; I'm running 24.3.1 and the code is unchanged in 27.1
except for tiny stylistic changes to three comments.

There are two bugs here:

(1) If an output-and-delete action fires, and rsf-file does not already
exist, -the wrong message will be filed and deleted-!  Specifically, the
affected message will be the first unseen message, and -not- the spam.
(This message will be "stealth-deleted"---it won't show up with a D in
the summary window, but it -will- have a label of "deleted".  Expunging
or saving doesn't seem to actually delete it, but if you save, kill the
RMAIL buffer, and reload it, -now- the message has a D and -then- it
will be expunged on save.  Also, it was simply deleted without the kill
& reload in my main RMAIL file when I first hit this, though I haven't
reproduced that.  See below.  It's buggy either way.)
 
(2) Anytime rsf decides to prompt, it spuriously moves to the first
unseen message in order to make the rest of the screen look better
during the prompt.  But that means that when control leaves rsf and
reverts back to the usual mailreading flow, the user will entirely miss
seeing that message, since RMAIL will have been incorrectly told the
user has seen it.  (This is because RMAIL marks the message as seen
while rsf is displaying an entirely unrelated prompt about a piece of
-spam-, so the user is likely looking at the prompt, not an entirely
unrelated non-spam message.  Then, when rsf exits, the message -after-
what was the first unseen message is made current.  This will cause
users to simply miss mail unless they notice (perhaps from the summary
buffer) that they've skipped one.  And if there are two prompts (such
as for an expunge), yet -another- message will be skipped.)

Both of these bugs are caused by use of rmail-first-unseen-message in an
undisciplined fashion---showing the message it denotes will side-effect
RMAIL's state, but rsf's author didn't seem to know that.  Note that bug
#1 is caused by a failure to reset the current message (from the first
unseen message to the target message) before carrying out the file &
delete.  This may have been overlooked because it will only occur if
the output file doesn't already exist, so it'll happen on the first
use and then the bug will seem to vanish, if it's even noticed at all.

[Note:  Even though I didn't do the kill-and-reload-the-buffer thing for
my main RMAIL file, which is where I first noticed this bug, the wrong
message -was- deleted.  I could verify that this did in fact happen,
because it wound up in rsf-file, and no line of that message could be
found in my RMAIL file via grep, even though it -could- be found in a
backup taken the day before.  So the message was clearly deleted, and
it was clearly filed---in fact, I wouldn't have caught the deletion at
all if I hadn't immediately looked at rsf-file (after all, I was just
starting to use rsf and was thus testing it out).  Someone who was less
paranoid could easily have simply lost an important message.  My test
case clearly shows the misfiling of the wrong message, through, and
the stealth-deletion of it as well.]

[Note:  There are a lot of FIXME's scattered through the original code;
I've removed one of them, but there are several left that I haven't
looked at.  In my own version (not in this patch), I've also added a few
small features, such as (a) auto-expunge without prompting and/or (b)
not even trying to offer to expunge and not expunging (to be left to the
user later), and I've also tightened up the prompts and feedback, but
those are -not- in this patch; if there's interest, I'll post a second
patch.  This patch is solely a bugfix.]

Patch:

--- rsf.original.el	2020-12-07 18:36:10.597449493 -0500
+++ rsf.patched.el	2020-12-07 18:40:29.811489011 -0500
@@ -216,0 +217,5 @@
+;; Don't spuriously advance to the next unseen message while prompting, because that causes it to then
+;; be -missed- while actually reading mail afterwards!  Call this instead of rmail-first-unseen-message.
+(defun rsf-rmail-last-seen-message ()
+  (max 1 (1- (or (rmail-first-unseen-message) 1)))) ; r-f-u-m can return nil in a completely empty buffer.
+
@@ -330,2 +335 @@
-          (let ((rmail-current-message msg) ; FIXME does this do anything?
-                (action (cdr (assq 'action
+          (let ((action (cdr (assq 'action
@@ -340 +344,2 @@
-                  (rmail-show-message (rmail-first-unseen-message) t))
+		   (rmail-show-message (rsf-rmail-last-seen-message) t))
+	      (setq rmail-current-message msg) ; Reset to spammy message!
@@ -380 +385 @@
-	    (rmail-show-message (or (rmail-first-unseen-message) 1) t)
+	    (rmail-show-message (rsf-rmail-last-seen-message) t)

To reproduce:

Create a file (let's call it test-rmail) with these contents:

[BEGIN]
From foo@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <foo@example.com>
From: foo@example.com
To: foo@example.com
Subject: 1
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: --------

foo.

From foo@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <foo@example.com>
From: foo@example.com
To: foo@example.com
Subject: 2
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: --------

foo.

From foo@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <foo@example.com>
From: foo@example.com
To: foo@example.com
Subject: 3
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: --------

foo.

From foo@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <foo@example.com>
From: foo@example.com
To: foo@example.com
Subject: 4
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: ------U-

foo.

From foo@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <foo@example.com>
From: foo@example.com
To: foo@example.com
Subject: 5
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: ------U-

foo.

From foo@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <foo@example.com>
From: foo@example.com
To: foo@example.com
Subject: 6
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: ------U-

foo.

[END]

Create a second file (let's call it test-spam) with these contents:

[BEGIN]
From spam@example.com  Mon Dec  7 19:05:49 2020
Return-Path: <spam@example.com>
From: spam@example.com
To: spam@example.com
Subject: spammy
Date: Mon, 07 Dec 2020 19:05:49 -0500
Message-ID: <87r1ozlffh.fsf@example.com>
MIME-Version: 1.0
Content-Type: text/plain
X-RMAIL-ATTRIBUTES: ------U-

spam.

[END]

To reproduce in the unpatched code, make a second copy of "test-rmail"
(it'll get destructively modified, and you want to be able to put it
back), ensure that "spam-output" does not exist, and:

(require 'rmail-spam-filter)
(setq rsf-file "spam-output")
(setq rsf-definitions-alist
      '(
	((from . "spam@example.com")
	 (action . output-and-delete))
	))

Type: M-X rmail-input test-rmail

If you type h, note that only message #1 is marked seen.

Type: C-U g test-spam

You'll be prompted to create spam-output; answer yes.

Note that the message whose subject is "2" is now in spam-output
(and potentially also deleted from rmail-test), and -not- the
spam!  Note also that you're magically on message #4, whereas
typing g should have only incremented the current message by
one, so you should be on message #2.

To verify the patch:  delete spam-output, regenerate test-rmail from
its backup, kill the test-rmail buffer, load the patched code, and try
the above steps again.  None of the bugs should happen any more---the
current message stays current (except for advancing by 1 only if g was
used), and the spam message---and not the first unread message---winds
up in spam-output and is deleted (and the deletion shows up in the
summary buffer as a D, instead of being stealthed via just its label).