Closed Bug 1702692 Opened 3 years ago Closed 3 years ago

Embedded image not displayed if invalid header is found after bug 1681487 for some IMAP servers

Categories

(MailNews Core :: Networking: IMAP, defect)

Product:
MailNews Core
Component:
Networking: IMAP
Platform:
x86_64
All
Type:
defect

Tracking

(thunderbird_esr78 wontfix)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 78.0
Tracking Flags:
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: rjshelq, Assigned: gds)

References

(Blocks 1 open bug, Regression)

Details

Attachments

(2 files, 4 obsolete files)

Bowl of Saki for April 1.eml
69.35 KB, message/rfc822
Details
more-tolerant-offline-msg-record-check.patch
12.21 KB, patch
benc
: review+
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/89.0.4389.90 Safari/537.36

Steps to reproduce:

After updating from TB 78.7 to TB 78.8.0, I no longer see inline images in html emails which use the <div> tag around the image source.

I have two Windows 10 machines, and both of them properly displayed the inline images using TB 78.7, but after updating to 78.8.0, both machines fail to display inline images inside of <div> tags, and only display the alt text.

It appears that something changed in the TB rendering engine in the jump from 78.7 to 78.8, and now the inline image specified inside <div> tags is not shown at all.

The embedded image in the email is displayed properly in all other email clients that I've tested including Apple Mail on Mac, Apple Mail on iPad, K9 Mail on Android, Microsoft Mail for Win 10, Microsoft Webmail, Google Webmail, and Google Mail on Android.

For example, this fragment of the html code in the email fails to display an image embedded in the email using TB versions 78.8 or 78.9, but worked fine up through 78.7:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=us-ascii">
<title>test page</title>
</head>
<body>
	
<div>
	<img style="width:99%; max-width:450px; display:block; margin-left:auto; margin-right:auto" alt="Nature photo with quote" src="cid:fbimage040121" >
</div>
	
<div>
	<br>
	<p style="font-family: Arial, Helvetica, sans-serif; font-size:15px">
			Commentary for Consideration:
	</p>
    </div>

P.S.
An entire sample email is included as an attachment to this bug report.

Actual results:

The image defined inside the <div> tags is no longer shown by TB 78.8 or 78.9. Prior to that, there had never been a problem.

Expected results:

The embedded image in the email is displayed properly in all other email clients that I've tested including Apple Mail on Mac, Apple Mail on iPad, K9 Mail on Android, Microsoft Mail for Win 10, Microsoft Webmail, Google Webmail, and Google Mail on Android.

Interestingly, If the same email is saved as an eml file, and then TB is used to open that file, the image is properly displayed. But the original email in TB still fails to display the image, with only the alt text displayed instead of the image.

Component: Untriaged → Message Reader UI
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64

Picture displays fine when message is imported into a folder. It shows a few purple flowers and text "When one praises ...". Where's the problem?

I can confirm the bug when "Menu: View -> Display Attachements Inline" is disabled.
If I enable the option, I can see the image.

In my opinion the MIME structure is a bit broken. That's why the image is treated as an attachment.

Really? That shouldn't make any difference and for me it doesn't make any difference in TB 78.9. Clearly the picture is embedded. You have multipart/related outside, multipart/alternative inside with text/plain and text/html and at the end the image as an alternative part. Let's not discuss that this isn't the way TB encodes HTML e-mail, but TB has always supported this somewhat defective structure (see for example bug 1251783). The image is NOT treated as attachment, for an attachment you need multipart/mixed which is not used.

