Closed
Bug 644567
Opened 13 years ago
Closed 13 years ago
large search text causes thunderbird to hang then crash [@ operator new(unsigned int) | gfxTextRun::operator new(unsigned int, unsigned int, unsigned int)]
Categories
(Thunderbird :: Folder and Message Lists, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 8.0
People
(Reporter: hacktalkblog, Assigned: choudharypranay)
Details
(Keywords: crash)
Crash Data
Attachments
(2 files)
1.17 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
1.02 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.14) Gecko/20110218 Firefox/3.6.14 ( .NET CLR 3.5.30729; .NET4.0E) Build Identifier: 3.1.9 While having a lot of lorem ipsum text in my clipboard I thought I copied a keyword to search for more messages and ended up pasting about 2 million characters into the search bar which caused thunderbird to hang and eventually crash bringing up the Mozilla Crash Reporter. Reproducible: Always Steps to Reproduce: 1. Put a large body of text into your clipboard (http://www.lipsum.com/ to generate text) 2. Paste this large text buffer into your search bar 3. Watch as Thunderbird crashes Actual Results: Thunderbird will hang for a while and eventually crash, I believe this is due to memory exhaustion. Expected Results: Thunderbird should have a maximum amount of potential characters that it searches for to avoid this issue.
Comment 1•13 years ago
|
||
Can you provide a crashID ? (see http://support.mozillamessaging.com/bg/kb/Mozilla+Crash+Reporter)
Keywords: crash
Comment 2•13 years ago
|
||
is this in "Search all messages"? Or quick filter bar? no crash here using trunk build for either of them. 32bit xp
Reporter | ||
Comment 3•13 years ago
|
||
Here's the crash report: http://crash-stats.mozilla.com/report/index/bp-03c62d77-ed15-4c4b-9f33-56f872110324 It's in "Search All Messages... <Ctrl+K>" I got it to crash on Windows XP SP3 32-bit in a VM As well as Vista Ultimate 64-bit. Looks like EDI ends up getting filled with the junk data and an access violation occurs. I looked to see if this would somehow be exploitable by a hacker to gain code execution but there is no control over any of the registers or anything in the SEH chain.
Comment 4•13 years ago
|
||
operator new(unsigned int) | gfxTextRun::operator new(unsigned int, unsigned int, unsigned int) is not a rare crash https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&date=&range_value=4&range_unit=weeks&query_search=signature&query_type=exact&build_id=&process_type=all&do_query=1&query=operator%20new%28unsigned%20int%29%20|%20gfxTextRun%3A%3Aoperator%20new%28unsigned%20int%2C%20unsigned%20int%2C%20unsigned%20int%29 all are Unhandled C++ Exception example with comments is bp-1ab2688a-10d6-4241-bea7-b61d12110228 Two mails have been mixed within thunderbird and thunderbird is not able to display the correct contents of the selected mail. The mails are correct on the server, as the mails are correctly read from mail clients on other computers.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: large search text causes thunderbird to hang → large search text causes thunderbird to hang then crash [@ operator new(unsigned int) | gfxTextRun::operator new(unsigned int, unsigned int, unsigned int)]
Updated•13 years ago
|
Component: Search → Graphics
Product: Thunderbird → Core
QA Contact: search → thebes
Comment 5•13 years ago
|
||
This is typical in mozilla-1.9.2 for a testcase that creates huge textruns; eventually we run out of memory, and because gfxTextRun::operator new does the bulk of the allocation needed for textruns, that's usually where the OOM exception happens. FWIW, current Gecko trunk code (2.0) tries to handle this more robustly, by pre-allocating the memory needed using a fallible allocator and simply discarding textruns that can't be allocated, but there are still plenty of ways to trigger an OOM crash by injecting ridiculous amounts of text in various places. As long as we're crashing safely, that's not in itself a major problem. If you have an example where an allocation failure does _not_ cause an immediate crash, nor handles the failure safely, but (for example) continues using an incorrectly-sized or null buffer, then we have a Core bug that we need to fix, but I don't see evidence of that here at present. In a case such as the search bar, it might be reasonable for the application to set some kind of arbitrary (but larger-than-anyone-could-realistically-want....perhaps several thousand characters?) limit on the amount of text that can be inserted. Moving this back to Thunderbird for that possibility to be considered.
Component: Graphics → General
OS: Windows Vista → Windows 2000
Product: Core → Thunderbird
QA Contact: thebes → general
Updated•13 years ago
|
Component: General → Search
QA Contact: general → search
Comment 6•13 years ago
|
||
Yes, it would make sense for us to set a maxlength on our search fields.
Keywords: helpwanted
Comment 7•13 years ago
|
||
Bryan could you give us a good limit in your opinion on what value we should settle for ?
Reporter | ||
Comment 8•13 years ago
|
||
Honestly I can't see any person searching for more than 1000 characters. Even 200 is pushing it but with 1000 we have a decent enough gap from what we think the average user will search for and the most anyone should ever have to use.
Comment 9•13 years ago
|
||
The tokenizer in use for global search has an upper limit on the maximum size of a token (20 characters), although our ability to perform phrase/boolean searches means that multiple tokens can be present. Because multiple tokens result in multiple independent inverted-index list retrievals which are then (basically) intersected, performance sanity suggests we would still want to cap the number of tokens. The non-global search mechanism I believe is naive about line breaks and so presumably tops out at the line length in properly wrapped messages. I would say never more than 192, and that is probably still too much rope.
Comment 10•13 years ago
|
||
(In reply to comment #9) > I would say never more than 192, and that is probably still too much rope. That certainly seems a large enough value. If we wanted to be cute we could use 144
Comment 11•13 years ago
|
||
Personally, I'd prefer that we lean more to the generous side. Yes, a couple of hundred is probably more than anyone really needs, but it takes a lot more than that to actually cause problems (comment #0 mentions "pasted about 2 million characters...", for example), so I'd suggest at least a thousand, maybe even 4K or 16K or something "huge" like that. This should ideally be a limit that nobody ever encounters except by accident (as in the report here) or in the course of deliberate stress-testing/attempted DoS.
Comment 12•13 years ago
|
||
But for the specific Thunderbird text-boxes of the global search box and the quick filter bar, it seems reasonable to cap these lower since the "maxlength" provides us with a per-instance cap (and we are not pursuing a global widget cap) and there is basically zero chance of a search string that long finding anything (even if such a string actually exists in the database), at least on a human-tolerable time scale. From a UX perspective, I would find a much lower limit on the order of "only what is visible" most compelling in the case someone somehow manages to accumulate parts of a search string they can't see. However, we have some serious screen real estate problems with search boxes (primarily the quick filter bar) and tend to clear the boxes frequently enough that it's not fatal, so that's not completely practical or necessary.
Comment 13•13 years ago
|
||
What we need is clear -> setting the good first bug status.
Whiteboard: [good first bug]
Updated•13 years ago
|
Crash Signature: [@ operator new(unsigned int) | gfxTextRun::operator new(unsigned int, unsigned int, unsigned int)]
Assignee | ||
Comment 14•13 years ago
|
||
I'm interested to work on it. So the only thing I've to do is to set the maxlength of search text box to a number like 4K. Please correct me if I'm wrong.
Comment 15•13 years ago
|
||
(In reply to Pranay Choudhary from comment #14) > I'm interested to work on it. > > So the only thing I've to do is to set the maxlength of search text box to a > number like 4K. Please correct me if I'm wrong. Yes, but please use 192. (As I noted previously, neither of the search mechanisms is going to do anything useful with the extra text other than take a lot longer to run, and in the case of global search, potentially generate pathological overflow of the SQLite cache.)
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #552092 -
Flags: review?(bugmail)
Comment 17•13 years ago
|
||
Comment on attachment 552092 [details] [diff] [review] Added maxlength="192" for searchAllMessages textbox looks great, thanks! pushed to trunk: http://hg.mozilla.org/comm-central/rev/2c5755959cde
Attachment #552092 -
Flags: review?(bugmail) → review+
Comment 18•13 years ago
|
||
We should probably also fix this for the quick filter bar since the text runs are the same. Any interest in also providing a patch for that too? :) (If not, let me know and I can clear the assigned status for you.)
Assignee: nobody → choudharypranay
Severity: critical → normal
Status: NEW → ASSIGNED
Component: Search → Folder and Message Lists
QA Contact: search → folders-message-lists
Target Milestone: --- → Thunderbird 8.0
Version: unspecified → Trunk
Updated•13 years ago
|
Comment 19•13 years ago
|
||
FWIW, I can't find stacks with gfxTextRun in top of stack for v5 or higher. But definitely does have users crashing in v3.
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #552332 -
Flags: review?(bugmail)
Assignee | ||
Comment 21•13 years ago
|
||
Just curious..... Why the number 192?
Comment 22•13 years ago
|
||
(In reply to Pranay Choudhary from comment #21) > Just curious..... Why the number 192? It's arbitrary and power of 2-ish, but I tried to pick a length well beyond the length at which gloda and non-gloda searches are unlikely to return valid responses. I would expect non-gloda search to succeed at searches at most around the length of a single line, so assume 80 columns (larger than average) * 2 + 32 :) That also gives us ~9 maximum-length gloda tokens (or fewer but longer tokens that get truncated).
Updated•13 years ago
|
Attachment #552332 -
Flags: review?(bugmail) → review+
Comment 23•13 years ago
|
||
Thanks again! pushed to trunk: http://hg.mozilla.org/comm-central/rev/a4076abb21b7
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.
Description
•