152468 - string truncation causes filter misses

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

I was seeing some filter misses that I didn't expect in my spam filters, so I
started putting printfs in the code.  The problem seems to happen at
nsMsgBodyHandler::GetNextLine around this line of code:

while (eatThisLine && length >= 0);  // if we hit eof, make sure we break out of
this loop. Bug #:
  return length;

What ends up happening is that sometimes -1 gets returned, and then the next
call to GetNextLine ends up down in GetNextFilterLine in this code:

  else if (m_headersSize == 0) {
    buf[0] = '\0';
  }

Which causes the last byte of the previous header to be overwritten with null.

It's possible that just changing the while loop to look for length > 0 is the
correct fix; I'm not entirely sure.

Comment 1

[David :Bienvenu]

-> Navin

Comment 2

[Navin Gupta]

dmose, can you elaborate what type of filters you have ? thx.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

These are filters that are filtering over IMAP against "custom header".  Example
filter:

name="spam: content-type"
enabled="yes"
type="1"
action="Move to folder"
actionValue="imap://[e-mail]/Potential Spam"
condition="AND (\"Content-Type\",contains,charset=) AND
(\"Content-Type\",doesn't contain,charset=us-ascii) AND
(\"Content-Type\",doesn't contain,charset=iso-8859-1) AND
(\"Content-Type\",doesn't contain,charset=\\"us-ascii\\") AND
(\"Content-Type\",doesn't contain,charset=\\"iso-8859-1\\")"

This filter often incorrectly matches one of the last two and conditions because
the final quote mark has been trounced by a null.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

My initial diagnosis was wrong.  The real problem here is that
nsParseMailMessageState::FinalizeHeaders() is destructive, and writes NULLs into
the header in several places (notably Message-ID, Content-Type: charset= , and
Newsgroups) in order to cheaply truncate strings.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I've got a patch... taking.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v1

Generate new truncated strings, rather than writing into ones that should be
read-only.  This is the right fix, assuming nothing else depends on the old
behaviorial side-effects (which seems fairly unlikely).

Comment 7

[David :Bienvenu]

So, in this instance, to be clear, you were counting on the trailing " in the
charset header to be there. This fix looks OK to me, but the other alternative
is to simply save and restore the crunched character(s) instead of copying the
string, which would be faster. 

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

bienvenu: yes on all counts.  I considered doing the crunch/restore solution,
but it was clear that this data was already shared, and not being confident that
I understood all the sharing that was going on, I had threadsafety fears.  I
also didn't like the fact that it forces us to violate |const char *|-typed
data.  What would you prefer I do?

Comment 9

[David :Bienvenu]

Comment on attachment 90360 [details] [diff] [review]
patch, v1

sr=bienvenu

Comment 10

[David :Bienvenu]

there aren't any threadsafety issues - this data is only manipulated on the ui
thread. However, I think the copying is OK. It's not going to slow us down
appreciably.

Comment 11

[Navin Gupta]

Comment on attachment 90360 [details] [diff] [review]
patch, v1

r=naving

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.