1216951 - Signature verification failing for blank lines in chunked base64 attachment

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[giulia]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151014143721

Steps to reproduce:

In order to reproduce the proble, i perform the following step:
1) I send a message, containing a 11MB attachment, to myself using a PEC (italian certiifed electronic system) account
2) As I sent the email to myself, I received a multipart/signed message (the PEC provider generates the SMIME envelope)
3) The signature verification fails

The attached ZIP file contains EML from TB and Webmail and IMAP LOG


Actual results:

Looking inside the received EML (see attached tb_imap_chunks.eml) there are blank lines inside the base64 attachment (try searching the sequence YmIA+ry8AAXWAAABZoAA+oDTAANBwAACMwAAAFUqAP9JJAABDysAAvtBAP4AAAAAnaIACJ26).
These lines corrupt the mail content and signautre verification fails 

If you search the same char sequence in the attached LOG, you will find this warming/error: PARSER: CR/LF fell on chunk boundary.

Thunderbird shows this behaviour with different IMAP servers 


Expected results:

No blank lines inside the downloaded message (seee attached webmail_imap_chunks.eml) and search for the same char sequence

Comment 1

[giulia]

Attachment: tb_imap_chunks.eml

Comment 2

[giulia]

Attachment: webmail_imap_chunks.eml

Comment 3

[giulia]

Attachment: thunderbird.log

Comment 4

[giulia]

The problem seems linked to the imap chunking algoritm.

As workaround, I set the preference mail.imap.min_chunk_size_threshold to the value of 100000000. 

As consequence, the boolean expression in the method nsImapProtocol::FetchTryChunking (nsImapProtocol.cpp:3584) on line 3596 will evaluate to false, avoiding imap chunking

In my experience, the 'extra' blank line appears inside the base64 encoded section of a mail; this implies that 'normal' messages are not affected by the problem as the base64 decoder will ignore the 'extra' newline (i suppose)

Giulia

Comment 5

[giulia]

Some additional information.

I played a little with preferences and by setting:

- mail.imap.chunk_size = 256
- mail.imap.min_chunk_size_threshold = 100

I can reproduce the problem also on 'small' messages

I tested the behaviour on TB38 and TB31 on both windows and ubuntu and I found the problem on each version

Giulia

Comment 6

[giulia]

Hi

Is it possible to try to fix it for tb45 ?

Giulia

Comment 7

[Kent James (:rkent)]

It is unlikely that a core developer would attempt to fix this prior to TB 45. If you can do a patch yourself we would consider it.

Comment 8

[Wayne Mery (:wsmwk)]

I should think we might already have a bug about this, but I'm not tuned in to these sorts of bug reports.
I'm not sure it's a good query, but is anything in http://mzl.la/1LFVDhJ related?

Comment 9

[giulia]

I checked the suggested bug list, but they seem unrelated

Giulia

Comment 10

[Andrea Giudiceandrea]

The bug also occurs in Thunderbird 52.5.2
User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2
Build ID: 20171221110448

and I can confirm that setting mail.imap.min_chunk_size_threshold to the value of 100000000 prevents the bug from occurring.

The bug seems to be related with incorrect handle of CR/LF end of line characters.

These are two lines (74 bytes each, in hex) of the base64 attachment part of a received message (about 859kB) as stored in the server and downloaded from the webmail (original):

[...]
57 71 68 64 2B 62 36 39 35 67 35 34 54 65 6E 45 4E 4E 74 72 73 67 4D 39 2B 69 78 75 70 53 63 78 4F 36 32 57 37 54 58 50 38 74 70 41 37 52 70 2B 38 67 42 65 67 39 66 67 4E 51 44 47 31 47 73 53 42 35 48 58 37 69 77 75 0D 0A
33 72 68 78 59 33 46 70 69 57 4C 52 61 31 4E 54 56 36 39 65 66 52 73 45 69 73 4D 69 36 55 56 6C 39 61 46 53 43 70 70 46 67 72 71 2B 4D 74 47 6D 45 45 63 45 33 43 45 39 52 67 32 54 53 31 4A 65 6C 31 34 32 75 5A 62 6F 0D 0A
[...]

This is the same part of the same message as stored in the INBOX mbox file after retrieved via IMAP by Thunderbird (corrupted) with mail.imap.min_chunk_size_threshold set to the default value of 98304:

[...]
57 71 68 64 2B 62 36 39 35 67 35 34 54 65 6E 45 4E 4E 74 72 73 67 4D 39 2B 69 78 75 70 53 63 78 4F 36 32 57 37 54 58 50 38 74 70 41 37 52 70 2B 38 67 42 65 67 39 66 67 4E 51 44 47 31 47 73 53 42 35 48 58 37 69 77 75 0D 0D 0A
33 72 68 78 59 33 46 70 69 57 4C 52 61 31 4E 54 56 36 39 65 66 52 73 45 69 73 4D 69 36 55 56 6C 39 61 46 53 43 70 70 46 67 72 71 2B 4D 74 47 6D 45 45 63 45 33 43 45 39 52 67 32 54 53 31 4A 65 6C 31 34 32 75 5A 62 6F 0D 0A
[...]

As you can see, in the original message both lines (and all the other line in the whole message) end with 0D 0A (CRLF).
In the corrupted message, instead, the first line (and only this one in the whole message) ends incorrectly with 0D 0D 0A (CRCRLF).

Note that in the original file the character 0x0D of the first ("incrimanted") line is at offset 0x6FFFF and the character 0x0A is at offset 0x70000. Maybe this is related to the bug: in fact, in the Giulia original message attached to the bug report (webmail_imap_chunks.eml) the character 0x0D of the corrupted line is at offset 0x13FFFF and the character 0x0A is at offset 0x140000.

As reported by Giulia, in this scenario, signature verification succeeds in the webmail, fails for the same message retrieved via IMAP by Thunderbird and succeeds for the same message downloaded from the webmail and imported or opened in Thunderbird.

In addition to the above, if you save as an .eml file the message corruptly retrieved via IMAP by Thunderbird, the "incriminated" line in the file ends with 0D 0A 0A (!!!), but if you save the same corruptly retrieved message using "ImportExportTools" add-on, then the "incriminated" line in the file ends with 0D 0D 0A.

I think this bug needs to be fixed.

Comment 11

[Andrea Giudiceandrea]

The bug occurs in Thunderbird 52.5.2 with default options and probably any IMAP server:
mail.server.default.fetch_by_chunks = true
mail.imap.chunk_size = 65536 (with this value the bug occurs if CR/LF are at any of 0x0FFFF/0x10000 offsets of the original retrieved message from IMAP server)
mail.imap.min_chunk_size_threshold = 98304

The chances of running into the bug increase as mail.imap.chunk_size and mail.imap.min_chunk_size_threshold values decrease.

In this scenario, when the bug occurs the log reports the error: "PARSER: CR/LF fell on chunk boundary." and at that point the line retrieved will end wrongly with CR CR LF (0D 0D 0A) instead of the right CR LF (0D 0A). Obviously in this case the signature verification of a multipart/signed SMIME message will fail.

It seems this bug lies in how nsImapServerResponseParser::msg_fetch_literal (not properly) handles CRLF end of lines that are split across two chunks.

