Closed Bug 454344 Opened 13 years ago Closed 13 years ago
IMime Emitter add All Headers method not js-safe as 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...
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 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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0b1
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.
I like Neil's suggestion and I think that's worth exploring...
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 #337696 - Flags: review? → review?(bugmail)
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 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.
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.
(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.
I looked in the debugger, and I don't need to subtract 1, and I can use SubString as Neil suggests...
Attachment #337940 - Flags: review? → review?(bugmail)
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+
carrying forward Neil's sr. Neil's right, Substring deals with the empty string correctly, from what I can tell.
Attachment #337965 - Flags: review?(bugmail) → review+
fix checked in - 98654f6296b5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.