nsIMimeEmitter addAllHeaders method not js-safe as is

RESOLVED FIXED in Thunderbird 3.0a3

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: asuth, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 3.0a3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 337591 [details] [diff] [review]
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...
(Reporter)

Comment 1

10 years ago
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

10 years ago
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.
(Reporter)

Comment 3

10 years ago
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.
Attachment #337591 - Flags: superreview?(bienvenu)
Attachment #337591 - Flags: review?(bienvenu)
(Reporter)

Updated

10 years ago
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0b1

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
I like Neil's suggestion and I think that's worth exploring...
(Assignee)

Comment 6

10 years ago
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?
Attachment #337591 - Flags: superreview?(bienvenu)
Attachment #337591 - Flags: superreview-
Attachment #337591 - Flags: review?(bienvenu)
Attachment #337591 - Flags: review-
(Assignee)

Comment 7

10 years ago
Created attachment 337696 [details] [diff] [review]
use ACString
Attachment #337696 - Flags: superreview?(neil)
Attachment #337696 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #337696 - Flags: review? → review?(bugmail)

Comment 8

10 years ago
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

10 years ago
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.
(Assignee)

Comment 10

10 years ago
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

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Created attachment 337940 [details] [diff] [review]
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...
Attachment #337696 - Attachment is obsolete: true
Attachment #337940 - Flags: superreview?(neil)
Attachment #337940 - Flags: review?
Attachment #337696 - Flags: superreview?(neil)
Attachment #337696 - Flags: review?(bugmail)
(Assignee)

Updated

10 years ago
Attachment #337940 - Flags: review? → review?(bugmail)

Comment 13

10 years ago
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.
Attachment #337940 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 14

10 years ago
Created attachment 337965 [details] [diff] [review]
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.
Assignee: bugmail → bienvenu
Attachment #337940 - Attachment is obsolete: true
Attachment #337965 - Flags: superreview+
Attachment #337965 - Flags: review?(bugmail)
Attachment #337940 - Flags: review?(bugmail)
(Reporter)

Updated

10 years ago
Attachment #337965 - Flags: review?(bugmail) → review+
(Assignee)

Comment 15

10 years ago
fix checked in - 98654f6296b5
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.