In particular in this lines at https://dxr.mozilla.org/comm-central/rev/73858a61806859269666a58fd830d145e1522425/mailnews/imap/src/nsImapServerResponseParser.cpp#3099-3111

if (ContinueParse())
    {
      // When we split CRLF across two chunks, AdvanceToNextLine() turns the LF at the
      // beginning of the next chunk into an empty line ending with CRLF, so discard
      // that leading CR
      bool specialLineEnding = false;
      if (lastCRLFwasCRCRLF && (*fCurrentLine == '\r'))
      {
        char *usableCurrentLine = PL_strdup(fCurrentLine + 1);
        PR_Free(fCurrentLine);
        fCurrentLine = usableCurrentLine;
        specialLineEnding = true;
      }

This piece of code was written by Irving Reid in 2012 https://hg.mozilla.org/releases/comm-esr52/rev/5a6bee217340

I'm not expert enough to improve the code in order to fix the bug, but if some developers want deal with the bug fixing, I could give them some help.

Comment 12

[Wayne Mery (:wsmwk)]

see comment 11 and http://mzl.la/1LFVDhJ (which I'm not sure is a good enough query to find any related open bugs)

Is there an obvious error?

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Wayne Mery (:wsmwk) from comment #12)
> Is there an obvious error?
Not that I can see. Someone needs to sit down, reproduce it and debug it. If we only had more staff ...

Comment 14

[Andrea Giudiceandrea]

(In reply to Wayne Mery (:wsmwk) from comment #12)
> http://mzl.la/1LFVDhJ (which I'm not sure is a good
> enough query to find any related open bugs)
> 
It seems Bug 576235 is related.

Comment 15

[gene smith]

I appended both of the attached messages into a tb mailbox (did not use smtp to send them) and they both appear the same in tb. I see 3 attachments listed, 2 of them are visible inline (postacert and an abstract painting). Attachment daticert.xml is not visible.
However, for both .eml that I "dragged/dropped" in, both show an envelope symbol with a red X to the left of the date/time. Is that the signature verification failure you speak of? I see it for both files, webmail and the one you saved from tb.

Probably a dumb question, but what do you mean exactly by "fails signature verification"? I assume tb is failing the verification or is it the "PEC" failing the verification? Do I need to actually send the email to myself via smtp to see the problem you see?

I do see the blank line in the tb file. Haven't yet looked at the hex versions you show above.

Comment 16

[gene smith]

I clicked the red X and see the "Digital Signature Is Not Valid" dialog. So that must be what you are seeing. Should I see it for both eml files that I imported?

Comment 17

[gene smith]

I removed the extra eol chars from the tb INBOX mbox file. There were exactly 7 lines that needed fixing. I still see the red X certificate error button. When I click it, all I see is that the certificate issuer is unknown, nothing about bad data.

The extra \r and/or \n don't seem to affect the graphical attachment display. Are we really sure that the cert failure is due to this? Right now my INBOX file contents on tb are identical to the file webmail_imap_chunks.eml and I still see the red X. The only difference between them is INBOX has a couple of X-Mozilla* header items at the top added by tb. Otherwise, all lines and line endings are the same (all lines end in 0d0a, aka, crlf or \r\n).

Or maybe I am totally missing something here. Do I need to import a certificate of some sort (CA file?) to even test this?

Comment 18

[Jorg K (CEST = GMT+2)]

Let me summarise what I understand here:
Due to some internal faulty "chunking" technique, spurious newlines get injected into base64 attachments. That clearly visible comparing the attached webmail_imap_chunks.eml with tb_imap_chunks.eml

Those extra lines sit between lines 17757 and 214371. The MIME part headers are at:
Line 19: Content-Type: multipart/signed; protocol="application/x-pkcs7-signature"; micalg=sha1; boundary="----0DE9B9C89D0FE6C8F56A79514745A670"
Line 25: Content-Type: multipart/mixed; boundary="----------=_1445424326-11845-16"
Line 30: Content-Type: text/plain; charset="iso-8859-1"
Line 48: Content-Type: application/xml; name="daticert.xml"
Line 71: Content-Type: message/rfc822; name="postacert.eml"
Line 87: Content-Type: multipart/mixed;
Line 94: Content-Type: text/plain; charset=iso-8859-15; format=flowed
Line 100: Content-Type: image/png;
Line 225856: Content-Type: application/x-pkcs7-signature; name="smime.p7s"

So absolutely *all* unwanted newlines fall into the rather large attached image. So I'd say that Gene's observation is correct, fixing the those lines doesn't change a thing about the validity of the certificate, it's show as invalid since the issuing authority can't be trusted. You'd have to import some root certificates to fix that. But that's not the point here.

So we don't want to deny that the chunking algorithm is faulty, but I really wonder what the effect of the extra newline in the base64 part is, if any. To test this further I manually injected more newlines into the last MIME part after line 225856. I see no difference in behaviour, so at a guess, when decoding base64, empty lines are just ignored. I'd have to dig through the code to confirm this.

I think Magnus won't have any magic insight here, so clearing the NI for him as well.

Comment 19

[gene smith]

Ignoring the cert problem for now, I only see the chunking problem (extra eol's) if I import (drop) in the "bad" attachment file above that already has the extra eol's. If I drop in the "good" attachment file above (webmail_imap_chunks.eml) I don't see the problem. I am testing by repairing the folder that causes a re-fetch of everything from the server. I'm not 100% sure that is the same as what would happen when the emails are just received normally.

What I see on repair seems a bit strange. First the whole file is fetched in separate chunks. Then as soon as that finishes the file is fetched again as a full body fetch, that means there is one request from tb and then the server responds with a long tcp stream of responses. 

Since the full body fetch occurs last, it may overwrite any chunking problems (extra eol's) that would be seen if the file was only fetched using chunking.

I see the same thing (chunking followed by full body fetch) on my ISP's server (openwave) and on a local server (dovecot). I can't see what is actually stored internally on openwave but I can on the local dovecot. It is exactly the same as webmail_imap_chunks.eml with no extra eol's.

I also accessed the webmail_imap_chunks email stored on openwave imap server from two other systems that just fetched it normally without doing a repair. On one system (maildir based) the received file has no extra eol's. Same for the other systems that is mbox based. All of my systems running tb are configured with default chunking parameters.

Comment 20

[giulia]

Hi 

Repairing the folder sometimes work. As I wrote, unfortunately, the problem is not deterministic

My suggestion is to use the preferences values I reported in Comment 5, in order to check the problem also on 'small' messages

If can help and you provide me an email address, I can send some test messages to you in order to examine a real message exchange

Let me know

Giulia

Comment 21

[gene smith]

(In reply to giulia from comment #20)
> Hi 
> 
> Repairing the folder sometimes work. As I wrote, unfortunately, the problem
> is not deterministic
> 
> My suggestion is to use the preferences values I reported in Comment 5, in
> order to check the problem also on 'small' messages
> 
> If can help and you provide me an email address, I can send some test
> messages to you in order to examine a real message exchange
> 
> Let me know
> 
> Giulia

Hi Giulia,
Yes, I tried the parameters you reported in Comment 5 and didn't see a problem. But I will try again. If you can send a test email that has the problem to me at [e-mail] that would be great!

Comment 22

[gene smith]

I am seeing something really bad. It looks like an infinite sequence of fetches:

for n=0 ; n < infinity; n++
{
tb: UID fetch 1002 (BODY.PEEK[1.3.2]<n*65536.65536>)
server: OK UID FETCH complete
}

This kept happening with zero delay until I shutdown tb. All I did before this was set a folder to not be sync'd (online only) and then repaired the folder. This deleted the mbox file as might be expected. Not sure what's causing this network flood. Sounds similar to what was reported in bug 1432879

Comment 23

[giulia]

Hi Eugene

I will send you some test messages from [e-mail]; the subject of emails will start with the word ANOMALIA and emails will contain signatures

I will send 'small' messages.

Let me know

Thanks

Giulia

Comment 24

[Andrea Giudiceandrea]

Hi Eugene, Giulia and Jorg,
thanks for your effort to test the bug.

Eugene, I'm sending you ([e-mail]) a email message with:
- a S/MIME signed message retrieved by TB via IMAP, as it's stored in the inbox mbox file, with CR/LFs at chunk boundaries (with mail.imap.chunk_size set to 2048 to increase the chance to catch the bug) that are incorrectly stored as CR/CR/LF: the signature verification fails in TB;
- the same message retrieved by TB and then saved on disk ("Save as"->"File"): CR/LFs at chunk boundaries are saved as CR/LF/LF;
- the same message downloaded from the WebMail: the signature verification succeeds in WebMail and also if imported in TB;
- the TB log.

Hope this will help you.

Andrea

Comment 25

[Andrea Giudiceandrea]

Regarding the concerns expressed by Jorg K (GMT+1) and 	gene smith:

(1) the s/mime signature verification fails on both the initially (2 years ago) attached messages, instead of only one, because now the time validity of the signing certificate is expired (2th Feb 2017)

(2) the s/mime signing process involves the whole body part of the message with the original attachments already base64 encoded; than the process append another MIME part, an application/x-pkcs7-signature; when the message is retrieved, the signature verification process takes place before the base64 decoding process, so if any byte of the whole body part is different from the original (e.g. changing some CR/LF in CR/CR/LF) the verification fails.

(3) actually it seems that "when decoding base64, empty lines are just ignored", so you don't notice the problem if the message is not signed; but if the message is signed with S/MIME protocol, every modified or added byte in the body part of the message makes the signature verification fails

Comment 26

[gene smith]

Yes, I saw the Feb. date and thought its not Feb 2017 yet, didn't notice the year. Guess I think it is still 2017. Duh :).
Got the 3 emails from Guilia. With default setting they all come into tb with no signing errors. Will play with the parameters and see what happens.

I don't see the word ANOMALIA in the subjects. Also, the first two mails have 3 attachments. The last one has only two (no mail-toolbar.png file). I that what was sent?

Comment 27

[Jorg K (CEST = GMT+2)]

As Andrea said in comment #25 (2), changing the length of body or MIME parts affects the signature verification. So most likely we need to get some certificates ourselves, like at Comodo, and come up with our own test cases.

Comment 28

[giulia]

Eugene, you are right

The subject has to start with the words "POSTA CERTIFICATA" and the first two messages have the PNG attachment

The other two attachments are related to PEC (the italian certified mail system):
1) the postacert.eml contains the real message I sent to you (the PNG image is attached to this email)
2) the daticert.xml contains a trasaction receipt
The encapsulating message (containing the above listed attachments) is signed 

Let me know if you need more messages with different sizes.

A brief note: if I understood the code, chunking is not used on GMail

Thanks for your effort

Giulia

Comment 29

[gene smith]

I have verified that adding the extra newline causes the signature verification to fail but the inline png attachments are still perfectly visible. So I have been working with the code that parses the "chunked" responses that is messing up.

Also, it is not clear to me when chunking is used and when the full emails fetch is used and the results saved to the mbox. So far I have only seen chunking exclusively when I send any large email to myself and then click on Inbox, both on the same system, which is how the reporters describe the problem. If, on system B I send the large email to myself, and then on system A receive the new mail, all that occurs is the full body fetch (essentially one giant chunk). See note below for more chunking observations.

Of course, the problem becomes more likely if you have many chunks and/or smaller chunks, as reporters pointed out. Actually, each chunk consists of multiple lines of about 70 chars. The problem occurs when the last line of a chunk ends in \r instead of the correct \r\n, due to natural splitting. This causes the 1st line of the next chunk to be just be string "\n". The bug in the code causes the "bad" lines to end with \r\r\n. So the extra \r causes the signature checksum error (but really doesn't affect anything else).

The problem with the code is it does nothing with the "\n" string that is the first line in the next chunk. The code is actually expecting the line to be "\r" instead of "\n". See nsImapServerResponseParser.cpp about line 3109:


      if (lastCRLFwasCRCRLF && (*fCurrentLine == '\r'))   //<<--should be looking for '\n'
      {
        char *usableCurrentLine = PL_strdup(fCurrentLine + 1);
        PR_Free(fCurrentLine);
        fCurrentLine = usableCurrentLine;
        specialLineEnding = true;
      }

But, even with this fixed, when the "\n" line is detected, chunking would stop and then a full body fetch occurred. The resulting INBOX file was uncorrupted with the extra \r but I am not sure that was the intention of the code. I would think that if the system decides chunking is appropriate then it should continue on fetching the email in chunking mode and ignore the periodically occurring "\n" lines.

I will attach a work-in-progress patch/diff that shows what I have done, all in function msg_fetch_literal() of nsImapServerResponseParser.cpp. It correctly detects the problem condition (now called chunkStartsWithNewline instead of lastCRLFwasCRCRLF) and acts on it by ignoring the short "\n" line. 

Note: The last few times I tried sending the big email to myself, for some reason chunking was not used. Previously (last two days), when sent it always failed "copy to Sent folder" (no idea why at this point) but it actually did save it there and I always cancelled the error dialog. Now it's not failing and I have to cancel the save to Sent to trigger chunking. If I don't cancel it, I see full body save (1 big chunk)! I have no idea what is going on with this. So a major question is when does chunking actually happen and what does it really buy you? (Also, why was my save to sent failing now it's not?)

Comment 30

[gene smith]

Attachment: chunk.diff

Work in progress.

Comment 31

[gene smith]

I have turned off the "save to Sent folder" option and now I never see "chunking" when a new email is received. I am sending myself the "POSTA CERTIFICATA: test large attachment" email (about 15.7MB). It comes in as one big fetch response, not in small pieces.

However, only when I set config editor item "mail.server.default.autosync_offline_store to false (default is true) and send myself the same email, is it fetched in 3503 byte chunks. (I have set my chunk size to 3503). The received email is still cleanly stored in the INBOX mbox file (no extra EOL chars). This is with my patch above in place that correctly fixes the chunk boundary problem.

I see in the log_tb_2048.log file that the reporters sent that 2048 bytes chunks are used and 9 times the "chunk boundary" error is printed. Was this file obtained while the email was being fetch as new mail from the server into INBOX mbox file? I assume it is. Also, are there any non-default config editor setting such as the autosync_offline_store mentioned above?

Reporters, do you know if you ever see emails fetched with full-body fetch (one fetch request, many responses) rather than in chunks when an emails is received?

Comment 32

[gene smith]

I see another probably unrelated problem consistently. If I configure a folder to not have an offline store and it contains the large email, when I access the email, it is fetched and partially shows up, but the large png inline attachment shows as the raw base64 encoded text data just like it does in the mbox file. At the bottom, only 2 instead of 3 attachments are available. It doesn't seem to matter whether it is fetched in chunks or not. This should be noted in a new bug.

Comment 33

[giulia]

I think that the decision to use/notuse the chunking algorithm is in the method FetchTryChunking in nsImapProtocol.cpp (line 3678) (https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp?q=FetchTryChunking&redirect_type=direct#3678).

For example, if the IMAP server is not IMAP4Rev1, chunking is not applied. 

Giulia

Comment 34

[Andrea Giudiceandrea]

(In reply to gene smith from comment #31)

> Was this file obtained while the email was being fetch as new mail from the server
> into INBOX mbox file?

Yes.

> Also, are there any non-default config
> editor setting such as the autosync_offline_store mentioned above?

mail.server.default.autosync_offline_stores = true (default)
*chunk* settings are default (except mail.imap.chunk_size for testing purpose)

Gene, have you seen that the changeset https://hg.mozilla.org/releases/comm-esr52/rev/5a6bee217340 that introduced the code in nsImapServerResponseParser::msg_fetch_literal (nsImapServerResponseParser.cpp) also added a test (mailnews/imap/test/unit/test_chunkLastLF.js - mailnews/test/data/bug92111) that allegedly will test the "handling of CR/LF split across a chunk boundary"?

Comment 35

[gene smith]

(In reply to giulia from comment #33)
> I think that the decision to use/notuse the chunking algorithm is in the
> method FetchTryChunking in nsImapProtocol.cpp (line 3678)
> (https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapProtocol.cpp?q=FetchTryChunking&redirect_type=direct#3678).
> 
> For example, if the IMAP server is not IMAP4Rev1, chunking is not applied. 
> 
> Giulia

Yes, that does appear to be where it is decided to chunk or not. However, I rarely see this actually called. Typically the imap method selected is nsImapMsgDownloadForOffline when a new email is received and it just does a full body fetch and never even calls FetchTryChunking(). I only see it use chunking consistently when new mail received if I set autosync_offline_store to false. That's why I was wondering if maybe you have it set to the non-default false.

Comment 36

[gene smith]

Attachment: chunk2.diff

Discovered that chunk.diff did not actually fix it. When it "fixed" the missing \n all it did was save the last line of chunk X terminated with just a \r. The \n was still missing. The problem was that I needed to add in the missing \n to last line of chunk X. The handling of the 1st two lines of chunk X+1 seem OK.
I also maybe improved the code readability some and added some more comments to explain better what is done.
Again, this is still WIP and not quite ready to submit an official patch.

Comment 37

[Jorg K (CEST = GMT+2)]

Sorry I haven't gotten to this yet, I'm really busy with keeping up with bustage.

Comment 38

[gene smith]

(In reply to Andrea Giudiceandrea from comment #34)
> 
> Gene, have you seen that the changeset
> https://hg.mozilla.org/releases/comm-esr52/rev/5a6bee217340 that introduced
> the code in nsImapServerResponseParser::msg_fetch_literal
> (nsImapServerResponseParser.cpp) also added a test
> (mailnews/imap/test/unit/test_chunkLastLF.js - mailnews/test/data/bug92111)
> that allegedly will test the "handling of CR/LF split across a chunk
> boundary"?

Yes, I saw the bug report and patch that addresses this and other issues. I need to go back and read it again and maybe I will learn something. 
I hadn't seen the test you point out. It mentions some other rare conditions that I don't know that msg_fetch_literal() handles correctly. I will look closer. Thanks for pointing this out!

Comment 39

[gene smith]

Went back and re-read bug 92111. Didn't help a lot. Based on a comment entered after the fix for that bug was in effect, it didn't completely fix the problem (missing data from messages fetched from Exchange imap servers due to wrong messages size reported). The chunking changes in msg_fetch_literal() were just a small part of the fix and weren't really discussed or explained.

Comment 40

[giulia]

(In reply to gene smith from comment #35)

Hi Eugene

I didn't change the autosync_offline_store preference

Giulia

Comment 41

[gene smith]

Looking at this some more, I see that the reporter's test emails cause a lot of problems for tb, especially if offline stores are disabled. I think it is due to the nested nature of the attachments, but not sure. 

I was trying to determine why the reporters always see their emails fetched using chunks. Like I have mentioned before, I have to try really hard to see new emails (or any emails) fetched in chunks.

I see fetching in chunks if I:

1. Set autosync_offline_store to false. This causes storage to the mbox file only when a new email is accessed (clicked on). The new emails are fetched in chunks. The reporter's test emails show OK as long as storage to the mbox file (offline store) is enabled

2. If I return autosync_offline_store to true (default) and turn off the offline storage for Inbox and repair the Inbox folder (causing the INBOX mbox file to be deleted) and then restart tb, I then see each email (old or new) accessed in Inbox fetched in chunks. However, the reporter's test emails do not display correctly at all in this mode. Other emails with non-nested attachments display OK.

3. I also saw fetching in chunks if the "save to sent folder" is enabled and it fails or if I cancel the save to sent while it is going on or right before it finishes. For reasons I have been unable to determine, this causes the newly received test email provided by the reporters to be fetched in chunks when accessed in Inbox when a copy of it is in another folder and then "edited as new" and sent to myself.

Otherwise, with default configuration, I don't see fetching in chunks (many imap fetches and chunk sized responses). When I access it, the reporter's full email is obtained with one imap fetch request and many imap responses (one *big* chunk). I can't figure out why the reporters are always seeing chunking used when they access emails. I don't think it is caused by a server capability that is unique to their imap server which is Dovecot.

From what I can see and have read about several reported problems in the past, the "chunking" feature causes more problems than it solves (and I have not really found what it solves!?). I would recommend just turning it off with the mail.server.default.fetch_by_chunks option set to false and restarting tb. This should be an easy work-around for the primary problem reported in this bug.

Note: If you want to be account specific you may be able to leave mail.server.default.fetch_by_chunks at default value true and create a new pref, mail.server.serverX.fetch_by_chunks, set to false for only a specific account/server. You have to search the config editor to determine the X value (1, 2, 3...) for the target account. I haven't tried this.

Comment 42

[gene smith]

(In reply to giulia from comment #28)
>
> A brief note: if I understood the code, chunking is not used on GMail
> 

I haven't seen anything in tb code (yet) that disables chunking when the server is gmail. From what I have seen, to chunk or not to chunk isn't based on the server type or capabilities, but I could be wrong. Where in the code are you seeing this?

I have seen recommendations to disable chunking, like I have suggested above, when the server is exchange or gmail (google: "thunderbird fetch by chunks").

Comment 43

[Andrea Giudiceandrea]

(In reply to gene smith from comment #31)
> Reporters, do you know if you ever see emails fetched with full-body fetch
> (one fetch request, many responses) rather than in chunks when an emails is
> received?

I've made further tests sending the same message (a normal mime multipart, not S/MIME, with size > mail.imap.min_chunk_size_threshold) to accounts at various hosts, then the messages received were retrieved via the respective IMAP servers in order to see which one allows TB to fetch the message in chunks.

My tests show that:

imap.googlemail.com and imap.mail.yahoo.com seem to never allow to fetch in chunks

imap.tiscali.it, imaps.pec.aruba.it and relay.poste.it seem to always allow to fetch in chunks

imapmail.libero.it sometimes it allows to fetch in chunks, at others it doesn't


Anyway, every time the message is not fetched in chunks, the log reports, just before the message fetch:
***********
S-INBOX:ProcessCurrentURL: entering
S-INBOX:ProcessCurrentURL:imap://[IMAPURL]:  = currentUrl

FetchMessage peek: curFetchSize 0 numBytes 0
S-INBOX:SendData: 14 UID fetch 547 (UID RFC822.SIZE BODY.PEEK[])
ReadNextLine [stream=f90ab20 nb=57 needmore=0]
S-INBOX:CreateNewLineFromSocket: * 401 FETCH (UID 547 RFC822.SIZE 675420 BODY[] {675420}
S-INBOX:STREAM:OPEN Size: 675420: Begin Message Download Stream
***********


and every time the message is fetched in chunks:
***********
S-INBOX:ProcessCurrentURL: entering
S-INBOX:ProcessCurrentURL:imap://[[IMAPURL]]:  = currentUrl

SHELL: URL imap://[[IMAPURL]], OKToFetchByParts 0, allowedToBreakApart 1, ShouldFetchAllParts 0
FetchTryChunking: curFetchSize 674436

FetchMessage peek: curFetchSize 2047 numBytes 2047
S-INBOX:SendData: 10 UID fetch 30777 (UID RFC822.SIZE BODY.PEEK[]<0.2047>)
ReadNextLine [stream=c549600 nb=62 needmore=0]
S-INBOX:CreateNewLineFromSocket: * 11558 FETCH (UID 30777 RFC822.SIZE 674436 BODY[]<0> {2047}
S-INBOX:STREAM:OPEN Size: 674436: Begin Message Download Stream
***********


"SHELL: URL ..." is written in the log by nsImapProtocol::ProcessSelectedStateURL at nsImapProtocol.cpp#2719 and "FetchTryChunking: ..." by nsImapProtocol::FetchTryChunking at nsImapProtocol.cpp#3686 that also is called by nsImapProtocol::ProcessSelectedStateURL at nsImapProtocol.cpp#2771


Needles to say that when the message is fetched in chunks from any imap server, it contains an extra CR for every CR/LF that falls at mail.imap.chunk_size boundary.


Capability strings seen in the log for the above mentioned imap servers:

***********
imap.googlemail.com
CAPABILITY IMAP4rev1 UNSELECT IDLE NAMESPACE QUOTA ID XLIST CHILDREN X-GM-EXT-1 XYZZY SASL-IR AUTH=XOAUTH2 AUTH=PLAIN AUTH=PLAIN-CLIENTTOKEN AUTH=OAUTHBEARER AUTH=XOAUTH
CAPABILITY IMAP4rev1 UNSELECT IDLE NAMESPACE QUOTA ID XLIST CHILDREN X-GM-EXT-1 UIDPLUS COMPRESS=DEFLATE ENABLE MOVE CONDSTORE ESEARCH UTF8=ACCEPT LIST-EXTENDED LIST-STATUS LITERAL- SPECIAL-USE APPENDLIMIT=35651584

imap.mail.yahoo.com
[CAPABILITY IMAP4rev1 ID MOVE NAMESPACE XYMHIGHESTMODSEQ UIDPLUS LITERAL+ CHILDREN SASL-IR AUTH=PLAIN AUTH=XYMCOOKIEB64 AUTH=XOAUTH2 AUTH=OAUTHBEARER] IMAP4rev1 Hello

imap.tiscali.it
[CAPABILITY IMAP4rev1 UIDPLUS NAMESPACE QUOTA CHILDREN SORT AUTH=CRAM-MD5 AUTH=DIGEST-MD5 AUTH=PLAIN] Dovecot ready.
[CAPABILITY IMAP4rev1 NAMESPACE UIDPLUS QUOTA CHILDREN SORT ID] Logged in

imaps.pec.aruba.it
[CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE AUTH=PLAIN AUTH=LOGIN] IMAP ready.
[CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE SORT SORT=DISPLAY THREAD=REFERENCES THREAD=REFS THREAD=ORDEREDSUBJECT MULTIAPPEND URL-PARTIAL CATENATE UNSELECT CHILDREN NAMESPACE UIDPLUS LIST-EXTENDED I18NLEVEL=1 CONDSTORE QRESYNC ESEARCH ESORT SEARCHRES WITHIN CONTEXT=SEARCH LIST-STATUS BINARY MOVE SEARCH=FUZZY NOTIFY SPECIAL-USE QUOTA ACL RIGHTS=texk] Logged in

relay.poste.it
CAPABILITY IMAP4rev1 UIDPLUS IDLE LOGIN-REFERRALS NAMESPACE QUOTA CHILDREN SORT THREAD=ORDEREDSUBJECT SASL-IR

imapmail.libero.it
[CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE AUTH=PLAIN AUTH=LOGIN AUTH=CRAM-MD5] Dovecot ready.
[CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE SORT SORT=DISPLAY THREAD=REFERENCES THREAD=REFS THREAD=ORDEREDSUBJECT MULTIAPPEND URL-PARTIAL CATENATE UNSELECT CHILDREN NAMESPACE UIDPLUS LIST-EXTENDED I18NLEVEL=1 CONDSTORE QRESYNC ESEARCH ESORT SEARCHRES WITHIN CONTEXT=SEARCH LIST-STATUS BINARY MOVE SEARCH=X-MIMEPART XDOVECOT NOTIFY METADATA QUOTA] Logged in
***********

Comment 44

[gene smith]

Thanks for the information on the various servers. I don't seem to see a certain capability that really controls chunking.

I may have been too certain in my observations. I do see chunking when new email arrives on at least one dovecot based account. However, I only see it when I turn off the IDLE feature. This is called "allow immediate server notification when a new message arrives" on the server configuration screen. When this is on (default) I see the entire message fetched in one transaction (1 request, lot of responses). If I turn it off, the entire message is fetched in many chunks (many requests and responses).

On the openwave (charter.net) server I don't normally see chunking regardless of IDLE. However, on all servers I do observer this: If the mailbox where new mail has been placed is configured to not have an offline store (e.g., there is only an INBOX.msf and no INBOX file), when new mail arrives, only when clicked on it is the email completely fetched. In this case it is fetched in parts and the parts (if large enough) are fetched in chunks. The chunk requests look like this and originate from the FetchTryChunking() call in nsIMAPBodyShell.cpp:

508 UID fetch 2581 (BODY.PEEK[1.3.2]<15794176.65536>)

It appears that all the servers you tested above support IDLE except for yahoo and tiscali. Also, some of them are probably dovecot. Do you have the IDLE setting on or off? (IDLE probably not relevant, but another data point.)

Also, I may have spoken too soon about problems displaying the big message with no offline store. I don't see a problem on dovecot servers. I see problems on openwave, yahoo and gmail. Openwave just displays text instead of the png (that I mentioned before). On yahoo and gmail, the chunks come in painfully slow and seem to eventually stop. But if I turn off chunking globally, even with no offline store, everything displays OK and relatively fast. (This is with that big test email.)

Unless I am missing something, chunking maybe only helps when you don't have offline store and maybe if you have a very slow network (but not even sure about this). In this mode it allows you to access the mime parts separately and selectively. If you have a very slow network, you probably want to have offline store so you can access existing emails faster. Also, if you do have offline store, chunking actually just fetches the whole message anyhow and not the separate parts. And since you need to save the whole message to store for offline in mbox file, you may as well not chunk and just get it all at once (a lot faster and easier on the server).

Therefore I would suggest defaulting chunking to OFF and maybe provide a new advanced server setting to switch it on for the rare cases where it might help (slow network with no offline store?). Of course, the EOL bug needs to be fixed either way.

Comment 45

[Andrea Giudiceandrea]

(In reply to gene smith from comment #44)
> It appears that all the servers you tested above support IDLE except for
> yahoo and tiscali. Also, some of them are probably dovecot. Do you have the
> IDLE setting on or off? (IDLE probably not relevant, but another data point.)

The "allow immediate server notification" option is on for the accounts tested.


> Therefore I would suggest defaulting chunking to OFF and maybe provide a new
> advanced server setting to switch it on for the rare cases where it might
> help (slow network with no offline store?). Of course, the EOL bug needs to
> be fixed either way.

I agree with you.

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 8947635 [details] [diff] [review]
chunk2.diff

I've finally gotten around to looking at this.
First question: Does it work? That would be a bonus ;-)

To give you qualified feedback, I first need to get qualified. So please explain all those variables here:
+        char *displayEndOfLine = (fCurrentLine + strlen(fCurrentLine) -
+                                  (charsReadSoFar - numberOfCharsInThisChunk + 1));
+        // Save these so original unmodified fCurrentLine is restored below.
+        char saveit1 = displayEndOfLine[1];
+        char saveit2 = displayEndOfLine[2];
What are fCurrentLine, charsReadSoFar and numberOfCharsInThisChunk?
What does displayEndOfLine address? The previous code only accessed displayEndOfLine[0] (char saveit = *displayEndOfLine;), but you read two characters behind that.

Nuking the leading LF with a null isn't my favourite. Can we start at fCurrentLine+1.

Note that f? is usually about someone else trying it, but I'm in no position to try this myself. Or am I missing something?

Comment 47

[gene smith]

(In reply to Jorg K (GMT+1) from comment #46)
> Comment on attachment 8947635 [details] [diff] [review]
> chunk2.diff
> 
> I've finally gotten around to looking at this.
> First question: Does it work? That would be a bonus ;-)

It worked the last time I tested it, several weeks ago.
Since I posted the patch I did improve, in my opinion, the comments that show examples lines of a chunk.

> 
> To give you qualified feedback, I first need to get qualified. So please
> explain all those variables here:

> +        char *displayEndOfLine = (fCurrentLine + strlen(fCurrentLine) -
> +                                  (charsReadSoFar -
> numberOfCharsInThisChunk + 1));

fCurrentLine is the string pointer to the current chunk line. When we are at the end of a chunk, it can point to two concatentated lines.

Examples: 
a) when not at end of chunk, fCurrent line will point to a line like this:
"APwAAAAAmZkA/wAAAAAREQD/AAAAAquVAAbk8QAHCBAAAPD0AAP5+wABRCoA+0BgAP0AAAAA\r\n"
and when at the end of a chunk, fCurrent line will point to line(s) like this,
b) when last line of chunk is short enough to fit without continuation to next line:
"1s8AA5i4AAvF4QAG6+sAAD0bAPsAAAAA1OAAC)\r\n"
c) when last line of chunk requires some continuation on an extra line:
"FxcA/wAAAALN2gADu80ACS0nAPpVVAD1wNAABF5YAPhAJgD31+QABAAAAP8oMQD+HBwA/umj\r\n"
")\r\n"
d) when last line of chunk excludes the trailing '\n' due to the \r\n being split between chunks (the problem case!):
"/gAOC/wA/QAAAAAAAAAA8wACCvz+AgIEAAD8/P4ABQUAAPoAAAD+AAEA/voHAAQGBQD/BAQA\r"
")\r\n"

displayEndOfLine point to the last "literal" in the chunk. I define 'literal' as the email data and its EOLs and not imap protocol elements and their EOLs. 
Specifically, for examples above, displayEndOfLine,
a) points to '\n'
b) points to 'C'
c) points to '\n' that follows "j\r"
d) points to '\r' that follows the A

Only when displayEndOfLine[0] == '\r' is nextChunkStartsWithNewline set to true that indicates the chunks are split between the \r and \n.

> +        // Save these so original unmodified fCurrentLine is restored below.
> +        char saveit1 = displayEndOfLine[1];
> +        char saveit2 = displayEndOfLine[2];
> What are fCurrentLine, charsReadSoFar and numberOfCharsInThisChunk?
> What does displayEndOfLine address? The previous code only accessed
> displayEndOfLine[0] (char saveit = *displayEndOfLine;), but you read two
> characters behind that.

The "saveit's" are only used for the last line of a chunk so the line can be restored to its original state. For the last line of the chunk you always have to overwrite a character to null terminate the line. And when nextChunkStartsWithNewline is true (\n  should be in next chunk) you have to add back in the missing \n, which overwrites another character, so you have to save two items in that case.

> 
> Nuking the leading LF with a null isn't my favourite. Can we start at
> fCurrentLine+1.

I'm a bit fuzzy on why I had to do this before the string (now a null string after the \n is changed to 0) is passed into HandleMessageDownLoadLine(). I tried to just skip the call to HMDLL() but something didn't work, so I "nuked" the \n (changed it to \0 and called HMDLL() with the null string and it worked. I will check this again.

Here's how the leading lines of the next chunk look when when nextChunkStartsWithNewline is true:
"\n"
"APwAAAAAmZkA/wAAAAAREQD/AAAAAquVAAbk8QAHCBAAAPD0AAP5+wABRCoA+0BgAP0AAAAA\r\n"

You are probably right. If I just pass in fCurrentLine+1 it may still be a null string and I won't be changing anything. Probably doesn't point to the lead char, e.g., 'A', of the next line. 

If I do keep the change of the \n to 0, I may need to restore it to \n after calling HMDLL() which is not in the patch. Although not restoring it doesn't seem to cause a problem.  Will check all this closer.









> 
> Note that f? is usually about someone else trying it, but I'm in no position
> to try this myself. Or am I missing something?

Comment 48

[gene smith]

> 
> Note that f? is usually about someone else trying it, but I'm in no position
> to try this myself. Or am I missing something?

Didn't know that. I though f? just meant informal feedback. So I should do r? for this type of thing that is not yet an official patch? (Sorry for the big whitespace above!)

Comment 49

[Jorg K (CEST = GMT+2)]

Comment on attachment 8947635 [details] [diff] [review]
chunk2.diff

Thanks Gene, this is a little clearer now, but you opened the door to more questions ;-(

What's a "chunk"? And how does this relate to the fCurrent line buffer? And the ")" is there to end the data? Why do we get into state d) at all? Why isn't the A before the \r put into the next chunk so the \n would fit?

Sorry about the ignorance, as I said, I need to get qualified first. Sadly the original authors have all left, so we need to build up the know-how and document everything.

As for the f?. As I said, *usually* it's about trying something, but you're quite right, you can also request informal feedback for a certain approach.

So I think we're on the right track here. It would be good to document all the processing since this is difficult stuff. I ventured into it a tiny little bit myself here:
https://hg.mozilla.org/comm-central/rev/b143aa60d983#l1.12

Comment 50

[gene smith]

(In reply to Jorg K (GMT+1) from comment #49)
> Comment on attachment 8947635 [details] [diff] [review]
> chunk2.diff
> 
> Thanks Gene, this is a little clearer now, but you opened the door to more
> questions ;-(
> 
> What's a "chunk"? And how does this relate to the fCurrent line buffer? And
> the ")" is there to end the data? 

TB decides sometimes to fetch the whole message in chunks (configured as 64k by default) rather than just getting the whole message. How it decides if it is necessary I am not yet 100% sure, but it only happens when the function FetchTryChunking() is called. If the message size is greater than the "chunk threshold" (configured to about 98k by default), the message is fetched in chunks; otherwise the whole message is fetched. But often FetchTryChunking() is not called at all so then the whole message is then fetched. 

Each chunk response comes in as a series of about 70 char "lines" of "literal" data that are each terminated with \r\n. The chunk is enclosed in non-literal parenthesis, (chunk-data-in-many-lines)\r\n. Each literal line of the chunked response is processed in the main loop of msg_fetch_literal() with fCurrentLine pointing to first literal of the line being processed. 

> Why do we get into state d) at all? Why
> isn't the A before the \r put into the next chunk so the \n would fit?

We are looking at the raw response from the imap server here. Since the requested chunk size is how many literal chars to return in the imap response, the chunk could end anywhere, even rarely between an \r and \n. At least that's what I observe.

> So I think we're on the right track here. It would be good to document all the processing since this is difficult stuff. I ventured into it a tiny little bit myself here: https://hg.mozilla.org/comm-central/rev/b143aa60d983#l1.12

Yes, I've been pretty frustrated trying to understand how all this and other stuff works. The comments in the imap code are usually not real helpful and a lot are "cute" remarks by, I assume, the original authors that don't add much. However, I did find your bug reports for changing the way caching works to be helpful when working on bug 1430480.

Back to chunking. Yesterday I tried again to cause a chunked response into the mbox file and was unable to. With the huge email provided by the reporters, it would first fetch it all by chunks and then fetch the whole thing in one big fetch and write that to mbox. Only if I disable offline storage did it only do chunking. Then it saves it to disk cache. So I need to look again at this.

Comment 51

[Jorg K (CEST = GMT+2)]

Thanks for the explanation, it's slowly becoming clearer. I think we need to document the chunk variations. You already gave examples a) b) c) and d), but I guess we'd need complete examples with one entire chunk and then the next one.

So if I understand it correctly so far, a chunk is
(data1\r\ndata2\r\ndata3)\r\n or 
(data1\r\ndata2\r\ndata3\r\n)\r\n or in unfortunate cases
(data1\r\ndata2\r\ndata3\r)\r\n
and the \n of the literal follows in the next chunk, right? - like so (\ndata4 ...etc.)

In a) fCurrent holds data1\r\n or data2\r\n
In b) fCurrent holds data3)\r\n
In c) fCurrent holds data3\r\n)\r\n
In d) fCurrent holds data3\r)\r\n

displayEndOfLine always points to the last byte of literal data, so in b)-d) to the character preceding the ), only in a) to the \n since we're not at the end of a chunk.

