1717147 - Move antivirus quarantining (mailnews.downloadToTempFile=true) implementation into mail store.

Status: RESOLVED FIXED

(MailNews Core :: Backend, task)

Description

[Ben Campbell]

Here's my understanding of the quarantining system (I found Bug 1144999 a useful read on this):

The POP3 code has an option (mailnews.downloadToTempFile=true) to download messages first into a temporary file, before adding it properly to the mailstore.

The reason is to allow antivirus apps to scan the individual email file and potentially quarantine it, rather than quarantining the entire mbox, and screwing up an entire folder-worth of the users email(!).
(I'm guessing that TB experiences a quarantine by the AV software as a read error when it tries to access an embargoed file? Can't really see any other sensible way for the AV software to intervene. I'm sure there are some notification hooks in Windows, say. But the the AV still needs deal with apps which don't use those, and I think faking an error is the only way).

So far, so good. But there are a few issues:

1. what about other protocols, eg IMAP? (seems to be POP3 only right now!)
2. it seems to cause a lot of strife with filters (lots of bugs reported over the years)
3. it's not needed for maildir, which already writes emails to individual files. (there's code to bypass it, depending on the type of mailstore)

My proposal is that quarantining should be handled inside the mailstore code (if required - it'd be a no-op on maildir).
This would mean it'd automagically apply to any protocol which wants offline storage, and simplifies the whole mail delivery/filter application code.

Stupid idea? Is there anything I'm missing?

Comment 1

[ISHIKAWA, Chiaki]

Probably correct idea.

Except, the implementation can be messy.

In my effort to try to enable write buffering in TB code, (the code is yet to be merge. I get distractions.)
I faced a situation where a file-class (there seem to be so many of them in TB) was used to read a file and then used to create another file object to write to a file (or was it vice versa) . This worked fine for the current non-buffered case.

However, suddenly when I tried to enable buffering and tried to create a file object that can be used to write to a file with buffering,
that creation of the new file object from anohter file object failed. Obviously, class inheritance or whatever was not complete in some classes.
There were unimplemented cases.
So I had to go through a contortion to do exactly that.
AND THAT WAS WHERE THE WRITING TO TEMPORARY FILE was done by TB for AV checking.

Oh well.

Comment 2

[Matt]

I think fixing this is not something that should be done. Removal makes more sense with the arrival of maiDir by default and an offer to users to convert existing stores. Users routinely exclude the profile folders from anti virus scanning to make the program more responsive and to work around constant hangs caused by excessive disk IO.

Having some malware in a mbox is a relatively innocuous thing, but when the AV deletes the entire file it is not so much fun, these appear in support forum as my mail for the last 20 years disappeared. Then having Thunderbird not work out the mail is gone for sometime weeks (compacting is good at locating the issue and updating the MSF) just makes Thunderbird look worse than it actually is.

I still have concerns with the integrity of the MSF database in mailDir implementations, Thunderbird does not miss entire MBox files of mail, what makes us think the database is even a vaguely reliable representation of what is on disk. The entire data image integrity really needs some sort of audit. We still see corrupt sqlite global stores, the MSF files are, for some folk, constantly out of date. I think addressing the unrelability of what should be reliable data stores needs to occur and would rather see efforts go in that direction.

I think that perhaps the best result would be to simply remove the option entirely with the adoption of maildir as default, assuming we will be offering to convert all existing stores.

Comment 3

[Magnus Melin [:mkmelin]]

Matt, I think what's proposed is to give right layer of code the right responsibility.
I would imagine once maildir is deemed ready we'd make it on by default for new accounts, but wouldn't think mbox is going away even so.

Having the mailstore handle quarantine seems correct to me.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #3)

Matt, I think what's proposed is to give right layer of code the right responsibility.
I would imagine once maildir is deemed ready we'd make it on by default for new accounts, but wouldn't think mbox is going away even so.

My mail for the last 15 years or so is in mbox format files. :-)

Also, as Matt noted regarding db stability for maildir, my take is that until there are enough adventurous users who adopt maildir as default mail format, I won't switch to it any time soon. Sorry, but that is fact of life.
Even in the age of SMS, SNS, or whatever, I still depend on e-mails on office work, etc.
TB needs the steadfast reliability we expect of a browser which one uses for online banking. All the time.
I don't mind FF crashes while browsing youtube, newspaper sites.
But TB crashing is the last thing I want to see.

Having the mailstore handle quarantine seems correct to me.

This is going to be major refactoring work, I think, though.
Well, actually, I think pop3 code is so tied up with this AV handling and Ben noted there is no such code in
IMAP. (I don't use IMAP so I don't know much about imap handing of TB
because in the early days of 2000, the available imap servers were so buggy. Today maybe people depend on gmail imap server.
Maybe, just maybe, early TB developers thought AV handling of IMAP usage ought to be best done on the server side.
After all, the imap server stores all e-mails in a searchable manner (many servers do) and thus the server knows best, sort of.

Comment 5

[Wayne Mery (:wsmwk)]

Sancus posted in matrix "About 63% of users have (at least 1) IMAP account and ~43% a pop3 account according to stats."

Comment 6

[Ben Campbell]

Attachment: WIP: Bug 1717147: Move message-quarantining from nsPop3sink into mbox implementation.

Basically working, but turns out that a bunch of places assume that an
outputstream for writing messages is also a nsISeekableStream (ie a file).
This is a holdover from mbox-only days. It's usually just to make sure we're
writing to the end of the file. But there are a few places which use it to
determine the message offset in the file. This should be handled by other
means. So I'm just going to park this patch here until I can cull out the
nsISeekableStream assumptions blocking it.

Comment 7

[Ben Campbell]

Attachment: Bug 1717147: Move message-quarantining from nsPop3sink into mbox implementation. r=mkmelin

Comment 8

[Ben Campbell]

OK, I think it's there now. There's a try run here which (I think) looks OK:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ee17294c151b374badb306c5cc6f72561c0fadaf

Comment 9

[Ben Campbell]

Holding off for now landing this for now, until Bug 1719996 is sorted. Some of the groundwork there caused trouble (Bug 1729778) and needs reworking.

Comment 10

[Ben Campbell]

OK, ready for this one now, I think. New revision has no changes, just a rebase.

Try run looks OK:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f6efe59e2aa31e043ba7cf896af2d486f2cbd98e

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/35e064ada8de
Move message-quarantining from nsPop3sink into mbox implementation. r=mkmelin

Comment 12

[gene smith]

Sorry, I put this comment on the wrong bug. It can be deleted if that's possible.