466796 - In-Reply-To/References header parsing incorrect when mixed message-id and non-message-id content present or are just empty

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P1)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 improve references parser logic, add unit tests

With a header like so, arrow delimited (--> and <--, yes there's a newline):
-->In-Reply-To: <[e-mail]> from Anon User at "May 5,
 2003 09:45:21 am"<---

Madness like this can lead to the following references showing up:
-->[e-mail]<--
-->from Anon User at "May 5,
 2003 09:45:21 am"<--
--><-- (I'm not exactly sure what made this one, but it happens.)

These things are bad for gloda's conversation logic.  The empty reference is the worst because it can result in all poorly-formed messages ending up in one giant conversation.  (I expect we're unlikely to have collisions on the strings with timestamps in them.)  But the gibberish ones are still bad because they can make gloda believe messages are missing when they are not, could conceivably result in contradictory understandings of the thread structure, etc.  I say conceivably because I don't think any of our algorithms would get confused, but I could see how you could write one that would.

I have modified the references parsing logic so that it handles this case properly, while still handling all kinds of insane but legal (under rfc 822) cases it previously supported.  Unit tests have been added.  When examining the diff, be aware that some of the code still used tabs and I have whitespaced that code to avoid ridiculous and confusing results.

This really wants to get in beta 1.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

If there any issues with the patch as-is, please feel free to correct them and take ownership of the patch or what not in any way that allows this to get into the tree fastest.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I forgot to mention that this solution does not require reindexing the msf files since the string is what is actually stored in the mork property, and the parsing happens on-demand.

Comment 3

[David :Bienvenu]

I've read through the patch, and it looks OK - I'll apply it and test it...

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 350140 [details] [diff] [review]
v1 improve references parser logic, add unit tests

This looks ok to me.

Comment 5

[David :Bienvenu]

Comment on attachment 350140 [details] [diff] [review]
v1 improve references parser logic, add unit tests

I'll land this, after fixing the braces here:

+  for(; *ptr ; ptr++) {
+    if (*ptr == '>')

Comment 6

[David :Bienvenu]

fixed