So the aim is here to reunite the \r from one chunk with the \n from the next and discard it from the literal in the next chunk.

The existing code seems to remove the \n from the following chunk (char *usableCurrentLine = PL_strdup(fCurrentLine + 1); etc.) but that's too late, it missed the chance to add the \n to the previous chunk.

I guess the penny finally dropped. So please let me know when the patch is ready for review. We'll come up with a nice big comment block for future generations ;-)

Comment 52

[gene smith]

Attachment: 1216951-review-changes0.patch

I don't know a reason for the crashes pointed to in bug 1264302. But I have been extensively using, abusing and testing the chunking code, off and on, for several weeks, mostly with my changes in place, and haven't seen a crash.

In order to get reliable chunking with the the reporter's test emails I have to, of course, set the chunking size lower, e.g., 7001.  Also, this small temporary change causes tb to always call FetchTryChunking() instead of just downloading the whole message:

diff --git a/mailnews/imap/src/nsImapProtocol.cpp b/mailnews/imap/src/nsImapProtocol.cpp
--- a/mailnews/imap/src/nsImapProtocol.cpp
+++ b/mailnews/imap/src/nsImapProtocol.cpp
@@ -2615,17 +2615,17 @@ void nsImapProtocol::ProcessSelectedStat
       case nsIImapUrl::nsImapMsgFetch:
       case nsIImapUrl::nsImapMsgFetchPeek:
       case nsIImapUrl::nsImapMsgDownloadForOffline:
       case nsIImapUrl::nsImapMsgPreview:
         {
           nsCString messageIdString;
           m_runningUrl->GetListOfMessageIds(messageIdString);
           // we don't want to send the flags back in a group
-          if (HandlingMultipleMessages(messageIdString) || m_imapAction == nsIImapUrl::nsImapMsgDownloadForOffline
+          if (HandlingMultipleMessages(messageIdString) /* temp gds || m_imapAction == nsIImapUrl::nsImapMsgDownloadForOffline */
              || m_imapAction == nsIImapUrl::nsImapMsgPreview)
           {
             // multiple messages, fetch them all
             SetProgressString(IMAP_MESSAGES_STRING_INDEX);

             m_progressCurrentNumber[m_stringIndex] = 0;
             m_progressExpectedNumber = CountMessagesInIdString(messageIdString.get());

Comment 54

[Jorg K (CEST = GMT+2)]

Comment on attachment 8953891 [details] [diff] [review]
1216951-review-changes0.patch

Thanks for investigating this, the patch looks good and the comment you added is very useful.

However, with this patch
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_chunkLastLF.js
fails, so that needs further investigation.

I also have a few nits I'll mention in the next comment.

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: 1216951-review-changes0.patch - minor nits in comment and indentation

Instead of complaining about nits, I did a few very minor tweaks. Further comments to follow.

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 8956938 [details] [diff] [review]
1216951-review-changes0.patch - minor nits in comment and indentation

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

Here are further comments.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +3117,5 @@
> +        // "1s8AA5i4AAvF4QAG6+sAAD0bAPsAAAAA1OAAC)\r\n", where ")\r\n" are non-literals.
> +        // This an example of the last "good" line of a chunk that terminates with \r\n
> +        // "FxcA/wAAAALN2gADu80ACS0nAPpVVAD1wNAABF5YAPhAJgD31+QABAAAAP8oMQD+HBwA/umj\r\n"
> +        // followed by another line of non-literal data:
> +        // " UID 1004)\r\n". These two are concatenated into a single string pointed to

Sorry, I'm not so familiar with the structure of the data, what is the "UID 1004" and is the trailing space significant?

@@ +3176,5 @@
> +        {
> +          // This is first line of next chunk consisting only of '\n'.
> +          // Change the '\n' to 0. Probably unlikely that it is not '\n' when
> +          // chunksStartsWithNewline is true, but still check with assert below.
> +          fCurrentLine[0] = 0;

I dislike inserting null characters into a null-terminated string. Can you not use fCurrentLine+1 below?

Looking at HandleMessageDownLoadLine()
https://dxr.mozilla.org/comm-central/rev/2edea7714121b2f25cc155b0592520ce1ebfa6fa/mailnews/imap/src/nsImapProtocol.cpp#3844
I see string compares and strlen() which won't give the correct result if you null-terminate the line right at the start.

I actually tried with fCurrentLine+1, but that gave the same test failure.

Comment 57

[gene smith]

(In reply to Jorg K (GMT+1) from comment #56)
> Comment on attachment 8956938 [details] [diff] [review]
> 1216951-review-changes0.patch - minor nits in comment and indentation
> 
> Review of attachment 8956938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here are further comments.
> 
> ::: mailnews/imap/src/nsImapServerResponseParser.cpp
> @@ +3117,5 @@
> > +        // "1s8AA5i4AAvF4QAG6+sAAD0bAPsAAAAA1OAAC)\r\n", where ")\r\n" are non-literals.
> > +        // This an example of the last "good" line of a chunk that terminates with \r\n
> > +        // "FxcA/wAAAALN2gADu80ACS0nAPpVVAD1wNAABF5YAPhAJgD31+QABAAAAP8oMQD+HBwA/umj\r\n"
> > +        // followed by another line of non-literal data:
> > +        // " UID 1004)\r\n". These two are concatenated into a single string pointed to
> 
> Sorry, I'm not so familiar with the structure of the data, what is the "UID
> 1004" and is the trailing space significant?

"trailing space" = \r\n ?? If that's what you are referring to, yes it is significant since all lines end in \r\n typically.

This is an example how the response looks when the fetch in chunks is done for a "bodystructure" part, e.g., just an attachment, for message unique id 1004. The *leading* space before "UID" is present in the actual response I extracted this example from. When the whole message is fetched (which most typically happens), the chunks don't have the " UID XXX)\r\n" and just end in ")\r\n".

> 
> @@ +3176,5 @@
> > +        {
> > +          // This is first line of next chunk consisting only of '\n'.
> > +          // Change the '\n' to 0. Probably unlikely that it is not '\n' when
> > +          // chunksStartsWithNewline is true, but still check with assert below.
> > +          fCurrentLine[0] = 0;
> 
> I dislike inserting null characters into a null-terminated string. Can you
> not use fCurrentLine+1 below?
> 
> Looking at HandleMessageDownLoadLine()
> https://dxr.mozilla.org/comm-central/rev/
> 2edea7714121b2f25cc155b0592520ce1ebfa6fa/mailnews/imap/src/nsImapProtocol.
> cpp#3844
> I see string compares and strlen() which won't give the correct result if
> you null-terminate the line right at the start.
> 
> I actually tried with fCurrentLine+1, but that gave the same test failure.

I will look closer at what you are saying here about the null string. You provided me info about running these test long ago and I will have to dig it out if you are asking me to rationalize or fix the test failure.

Comment 58

[Jorg K (CEST = GMT+2)]

Sorry, I meant to write *leading* space.

As for the tests: Just paste
  mozilla/mach xpcshell-test mailnews/imap/test/unit/test_chunkLastLF.js
into the command line.

If you want to attach a debugger, make that:
  mozilla/mach xpcshell-test --interactive mailnews/imap/test/unit/test_chunkLastLF.js
and terminate with |quit();| when you're done.

Comment 59

[gene smith]

The "fake" imap server seems to be reacting in a way I didn't see with the "real world" servers I tested with. So I need to look at this closer and do more testing with more servers. Hopefully I will have something tomorrow.

Comment 60

[Wayne Mery (:wsmwk)]

great to see progress here because I have just updated bug 1264302 with more crash signatures, and if the connection to this bug is solid then patching this will solve a 52.6.0 topcrash

Comment 61

[Wayne Mery (:wsmwk)]

And one of the most prominent reporter of crashes in those bugs is Richard

Comment 62

[gene smith]

Attachment: 1216951-review-changes1.patch

Ok, now it passes the test_chunkLastLF.js test. Also, no longer "nuking" the \n.

I returned most of the block of code that I previously removed now that I understand better what it is for. Unlike all the other real-world servers I have checked, the "fake" (virtual) server used by the tests responses after a \r\n split with a line consisting of "\r\n" instead of just "\n". This previously removed block just changes the received "\r\n" line to "\n".

I re-tested with several real servers and verified that the lines that split the \r and \n at the end of a chunk still don't cause extra or missing \r and \n chars in the file that would affect a calculated signature or checksum.

Wayne, in testing this I haven't seen any actual "crash". The problem may be at a deeper level where the actual data "lines" that I work with here are built-up from the socket/stream data.

Comment 63

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957430 [details] [diff] [review]
1216951-review-changes1.patch

Excellent work! I really appreciate that you saw this through to the end even despite our nasty "fake" server quirks. Thanks for documenting everything well in comments! It's great that you have all all those servers at hand to make sure that things work in the real world.

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: 1216951-review-changes1.patch - patch for checkin, fixed minor nits in comment and indentation

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4868c63422f5
Fix broken handling of split CR and LF between chunks. r=jorgk

Comment 67

[Wayne Mery (:wsmwk)]

giulia, are your crashes gone in version 60?

Comment 68

[Matt]

Is this an indication of a regression? https://support.mozilla.org/en-US/questions/1249414

Comment 69

[gene smith]

The patch above fixed chunking for version 60. There is another fix to this fix that fixed an assertion crash under rare condition, bug 1451137 that is targeted for 62. So don't see how that could cause a difference between 60.5 and 60.4.

I'm a little rusty on when chunking occurs but I think the mime part has to be larger than 98k and fetched in parts. This mostly occurs when you have offline storage disabled for the folder/mailbox and attachments are not viewed inline. Word docs would not be viewed inline so chunking would only occur for that user if offline storage is disabled for the containing folder.

I'm not familiar with the add-on mentioned but disabling chunking in the config editor would also determine if chunking is causing the problem without the add-on, i.e., mail.server.default.fetch_by_chunks set false will disable chunking for all folders in all accounts.

If the user could provide a sample email that has the problem it might expedite a solution.

Comment 70

[Jorg K (CEST = GMT+2)]

Umm, so we're looking for a difference between 60.4 and 60.5? I uplifted
88bb91a8fb57 Gene Smith — Bug 1494764: Ignore missing \n on last line of last chunk. r+a=jorgk
https://hg.mozilla.org/releases/comm-esr60/rev/88bb91a8fb57

Would that have caused a problem.

Comment 71

[Eric Moore]

The 60.5.1 preview from https://support.mozilla.org/en-US/questions/1249414 appears to have worked around Bug 1527632 (a ')' appended to the end of IMAP messages) that occurs with version 60.5. That problem only occurred with certain IMAP accounts.