Closed Bug 1159604 Opened 5 years ago Closed 5 years ago

OOM crash in Firefox 37.0.2 when pasting large amounts of data

Categories

(Core :: General, defect, critical)

37 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: xowiatech, Assigned: dmajor)

References

Details

(Keywords: crash, crashreportid)

Crash Data

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150415140819

Steps to reproduce:

When you visit site like http://www.lettercount.com/ or any other site where you can put enter your BIG DATA (~250 Million Digit) like (1234512345123451234512345123451234512345123451234512345.....12345)


Actual results:

When you enter BIG DATA so Firefox will not handle it and application will crash.


Expected results:

It should process the data either give some kind of error instead of crashing application.
Crash Signature: 1) bp-df9a7d84-9520-448d-961b-e71e92150429 2) bp-4a12bf72-843e-4d44-a412-6e7f02150429
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Crash signatures:

1) bp-df9a7d84-9520-448d-961b-e71e92150429
2) bp-4a12bf72-843e-4d44-a412-6e7f02150429
Group: core-security
Severity: normal → critical
Crash Signature: 1) bp-df9a7d84-9520-448d-961b-e71e92150429 2) bp-4a12bf72-843e-4d44-a412-6e7f02150429 → [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | ConvertUnknownBreaks<wchar_t>]
Keywords: crash, crashreportid
Product: Firefox → Core
Summary: Firefox 37.0.2 crashing due to BIG Data → OOM crash in Firefox 37.0.2 when pasting large amounts of data
Benjamin, looks like this crashes in xpcom/io... can we make things there not crash and burn if they get too much data, and/or should we change the clipboard handling code to truncate large amounts of text or something, or...? :-)
Flags: needinfo?(benjamin)
302 dmajor
Flags: needinfo?(benjamin) → needinfo?(dmajor)
> OOM Allocation Size 	 620046000
Uh, yeah, that's bad.

The feature owners can weigh in if they want something more fancy, but at the very least it should be a fallible allocation. There's a null-check there already :-( I guess the code didn't know that nsMemory::Alloc was infallible.
Flags: needinfo?(dmajor)
I am pretty sure I've seen this pattern before, where code calls an infallible allocation but is prepared to handle failure. It might be worthwhile auditing all callers of nsMemory::Alloc (now moz_xmalloc in m-c) for this issue.
Well I'll take it if nobody else will.
Assignee: nobody → dmajor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I find it amusing that in its hg-lifetime, this file has had exactly one functional change and 15 stylistic changes.
Attached patch Use fallible allocations (obsolete) — Splinter Review
Attachment #8600605 - Flags: review?(bugs)
Don't we want to use moz_malloc, since free() (not delete) is used for releasing the memory?
That's a good point. I was following glandium's dev-platform advice a little too aggressively!
Attachment #8600605 - Attachment is obsolete: true
Attachment #8600605 - Flags: review?(bugs)
Attachment #8600906 - Flags: review?(bugs)
Comment on attachment 8600906 [details] [diff] [review]
Use fallible allocations

Based on https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation
we should probably use moz_malloc here. With that, r+
Attachment #8600906 - Flags: review?(bugs) → review+
https://groups.google.com/forum/#!topic/mozilla.dev.platform/ffSQld7olEE
"PSA: moz_malloc, moz_realloc, moz_calloc and moz_free are no more"

I'll ping glandium to update the MDN pages. There's some old prose in there that I'm not sure about so I don't feel comfortable just blindly doing s/moz_malloc/malloc/ by myself.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1163109f2895
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
I cannot crash it on my Win 7 machine, but Socorro data ([1], [2]) seems to indicate that this no longer happens in Firefox 40 or later.

Reporter, does this work for you now? The fix should be in the latest Firefox 40 build: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/40.0b4-candidates/build1/win32/en-US/

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=OOM+%7C+large+%7C+mozalloc_abort%28char+const%2A+const%29+%7C+mozalloc_handle_oom%28unsigned+int%29+%7C+moz_xmalloc+%7C+ConvertUnknownBreaks%3Cwchar_t%3E#tab-sigsummary
[2] - https://crash-stats.mozilla.com/report/list?signature=OOM%20%7C%20large%20%7C%20mozalloc_abort(char%20const*%20const)%20%7C%20mozalloc_handle_oom(unsigned%20int)%20%7C%20moz_xmalloc%20%7C%20ConvertUnknownBreaks%3CT%3E
Crash Signature: [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | ConvertUnknownBreaks<wchar_t>] → [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | ConvertUnknownBreaks<wchar_t>] [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | ConvertUnknownBre…
Flags: needinfo?(xowiatech)
Are you sure? Check these crash report ids.
1) Firefox 39.0 - bp-c44f27d7-3137-4ca1-b056-97ed72150721
2) Firefox 40.0b- bp-bca36083-37b4-4bff-b0ac-55e9d2150721
Flags: needinfo?(xowiatech)
(In reply to Xowia Technologies from comment #19)
> Are you sure? Check these crash report ids.
> 1) Firefox 39.0 - bp-c44f27d7-3137-4ca1-b056-97ed72150721
> 2) Firefox 40.0b- bp-bca36083-37b4-4bff-b0ac-55e9d2150721

So what you mean is that you're still crashing with the same steps as originally reported?
The crash has moved somewhere else, now that the line-break code is no longer the first point of failure.
It's frustrating to see so many moz_xmalloc's where the return value is null-checked. The latest one is nsClipboard::GetGlobalData in bp-bca36083-37b4-4bff-b0ac-55e9d2150721. I suspect that these codepaths were originally null-safe but ended up with moz_xmalloc through refactoring.

Ehsan, how hard would it be to write static analysis to catch these cases?
Flags: needinfo?(ehsan)
You mean checking places where we null check the result of an infallible allocation?  It is possible to detect, yes, but we do that all over the tree, so I'm afraid we'd only see the forest then, not the trees that you're looking for...
Flags: needinfo?(ehsan)
Well that sounds like a good opportunity for cleanup, independent of this bug.
I agree.  I just don't want to sign up to do that work.  :-)  Please feel free to file a bug, it's at least a good idea to keep it tracked.
See Also: → 1186212
Attachment #8636860 - Flags: review?(netzen)
Comment on attachment 8636860 [details] [diff] [review]
Use more fallible allocations

Review of attachment 8636860 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Please either re-ope this bug or put into a new follow up bug.
Attachment #8636860 - Flags: review?(netzen) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla40 → ---
https://hg.mozilla.org/mozilla-central/rev/275301e64d0e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1192040
Is this issue is resolved? If yes, then why it is not mentioned on release notes of 42.0?
(In reply to Xowia Technologies from comment #30)
> Is this issue is resolved?

It looks like the most recent patch landed in Firefox 42, based on comment 29.

> If yes, then why it is not mentioned on release notes of 42.0?

The release notes are a very small subset of things that have been fixed in a given release.
Moving from Core::Untriaged to Core::General https://bugzilla.mozilla.org/show_bug.cgi?id=1407598
Component: Untriaged → General
You need to log in before you can comment on or make changes to this bug.