Closed
Bug 454344
Opened 17 years ago
Closed 17 years ago
nsIMimeEmitter addAllHeaders method not js-safe as is
Categories
(MailNews Core :: MIME, defect)
MailNews Core
MIME
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0a3
People
(Reporter: asuth, Assigned: Bienvenu)
Details
Attachments
(2 files, 2 obsolete files)
2.38 KB,
patch
|
Bienvenu
:
review-
Bienvenu
:
superreview-
|
Details | Diff | Splinter Review |
4.01 KB,
patch
|
asuth
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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•17 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•17 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•17 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•17 years ago
|
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0b1
Comment 4•17 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•17 years ago
|
||
I like Neil's suggestion and I think that's worth exploring...
Assignee | ||
Comment 6•17 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•17 years ago
|
||
Attachment #337696 -
Flags: superreview?(neil)
Attachment #337696 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #337696 -
Flags: review? → review?(bugmail)
Comment 8•17 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•17 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•17 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•17 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•17 years ago
|
||
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•17 years ago
|
Attachment #337940 -
Flags: review? → review?(bugmail)
Comment 13•17 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•17 years ago
|
||
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•17 years ago
|
Attachment #337965 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 15•17 years ago
|
||
fix checked in - 98654f6296b5
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•