(In reply to José M. Muñoz from comment #4)

Really? That shouldn't make any difference and for me it doesn't make any difference in TB 78.9.

I also tested it with TB78.9 (and a slightly older trunk build). If I open the mail from the file, it makes indeed no difference as the reporter describes it. But from an IMAP folder it does.

@rjshelq: does enabling that setting fix your Problem?

but TB has always supported this somewhat defective structure (see for example bug 1251783). The image is NOT treated as attachment, for an attachment you need multipart/mixed which is not used.

Yes, this is probably a regression.

Flags: needinfo?(rjshelq)

My mistake, yes, if you import the message into an IMAP folder the image doesn't display. It does however display in TB 68. I had tried in a local folder.

So, Alfred, can you please mark this as regression and change the subject to:
"Embedded image not shown for message with MIME structure outer: multipart/related, inner: multipart/alternative in IMAP folder".
Also, please move the bug to MailNewsCore::Networking IMAP. Note that IMAP analyses the so-called "body-structure" of the message and doesn't fetch parts it doesn't deem necessary, like attachments aren't fetched if they are not inlined. However, since the advent of OpenPGP (bug 1629292), the pref mail.server.default.mime_parts_on_demand is false, so all parts should always be fetched. So this is doubly-puzzling.
BTW, setting or switching off inline attachments doesn't appear to make a difference for me, right now, TB 78 doesn't show the image, no matter what.

Alice, can you please find the regression for us.
STR:

  • Switch "View > Display Attachments Inline" off -> Note that may not be necessary, but it won't hurt.
  • Import message into IMAP folder, hopefully Gmail will do (I haven't tested that).
  • Check whether the image is displayed.
    For me, it displays in TB 68 but not TB 78.

Gene, here's work for you.

Flags: needinfo?(infofrommozilla)
Flags: needinfo?(gds)
Flags: needinfo?(alice0775)

(In reply to José M. Muñoz from comment #6)

  • Import message into IMAP folder, hopefully Gmail will do (I haven't tested that).

How do I import message into IMAP folder? Step by step please.

Flags: needinfo?(alice0775)

Save attachment 9213258 [details] to your desktop, then drag it only an IMAP folder. As I said, I hope it fails for Gmail.

(In reply to José M. Muñoz from comment #8)

Save attachment 9213258 [details] to your desktop, then drag it only an IMAP folder. As I said, I hope it fails for Gmail.

When drag and drop the eml to inbox of gmail from desktop, an error popups. It cannot import....

Yes, sadly so. It also fails for a subfolder of the inbox: "Unable to parse message." :-(
Do you have any other IMAP account at your disposition? Hotmail/Office365 or some local ISP?

(In reply to José M. Muñoz from comment #10)

Yes, sadly so. It also fails for a subfolder of the inbox: "Unable to parse message." :-(
Do you have any other IMAP account at your disposition? Hotmail/Office365 or some local ISP?

IMAP hotmail.com and yahoo.co.uk successfully imported.
And an image(flower) is shown in the eml. It seems work for me.

Thanks for testing. I've sent you another IMAP server to try. Sorry, these problems are sometimes hard to reproduce. I'm a bit surprised since the reporter has an MSN account, that's Hotmail, no? Also, did you switch "View > Display Attachments Inline" off? It didn't make a difference for me, but Alfred suggested it.

You're the best!!

It could only come from bug 1681487 which got backported to TB 78.8.0. Uff, exactly what the reporter said, 78.8.0 affected.

I can confirm that the email server which delivered the troublesome email to TB, which then refuses to show the embedded image, does indeed use Dovecot. So it appears that you're on the right track!

Flags: needinfo?(rjshelq)
Blocks: MIME-robustness-tracker
Status: UNCONFIRMED → NEW
Component: Message Reader UI → Networking: IMAP
Ever confirmed: true
Flags: needinfo?(infofrommozilla)
OS: Windows 10 → All
Product: Thunderbird → MailNews Core
Regressed by: 1681487
Summary: TB windows versions 78.8, 78.9 fail to display embedded images defined inside <div> tags → Embedded image not shown for message with MIME structure outer: multipart/related, inner: multipart/alternative in IMAP folder
Target Milestone: --- → Thunderbird 78.0

OK, I found the culprit: It's not the MIME structure, it's this first invalid header X-Proc-Disposition=blacklist-pass that makes the whole thing choke on some servers. If you remove it, the message displays fine. As we pointed out in comment #9, Gmail even refuses to store the message. That's related to the changes made in bug 1681487.

So I suggest to change the summary of the bug to: "Embedded image not displayed if invalid header is found after bug 1681487 for some IMAP servers".

Sorry Alfred, wires crossed.

Flags: needinfo?(infofrommozilla)

(In reply to José M. Muñoz from comment #16)

OK, I found the culprit: It's not the MIME structure, it's this first invalid header X-Proc-Disposition=blacklist-pass that makes the whole thing choke on some servers. If you remove it, the message displays fine.

Confirmed.

Flags: needinfo?(infofrommozilla)
Summary: Embedded image not shown for message with MIME structure outer: multipart/related, inner: multipart/alternative in IMAP folder → Embedded image not displayed if invalid header is found after bug 1681487 for some IMAP servers

Using the attached email, I see a picture of purple flowers with some text on top of it, followed by normal text with some links interspersed.

However, the summary line for the message is mostly empty. Only see a time of day, no subject or who it's from.

Any idea how the "bad" header
X-Proc-Disposition=blacklist-pass
got there? I assume the "correct" format should be
X-Proc-Disposition: blacklist-pass

FWIW, don't see a diff between Inline or not inline attachments.
Also, I'm looking with bug 1681487 changes in place.

Flags: needinfo?(gds)

As Alice above noted, when you drop the message onto a gmail folder it responds that the message is invalid:
2021-04-05 18:47:42.544157 UTC - [Parent 28239: IMAP]: I/IMAP 0x7f4d77203000:imap.gmail.com:S-gds:CreateNewLineFromSocket: 14 BAD Invalid Arguments: Unable to parse message
My charter server accepted it OK but the message is definitely corrupt since header items in summary are blank.

Drop onto dovecot folder with no error. Header in summary are all present (unlike charter). However, have to set "display attachments inline" to see the picture. Otherwise just see text "Nature photo with text" in its place.

There's definitely a problem with the "fix" in bug 1681487. It fixed the case for the reporter of that bug but doesn't handle the weird "blacklist-pass" line on this reporter's message. I'm not sure what is causing the problem but I'll look at it more tomorrow.
Problems I'm seeing:

  • mbox file grows each time the message is opened
  • blacklist line causes message to be treated as not stored for offline so using memory cache
  • see assertion crash on aNew in mem cache code in nsImapProtocol.cpp (debug build only)
  • message getting fetched by parts using bodystructure even with fetch mime on demand false
  • each mime part fetched as well as full body is fetched per msg open
  • works mostly OK on openwave server because it returns a "flat" bodystructure so no fetch by parts
  • openwave doesn't see basic header items like subject, full date etc but message displays OK
  • gmail rejects append due to "blacklist" line (probably good), others accept it.

Ok, looked at this again today. The "regression" here is actually that the check for "From " is now correct. At the point in the file where the "blacklist" line begins we are now looking for it to start with "From " since a bogus "From " is OK at that point. The current 78.8.x code looks for a "From " here and doesn't see it in the "blacklist" line so it fails. This causes the message to be re-fetched in parts so the pictures does not appears. With the the 78.7.x code and earlier, there was a bug that caused the "From " to be looked for at the beginning of the "200 byte peek buffer" instead of after the X-MOZILLA-STATUS* lines. The peek buffer starts with "From -" so the check passed and the message was just read directly from the file with no network activity.

The bug in the <=78.7 code was simultaneously found by Ben during his review and by me independently: See bug 1681487 comment 13 and bug 1681487 comment 14.

The "fetch by parts" that occurs is independent of the change made in bug 1681487 so that probably also needs to be fixed somehow. But its also possible to work-around this bug by just ignoring the line that appears here (marked "ignore this"):

From - Tue Apr  6 18:23:17 2021
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
X-Proc-Disposition=blacklist-pass  <-- ignore this
Return-Path: <ishq2-saki-owner-ishq2=wahiduddin.net@wahiduddin.net>
X-Original-To: ishq2@wahiduddin.net
Delivered-To: ishq2@wahiduddin.net

So instead of returning an error because the 4th line doesn't look like a valid header line, we just let it go. This fixes the problem in this bug and it also fixes the problem reported in bug 1681487. So the only requirement is for the first 3 lines to be like below and anything after that is just accepted.:

From - Tue Apr  6 18:23:17 2021
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000

This was discussed in bug 1681487 comment 14 but decided not to do it at that time.

Looking deeper into this I see several problems.

  1. When the problem with the header in the mbox file is found, a flag is set for the message marking it as "not offline" which means the offline store (in this case the mbox file) is invalid for the message due to the invalid header line containing "blacklist".
    This causes the message to be fetched again but this time instead of saving it to offline store, it just is saved in memory cache. However, this also fails because the the code here
    https://searchfox.org/comm-central/rev/08111d84bf909f3ecf367618f823ee605658472b/mailnews/imap/src/nsImapProtocol.cpp#9453
    thinks the URL is "local only" meaning it is stored for offline. This causes Cancel() to be called which "dooms" the just obtained cache entry. So the problem is that it is not know that that message was marked as "not offline".
    So if I comment out the "if (localOnly)" block starting here
    https://searchfox.org/comm-central/rev/08111d84bf909f3ecf367618f823ee605658472b/mailnews/imap/src/nsImapProtocol.cpp#9449
    that problem is fixed (actually hidden) and the full message is stored to memory cache. Of course this is not a valid "fix".

  2. With the full message in memory cache, it is then read. However, this also fails for the same reason it failed when read from offline store: The first header line is seen as invalid. This is checked here:
    https://searchfox.org/comm-central/rev/08111d84bf909f3ecf367618f823ee605658472b/mailnews/imap/src/nsImapProtocol.cpp#9350
    So if I comment out the "if (shouldUseCacheEntry)" block that "peeks" at the 1st 100 bytes in cache, this is also fixed. (Again, not a valid fix.)

  3. With these two change and with autosync still enabled, the message is fetched correctly and the picture appears. However, autosync apparently doesn't know the message was fetched and stored to memory cache so it still downloads the message every minute or so. This causes the mbox file to grow.

  4. If I turn off autosync, for some reason the first access attempts to read from memory cache. Since nothing was stored to memory cache before the attempted read, cache read fails and the picture still does not appear like before but the full message does get stored to memory cache when the message is fetched. If you move off of the message and then come back, the full message is successfully read from memory cache and the full message with the picture is displayed now.

So there are several issue to resolve here to do a correct fix.

I wrote in comment 22:

So instead of returning an error because the 4th line doesn't look like a valid header line, we just let it go. This fixes the problem in this bug and it also fixes the problem reported in bug 1681487. So the only requirement is for the first 3 lines to be like below and anything after that is just accepted.

Probably not a good idea since this check was added as part bug 531033 to address problems with randomly corrupted stored messages; not for messages with a known bad line as with the "blacklist" header line of this reporter's file.

See Also: → 531033
Attached patch possible-fix.diff (obsolete) — — Splinter Review

This seems to fix the problem even with autosync enabled.

I've added some MOZ_LOG items for test purposes and commented out an assertion. The "fix" is where I now get the "offline" flag which requires quite a bit of code in two places.

I need to check this again tomorrow to make sure it is really working. I've been fooled before. :)

Assignee: nobody → gds
Attachment #9214590 - Flags: feedback?(benc)
Comment on attachment 9214590 [details] [diff] [review]
possible-fix.diff

Review of attachment 9214590 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few things clang-format won't like incl. some trailing spaces. A lot of effort for a bad header. Can't that just be ignored somehow? Aren't we treading on dangerous ground with this assumption "probably just an invalid first header line"? What happened before bug 1681487 in this situation?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +9032,5 @@
>        MOZ_LOG(IMAPCache, LogLevel::Debug,
>                ("OnCacheEntryAvailable(): Trying to read part from entire message"));
>        // clang-format on
>        // We are here with the URI of the entire message which we know exists.
> +//      MOZ_ASSERT(!aNew,

Why is that removed? Do the assumptions made here no longer hold?

@@ +9498,5 @@
>  
>    bool localOnly = false;
>    imapUrl->GetLocalFetchOnly(&localOnly);
> +
> +  if (localOnly) {

Looks like the code that follows is copied from above. Maybe put that into a helper function.

There are a few things clang-format won't like incl. some trailing spaces. A lot of effort for a bad header. Can't that just be ignored somehow?

As I pointed out just ignoring a bad 1st header line is an option (comment 22). But it has been in place for over 12 year starting with bug 531033 as I described in comment 24.

Aren't we treading on dangerous ground with this assumption "probably just an invalid first header line"?

At that point we see there is an invalid header line in memory cache. We also know there was an invalid header in offline store on disk. So it's a reasonable assumption that it's not just a random error that will clear on the next fetch from server. So just go ahead and use what's in memory cache.

What happened before bug 1681487 in this situation?

Without the changes in my proposed diff, the picture in the user's email would not display, just the text that described it would appear. Also, autosync caused the message to be added to the mbox file every few minutes so the file grew continuously. (This is with fixes from bug 1681487 in place.)

Without bug 1681487 fixes, the mbox file is used to obtain the message and the "bad" 1st header line containing "blacklist" was ignored. The intention of the 12 year ago fix was to allow this line to be valid it it contain a colon or if it started with "From " both of which in this case it doesn't. However there is a bug in the pre-bug 1681487 fixes that caused it to falsely conclude that the line starts with "From " so it caused the bad line to be accepted and no fall back to re-fetching to memory cache occurred and the message displayed correctly. (I described this before in comment 22.)

The primary problem addressed in bug 1681487 was due to the 1st header line started with "From " but there was no colon or end of line marker within the "peek" buffer so the "From " was not even checked for causing line to be deemed invalid. Bug 1681487 fixed this to still allow the leading "From " to be checked for even if no colon or end of line marker is found in the peek buffer at the 1st header line. The user complaint here was just that the mbox file grew; the message displayed OK in this case.

Given the above problems introduced with bug 531033 12 years ago, that fix may actually have caused more problems than it solved. Examples:

Rejects:
From xxxxxxxxxxxxxxxxxxx // this is actually good
xxxxxxxxxxxxxxxxxxxxxxxx // this is actually bad
Accepts:
xxxxxxxxxxx:xxxxxxxxxxxx // this is actually good
xxxxxxxxxxxxxxxxxxx\r\n // this is actually bad

So just not checking the 1st header line might be the best solution as I suggested in comment 22 since the feature has been mostly broken over the last 12 years.

I must say that I really don't quite understand what bug 531033, bug 663761 and bug 1681487 were about. In my reading I think they are about checking that "the database has the correct offset into the offline store", that's what the comment says.

So you go to the offline file at this offset and you start reading and you now need to distinguish whether the stuff you're reading is a valid message start or not. Is it any harder than that? So what are the variations. These look valid:

From - Tue Apr  6 18:23:17 2021
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000

and

From - Tue Apr  6 18:23:17 2021
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
X-Proc-Disposition=blacklist-pass  <-- ignore this
Return-Path: <ishq2-saki-owner-ishq2=wahiduddin.net@wahiduddin.net>
X-Original-To: ishq2@wahiduddin.net
Delivered-To: ishq2@wahiduddin.net

and

From - Wed Dec  9 10:25:51 2020
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
From 3ST5YWzsPAAgl+UbM8WbcAV7c7VLY5V7QCUbc5UbQ7-UbIBVLc6VbYBULIDWbkBVroCVrU7lwk0.owwotm.kwu@docos.bounces.google.com Wed Jul 25 09:09:33 2018

After the first three lines you know that your offset is valid, so why look any further? What am I missing?

Looking at
https://searchfox.org/comm-central/rev/8a7261cd704a6070bc43a4f4ea88e2b33cf7934f/mailnews/base/src/nsMsgDBFolder.cpp#726
again, I think the code makes the mistake to go looking for valid headers after it already saw three valid lines.

The problem here is that an "invalid" line follows, the problem in bug 1681487 was that a long "bogus From" followed:

From 3ST5YWzsPAAgl+UbM8WbcAV7c7VLY5V7QCUbc5UbQ7-UbIBVLc6VbYBULIDWbkBVroCVrU7lwk0.owwotm.kwu@docos.bounces.google.com

I think, this whole check implemented in bug 531033 attachment 492804 [details] [diff] [review] and modified here https://hg.mozilla.org/comm-central/rev/e6a7df2f0c20 should be mostly "inactivated".

My approach would be this:

  • Check first line for "From " or "FCC".
  • Check next two lines for X-Mozilla-Status: and X-Mozilla-Status2: --> If present, do not check any further.
  • Now you're at the point where you didn't see a valid three-line-header. Does that ever happen? If it happens, you can still check for "From " again, or for a header with a colon.

I think, this whole check implemented in bug 531033 attachment 492804 [details] [diff] [review] [diff] [review] and modified here https://hg.mozilla.org/comm-central/rev/e6a7df2f0c20 should be mostly "inactivated".

My approach would be this:

Check first line for "From " or "FCC".
Check next two lines for X-Mozilla-Status: and X-Mozilla-Status2: --> If present, do not check any further.

Actually, I think the present code is wrong. I made it so that all 3 lines are mandatory when in the original code,
https://searchfox.org/comm-central/rev/879c3786e0ae220f737b13f3bc50989b9ba2b4b9/mailnews/base/src/nsMsgDBFolder.cpp#731
it looks like X_MOZILLA_STATUS is optional; but if present then X_MOZILLA_STATUS2 is optional. So actually only the "From " or "FCC" line are mandatory. The X-MOZ lines are just skipped over so you return the file offset for further reading to the 1st real header.

Anyhow, yes what you suggest, ignore the format of the 4th line (actually the first header line from the server) is my proposal.

Now you're at the point where you didn't see a valid three-line-header. Does that ever happen?
If it happens, you can still check for "From " again, or for a header with a colon.

Does it happen? I've not seen reports of that happening and when I look at mbox files I see the 3 leading lines. But if it does, check the next line for "^From " or contains a colon and call it good if it does and read from mbox. Otherwise, just re-fetch into memory cache and obtain the data from there. It either works (displays the message) or it doesn't. I.e., make no other changes in the cache code. This fixes both recent bugs.

Attached patch t2.diff (obsolete) — — Splinter Review

Let me propose this now:

Check the 1st line for "From " as we always have. If this fails, drop back to re-fetch to memory cache and don't check any more lines for validity from the mbox file.

The next two lines X-MOZILLA-STATUS and X-MOZILLA-STATUS2 are optional and are just skipped if present which is how the code now works and I actually didn't make either mandatory.
Note: These X-MOZ* lines as well as the dash and date after the initial "From " are removed when an IMAP folder is compacted.

Check that the next line starts with "From " or has a colon. If so, we're good and go ahead and use the data from the mbox file.

If not, check if the next line starts with "From " or has a colon. If so, we're good to use the mbox file. If not, call this an error and force re-fetch and store to memory cache.

This will fix this bug's reporter's issue as well as the previous bug 1681487. However, if memory cache still must be used, the message will be fetched so the user sees it but other problems like missing attachments and growing mbox files may still occur. I haven't been able to get a handle on these issues but they should be much rarer with this proposed change.

I also removed some dead code (a function never called) and made the "peek" buffer 100 bytes larger. Being larger would have also fixed the problem from bug 1681487.

Attachment #9214590 - Attachment is obsolete: true
Attachment #9214590 - Flags: feedback?(benc)
Attachment #9215293 - Flags: feedback?(benc)
Comment on attachment 9215293 [details] [diff] [review]
t2.diff

Review of attachment 9215293 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to say f+ here as it seems like an improvement on what was there already.
It certainly seems like it won't work properly on servers using maildir storage (they don't have "From " lines between messages, although messages _could_ still begin with a "From " sent down from an errant IMAP server, say).
The whole function just seems a bit problematic.

::: mailnews/base/src/nsMsgDBFolder.cpp
@@ +696,4 @@
>      // check if message starts with From, or is a draft and starts with FCC
> +    if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) ||
> +                          ((mFlags & nsMsgFolderFlags::Drafts) &&
> +                           !strncmp(startOfMsg, "FCC", 3)))) {

This seems bad (it was bad in the original code) - if we're using maildir, there should never be a "From " line (not quite sure about FCC). In that case, it'll skip this whole block and return with NS_OK, without skipping the X_MOZILLA_STATUS lines.

@@ +696,5 @@
>      // check if message starts with From, or is a draft and starts with FCC
> +    if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) ||
> +                          ((mFlags & nsMsgFolderFlags::Drafts) &&
> +                           !strncmp(startOfMsg, "FCC", 3)))) {
> +      startOfMsg[bytesRead] = '\0';

I think this needs to be moved up before the strncmps - otherwise the strncmps have the potential of reading into uninitialised bytes in startOfMsg.

@@ +721,5 @@
>          // startofMsg[msgOffset] should now be the first valid header line that
>          // will be read. Check that it appears to be a  true header line by
>          // checking that it contains a colon, ':'. Also, allow the line to
> +        // start with "From " since some broken IMAP servers return an initial
> +        // "From " line without a colon instead of a valid header.

(note to myself more than anything: So, presumably whatever code it is that adds the X_MOZILLA_STATUS headers, writes them first, then outputs the message verbatim, even if IMAP server sent us a message starting with a bogus "From ".  This seems right - we should keep all the data from the server intact in a single unchanged chunk, even if we are prefixing custom headers.)

@@ +732,5 @@
>            *size -= msgOffset;
> +          rv = NS_OK;
> +          printf("gds: 1st chance OK\n");
> +        } else if (findPos != kNotFound) {
> +          // No colon or "From" but containes EOL marker. Go ahead and tolerate

contains
Attachment #9215293 - Flags: feedback?(benc) → feedback+

Stepping back, my reading on what nsMsgDBFolder::GetOfflineFileStream() is aiming to do:

it returns an InputStream, positioned to read the first header of the message, having skipped:

  1. any mbox "From " lines
  2. leading X_MOZILLA_STATUS headers
  3. leading "From " line from an errant IMAP server
  4. new for this bug: skip any other badly-formed headers we want to tolerate (eg "X-Proc-Disposition=blacklist-pass").

If the function can't do that, it marks the message offline and presumably the calling code downloads it from the server (which I guess must also duplicate it again in the mbox/maildir!)

Does that sound about right?

More practically, for this bug, I'd propose nsMsgDBFolder::GetOfflineFileStream() should:

  • check for and skip an initial "From "/"FCC" line (optional - maildir shouldn't have one).
  • skip any leading X_MOZILLA_STATUS[2] headers
  • allow one bad line before a valid header (a "From " line, a "X-Proc-Disposition=blacklist-pass" bad header.... anything).

Which isn't far off from what the patch does.

Looking at the bigger picture:
I don't think it should be doing 1). mbox/maildir should be encapsulated entirely within the plugablemailstore. But the mailstore interface currently punts on this.
2) is a bit icky, but valid. Adding the X_MOZILLA_STATUS headers means that all the metadata we need to rebuild the message database is present in the mbox/maildir data. (not sure how X_MOZILLA_STATUS interacts with anything that needs to checksum/hash/sign/encrypt the message. Something to investigate).
3) and 4) are the same thing, I think. The question is where badly-formed emails are handled. I kind of think it shouldn't be handled here - we should be aiming to return the original message verbatim - exactly as the server sent it to us, warts and all. But I don't know the knockon effects on the rest of the code...

Also I don't think it should mark bad messages as offline. That seems like something the calling code should deal with. It should just return an error and be done with it.

Attached patch t3.diff (obsolete) — — Splinter Review

It certainly seems like it won't work properly on servers using maildir storage (they don't have "From " lines between messages, although messages could still begin with a "From " sent down from an errant IMAP server, say).
The whole function just seems a bit problematic.

::: mailnews/base/src/nsMsgDBFolder.cpp

@@ +696,4 @@
>      // check if message starts with From, or is a draft and starts with FCC
> +    if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) ||
> +                          ((mFlags & nsMsgFolderFlags::Drafts) &&
> +                           !strncmp(startOfMsg, "FCC", 3)))) {

This seems bad (it was bad in the original code) - if we're using maildir, there should never be a "From " line (not quite sure about FCC). In that case, it'll skip this whole block and return with NS_OK, without skipping the X_MOZILLA_STATUS lines.

I don't run with maildir but I have tried it in the past and have some archived maildir files produced by tb and they do have the same lines as mbox, e.g., taken from a file called .../cur/1545970334263000 :

From - Thu Dec 27 23:12:14 2018
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Return-Path: <orealius@juno.com>
Received: from impin007 ([68.114.189.31])
:

The original code would have rejected this if it didn't start with the "From ".

In my new t3.diff I now fail and mark offline if the initial "From " is missing. In the previous diff, a missing leading "From " was accepted as you pointed out. But if maildir file format has changed and no longer has the leading "From -" then the problem you point out does exist.

@@ +696,5 @@

>      // check if message starts with From, or is a draft and starts with FCC
> +    if (NS_SUCCEEDED(rv) && (!strncmp(startOfMsg, "From ", 5) ||
> +                          ((mFlags & nsMsgFolderFlags::Drafts) &&
> +                           !strncmp(startOfMsg, "FCC", 3)))) {
> +      startOfMsg[bytesRead] = '\0';

I think this needs to be moved up before the strncmps - otherwise the strncmps have the potential of reading into uninitialised bytes in startOfMsg.

Good point! I put it back where it was. Not sure what I was thinking by moving that line.

> +          // No colon or "From" but containes EOL marker. Go ahead and tolerate

contains

Fixed.

I was thinking this function is only used with mbox, but I guess not. There are several callers of this so having each place deal with the return code and marking as offline sounds like a pretty big change.

From my reading of the original rationale for this, the theory was that the file was somehow corrupted but the server content was OK. So if an mbox file record got written again with the server content it would be "good" after that (leaving a single orphan record). But it seems that we mostly just see bad headers in the server which cause the mbox file go "grow" due to being marked offline causing lot of orphan records. Not sure if this applies ot maildir, i.e., would you end up with lots of duplicate orphan maildir files?

Note: Repair or compact doesn't get rid of the orphan mbox records. You have to manually delete the mbox and msf files and let them re-download (with the bad header hopefully fixed in the server or you end up back where you were).

Attachment #9215293 - Attachment is obsolete: true
Attachment #9217904 - Flags: feedback?(benc)

(In reply to gene smith from comment #33)

In my new t3.diff I now fail and mark offline if the initial "From " is missing. In the previous diff, a missing leading "From " was accepted as you pointed out. But if maildir file format has changed and no longer has the leading "From -" then the problem you point out does exist.

Emails stored in maildir should definitely not have the leading "From " line. It's part of the mbox format (such as it is). However, I think our initial pass at a mbox->maildir converter did leave in the "From " lines, so they can still appear out in the wild. Sigh.

So. I think the steps here are, in order:

  1. Skip past any "From " line, if present.
  2. Skip past the X_MOZILLA_STATUS lines, if present.
  3. Skip past one bad header line, if present.
  4. Succeed if we're now on a valid header line.

maybe there should also be a step 2.5:
2.5) skip past another "From " line if present (that a naughty IMAP server might has sent us).
But step 3 would soak that up (at the expense of not tolerating a bad header also).

Anyway. If you want to modify your patch to do this, I think that sorts out this bug.
Bigger issues we can open new bugs for.

I was thinking this function is only used with mbox, but I guess not. There are several callers of this so having each place deal with the return code and marking as offline sounds like a pretty big change.

Yup.
Step 3 bothers me a little. It's obviously a real issue - emails with bad headers... but I don't see why the bad header should always be the first one. Surely there could be bad headers anywhere in the header section of the message?
But that would imply dealing with bad headers elsewhere, which gets a bit out of scope for this bug.

From my reading of the original rationale for this, the theory was that the file was somehow corrupted but the server content was OK. So if an mbox file record got written again with the server content it would be "good" after that (leaving a single orphan record). But it seems that we mostly just see bad headers in the server which cause the mbox file go "grow" due to being marked offline causing lot of orphan records.

That's a good point.
It seems like the whole 'mark bad messages as offline and grab a new one from the server' premise is flawed, and causing way more problems that it fixes. I'd be inclined to ditch that behaviour completely (after doing a bit of Bugzilla archeology to see if we're missing anything).
If local files are being corrupted there are probably bigger things to worry about!

Not sure if this applies ot maildir, i.e., would you end up with lots of duplicate orphan maildir files?

I'm not sure either. I think it should dupe them, but I'll do some checking (I've got lots of maildir work to do).

Note: Repair or compact doesn't get rid of the orphan mbox records. You have to manually delete the mbox and msf files and let them re-download (with the bad header hopefully fixed in the server or you end up back where you were).

How about this:
your patch is just about there for fixing this specific bug, so let's finish it off and land it.
Make the leading "From " line optional, to handle maildir.

Then we open new bugs to look at:

  1. removing the whole mark-bad-messages-offline approach (which leads to runaway mbox sizes)
  2. moving bad-header detection out of here and offloading it onto places which are actually in a position to deal with it properly (eg some places (sending) might just filter out bad headers, other places might want to treat them as fatal).
Attachment #9217904 - Flags: feedback?(benc)

Emails stored in maildir should definitely not have the leading "From " line. It's part of the mbox format (such as it is). However, I think our initial pass at a mbox->maildir converter did leave in the "From " lines, so they can still appear out in the wild. Sigh.

Ok, trust but verify...
I usually run w/o offline store but with default mbox set. I changed an account to maildir and let it convert and restarted TB and enabled offline store for Inbox on the account. I now see the maildir files under cur/ as a bunch of .eml files. Each .eml file looks just like a the mbox record and all begin with "From - <date>", e.g.:

From - Thu Apr 22 20:12:42 2021       // In maildir eml file and in mbox file!
X-Mozilla-Status: 0001                // In maildir eml file and in mbox file!
X-Mozilla-Status2: 00000000           // In maildir eml file and in mbox file!
Return-path: <gds@tana.it>
Original-recipient: rfc822;gd.smth@icloud.com
Received: from st11p00im-smtpin003.me.com by pv43p53ic-ztdg05063201 (mailgateway 2022B134)
	with SMTP id 064fe32b-2b6b-4a26-9d54-5559c1aacfaf 
	for <gd.smth@icloud.com>; Tue, 24 Nov 2020 20:49:13 GMT
X-Apple-MoveToFolder: INBOX 
:

I also sent a new mail to the account and I see the new .eml file there with the same header set. So, unless I'm confused and missing something, it looks like the maildir eml files are just like the concatenated messages in the mbox file.

I'm running a trunk version that I just updated maybe yesterday with both mozilla and comm code.

Finally, I checked with a printf that GetOfflineFileStream() is called for maildir and it is. That would mean that with the original version if there is no leading "From " in the file it would cause every access to result in setting the flag to force a re-fetch to memory cache (and possibly duplicate .eml files) which I don't think is happening. So it seems that the maildir eml files have the same headers as mbox. (Again, maybe I'm confused or missing something.)

(In reply to gene smith from comment #35)

So it seems that the maildir eml files have the same headers as mbox. (Again, maybe I'm confused or missing something.)

(disclaimer: just a quick reply right now - I haven't investigated this very deeply)

The maildir files shouldn't have the leading "From " line in them. I would guess that the code which writes out emails always puts in that "From " line, even if it's writing to a maildir store. Just found Bug 1686852, which already covers this.

I'm pretty sure the mbox->maildir conversion code does strip out the "From " lines (ah yes - Bug 1135309 covers removing the "From " line in mbox->maildir conversion). So I'd guess existing emails converted from mbox don't have the "From " line, whereas new emails added after the initial conversion do. I'll follow up on all this next week and write it up.

Bottom line: for now, GetOfflineFileStream() needs to treat the leading "From " as optional. It shouldn't be there on maildir (even if it often is right now!).

Ok, I see what you mean. FWIW, here's what I observe:
If I convert from mbox with no offline store (no mbox files present) to maildir with offline store, the result is maildir files with "From -".
If I convert from mbox with offline store (mbox files present) to maildir with offline store, the result is maildir files without "From -".
Any received email into an maildir folder with offline store results in a new file with "From -".

Also, when maildir files don't have the "From -" they look like this: 1619224675411.eml
and with the "From -" they look like this: 62dabf9f-b3aa-d7f6-0608-61a8164798e9@chartertn.net.eml

And, as I expected, with the current trunk GetOfflineFileStream(), when maildir messages without the "From -" are accessed, a fetch from the server occurs and the maildir file is not used to show the message. However, there is no redundant maildir data or new file written as occurs with mbox where redundant message records are appended on each fetch.
Edit: This is wrong. See comment 40.

Bottom line: for now, GetOfflineFileStream() needs to treat the leading "From " as optional. It shouldn't be there on maildir (even if it often is right now!).

Got it.

(In reply to gene smith from comment #37)

Ok, I see what you mean. FWIW, here's what I observe:
If I convert from mbox with no offline store (no mbox files present) to maildir with offline store, the result is maildir files with "From -".
If I convert from mbox with offline store (mbox files present) to maildir with offline store, the result is maildir files without "From -".
Any received email into an maildir folder with offline store results in a new file with "From -".

Cool - thanks for confirming that!

Also, when maildir files don't have the "From -" they look like this: 1619224675411.eml
and with the "From -" they look like this: 62dabf9f-b3aa-d7f6-0608-61a8164798e9@chartertn.net.eml

Ahh, that I can explain. I changed the maildir code ages ago to use a sanitised version of the mailID as the filename (eg 62dabf9f-b3aa-d7f6-0608-61a8164798e9@chartertn.net). For the mbox-maildir converter, I didn't want to slow it down by forcing it to parse the emails for the mailid header, so I left it with the old system of using a timestamp (I think) as the basis of the filename. The two schemes coexist just fine, but it really really irks me that the mbox-maildir conversion is a separate thing, and doesn't just use the exisiting mailstore code to handle it (it has to do with multithreading and xpcom). I'd really like to unify it properly one day...

See Also: → 663761
Attached patch t4.diff (obsolete) — — Splinter Review

I kept running into test errors when I re-structured my previous version, t3.diff, to work also with maildir message records. So I re-did the code in the form of "while" loop which I think makes it clearer as to what is going on and easier to test and verify that it does what it should.

What it does:

  1. Read the first 299 bytes from the start of the message offline store file to a buffer (mbox or maildir based, doesn't care).

  2. Each pass though the loop looks at consecutive lines in the buffer. When the message record is determined to be good or bad no more lines in the buffer are looked at and the loop is exited.

  3. If first line starts with "From " it is ignored.

  4. If consecutive lines start with "X-Mozilla-Status" or "X-Mozilla-Status2" they are ignored.

  5. If line is good (contains a colon or, except for 1st line, starts with "From ") message record is good with offset of this line.

  6. If no previous "bad" line found and this line is "bad" (contains no colon or does not start with "From ") its offset is saved as the potential first good line.

  7. If "bad" line is followed by an "X-Mozilla-Status*" line, the message record is bad.

  8. If "bad" line followed by a good line, message record is good with the bad line's offset.

  9. If "bad" line is followed by another bad line, the message record is bad.

  10. If determination of a good record cannot be made within the lines of the 299 byte buffer, the message record is bad.

  11. If the read() of the 299 buffer fails, the loop is skipped and, of course, the message record is bad.

The main difference from the original code is that

  • First line being "From " is no longer required and is ignored.
  • Order and number of consecutive X-Mozilla-Status* lines doesn't matter and are ignored.
  • Two consecutive bad lines or a bad line followed by X-Mozilla-Status* is considered bad.
  • Bad followed by good line is good with bad line's offset returned.
  • Good line with no previous bad line is good, with the good line's offset returned. (Actually, not a difference..)
  • Increased the buffer size from 199 to 299 bytes to allow longer lines as seen in bug 1681487.
  • All line in buffer potentially checked in loop before declaring message record good or bad.

Note: I have left temporary "printf's" in the code for test tracing. I think maybe adding some MOZ_LOG()'s too might be useful for logging when errors (bad message records) are detected.

Attachment #9217904 - Attachment is obsolete: true
Attachment #9219108 - Flags: feedback?(benc)

I wrote this in comment 37:

However, there is no redundant maildir data or new file written as occurs with mbox where redundant message records are appended on each fetch.

I now see that when a maildir message record is deemed bad and the offline flag is cleared, when fetched again into memory cache there is no immediate write or re-write of a maildir .eml file if tb is not restarted. However, on tb restart and on message access, a new .eml is written. So after 2 tb restarts you could end up with 3 files:

cec331c6-1269-5a52-a618-e2e97b723b69@chartertn.net.eml
cec331c6-1269-5a52-a618-e2e97b723b69@chartertn.net-1.eml
cec331c6-1269-5a52-a618-e2e97b723b69@chartertn.net-2.eml

So with maildir, growing file usage occurs similar to mbox with a "bad" message record.

Comment on attachment 9219108 [details] [diff] [review]
t4.diff

Review of attachment 9219108 [details] [diff] [review]:
-----------------------------------------------------------------

That all looks pretty sensible to me (at least, as sensible as we can hope for with such silly constraints!)

I think a MOZ_LOG upon bad message detection would be a good idea - the main thing is so that there's some steps we can give a user to help track down a borked message. Filename/offset, mailid, whatever.
Attachment #9219108 - Flags: feedback?(benc) → feedback+

(In reply to gene smith from comment #40)

I now see that when a maildir message record is deemed bad and the offline flag is cleared, when fetched again into memory cache there is no immediate write or re-write of a maildir .eml file if tb is not restarted. However, on tb restart and on message access, a new .eml is written. So after 2 tb restarts you could end up with 3 files:

cec331c6-1269-5a52-a618-e2e97b723b69@chartertn.net.eml
cec331c6-1269-5a52-a618-e2e97b723b69@chartertn.net-1.eml
cec331c6-1269-5a52-a618-e2e97b723b69@chartertn.net-2.eml

So with maildir, growing file usage occurs similar to mbox with a "bad" message record.

Thanks for checking that out!
I think that justifies another bug - I've (re)written it up as Bug 1709974.

Can you come to a conclusion here before TB 91 goes to beta on 2021-07-12?

Flags: needinfo?(gds)

Thanks for reminder on this. (Keeping NI set.)

From item 3 in comment 39 above:

If first line starts with "From " it is ignored.

To correctly describe the patch this should say "If first line start with "From " or if the folder is Drafts and the first line starts with "FCC" it is ignored.

However, looking at mbox file for Drafts I never actually see the first line start with "FCC". They always actually start with "From " like any other folder's mbox file. Instead I see, usually after the last X-MOZILLA-STATUS line a header line like this:

FCC: imap://gds@mail.tana.it/INBOX/Sent

However, I see this appear at other locations down in the file for some messages, but again, never as the first line.

So this will just be seen as another valid header line that may mark the starting offset of data read from the file for the message. (What the FCC: header is used for is unclear to me.) Anyhow, I'll keep the code the same and and check for leading "FCC" when folder is Drafts even though it seems to never occur (but maybe in some legacy mbox files?).

I'm looking again at the diff before submitting a formal patch. I'll at least need to take out the printf's.

Flags: needinfo?(gds)

This doesn't effectively change the t4.diff except to remove the printf's and add some MOZ_LOGs. I just used the MsgDB logging facility rather than make a new one. To enable the added logging, include MsgDB:3 in the MOZ_LOG environment var.

Here's an example of how the new log entries appear. In this case there are two consecutive invalid lines:

2021-06-18 05:17:20.546100 UTC - [Parent 197285: Main Thread]: I/MsgDB Invalid header line in offline store: X-Proc-Disposition=blacklist-pass
2021-06-18 05:17:20.548557 UTC - [Parent 197285: Main Thread]: E/MsgDB Leading offline store file content appears invalid, will fetch message from server.
2021-06-18 05:17:20.548564 UTC - [Parent 197285: Main Thread]: E/MsgDB First 300 bytes of offline store content are:
From - Thu Jun 17 23:53:41 2021
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
X-Proc-Disposition=blacklist-pass
X2-Proc-Disposition=blacklist-pass
Return-Path: <ishq2-saki-owner-ishq2=wahiduddin.net@wahiduddin.net>
X-Original-To: ishq2@wahiduddin.net
Delivered-To: ishq2@wahiduddin.net
Rece

The other minor changes I made is the read buffer holds 300 bytes instead of 299 and removed the check for not null mDatabase near the end of the function since a null value would have already caused a crash where it is used earlier in the function.

Attachment #9219108 - Attachment is obsolete: true
Attachment #9227820 - Flags: review?(benc)
Comment on attachment 9227820 [details] [diff] [review]
more-tolerant-offline-msg-record-check.patch

Review of attachment 9227820 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!
Attachment #9227820 - Flags: review?(benc) → review+
Keywords: checkin-needed-tb
Status: NEW → ASSIGNED
status-thunderbird_esr78: --- → wontfix

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a2f157344a91
Increase tolerance of offline store message record validity check. r=benc

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: