454344 - nsIMimeEmitter addAllHeaders method not js-safe as is

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

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

Attachment: change addAllHeaders to use size_is

rkent experienced a crash and tracked it down to likely being due to a lack of null-termination on addAllHeaders which passes a char * and a size value.  Apparently it is documented that it doesn't null-terminate, but since the method is not [noscript], it only leads to tears.

I have a patch that attempts to use the size_is annotation to make things happy.  Attaching...

Comment 1

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

To elaborate, my changes to allow a javascript-based nsIMimeEmitter on bug 447842 created the new situation where this method would actually be called through XPConnect.  Without an annotation (and maybe with; we'll see how the patch works), XPConnect assumes the string is null-terminated and goes looking for a nul character.  This causes crashes, at least on rkent's windows debug build.

Comment 2

[Kent James (:rkent)]

Before I applied this patch, my gloda installation was crashing during reindexing, usually within the first 500 messages. I applied the patch, and did not have any crashes after over 4000 messages. So this seems to fix the issue that was causing my crash.

Comment 3

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

Comment on attachment 337591 [details] [diff] [review]
change addAllHeaders to use size_is

The change to use size_is required a change of allheadersize from "long" to "unsigned long".  I have not propagated this change other than the required implementation signature change to be consistent and build.  I started to change it everywhere, but it's a longish thread to pull on (first the mime internals, then the nsIMimeHeader and implementations, etc.)  I'm not sure what the desired course of action is.

Comment 4

[[e-mail]]

Another possible approach is to change the parameters to a single ACString; the C++ caller can use nsDependentSubstring(ptr, len) to create a suitable string and the C++ callee can use .BeginReading() and .Length() to recover the values.

Comment 5

[David :Bienvenu]

I like Neil's suggestion and I think that's worth exploring...

Comment 6

[David :Bienvenu]

Comment on attachment 337591 [details] [diff] [review]
change addAllHeaders to use size_is

I think you'd need to rev the interface uuid for the change to the method...but I'm going to attach a patch trying Neil's approach - if you're OK with it, can you try it out?

Comment 7

[David :Bienvenu]

Attachment: use ACString

Comment 8

[Kent James (:rkent)]

I tested indexing using gloda with Bienvenu's attachment 337696 [details] [diff] [review], and did not experience any crashing while indexing 10,000 emails.

Comment 9

[[e-mail]]

Comment on attachment 337696 [details] [diff] [review]
use ACString

>-    return emitter->AddAllHeaders(allheaders, allheadersize);
>+    return allheadersize ? emitter->AddAllHeaders(
>+          nsDependentCSubstring(allheaders, allheaders + allheadersize - 1)) :
>+          emitter->AddAllHeaders(EmptyCString());
Why the -1?

The string guide suggests that you should use Substring() instead of directly constructing an nsDependentCSubstring.

Annoyingly the external API is actually easier to use here, because it has a Substring(char*, PRUint32) version.

Comment 10

[David :Bienvenu]

the -1 is because I thought the second param to nsDependentCString was a pointer to the last char in the string, not one past...and the string is not null-terminated. But I could be off by one...

I tried to use Substring, but as you say, w/o the external API, it's difficult.

Comment 11

[[e-mail]]

(In reply to comment #10)
> the -1 is because I thought the second param to nsDependentCString was a
> pointer to the last char in the string, not one past...and the string is not
> null-terminated. But I could be off by one...
> 
> I tried to use Substring, but as you say, w/o the external API, it's difficult.
The internal Substring(char*, char*) calls nsTDependentSubstring_CharT() (which is nsDependentCSubstring in this case) which simply calculates end - start. Fortunately this is inline, so I guess any decent compiler will optimise allheaders + allheadersize - allheaders back to allheadersize.

Comment 12

[David :Bienvenu]

Attachment: use substring, correct off by one error

I looked in the debugger, and I don't need to subtract 1, and I can use SubString as Neil suggests...

Comment 13

[[e-mail]]

Comment on attachment 337940 [details] [diff] [review]
use substring, correct off by one error

>+    return allheadersize ? emitter->AddAllHeaders(
>+          Substring(allheaders, allheaders + allheadersize)) :
>+          emitter->AddAllHeaders(EmptyCString());
Now you don't need to check the size either... sr=me with that fixed.

Comment 14

[David :Bienvenu]

Attachment: fix to not check allheadersize, per Neil

carrying forward Neil's sr. Neil's right, Substring deals with the empty string correctly, from what I can tell.

Comment 15

[David :Bienvenu]

fix checked in - 98654f6296b5