Missing OOM crash annotation in IPC::Channel::ChannelImpl::ProcessIncomingMessages

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
In this memcpy:
          buf = (char*)malloc(len);
          memcpy(buf, p, len);

I suspect that this crash happens if malloc fails and returns NULL. It seems like this case is for smaller allocations, so maybe the malloc() can be replaced with moz_xmalloc(). There's a version of this code in both ipc_channel_win.cc and ipc_channel_posix.cc. I read over the rest of the patch in bug 1235633 and did not see any other such issues (aside from the realloc one which has been fixed.)

https://crash-stats.mozilla.com/report/index/e22b99b5-2160-4322-82c7-114f72160420
(Assignee)

Comment 1

3 years ago
Created attachment 8744085 [details] [diff] [review]
OOM crash if malloc fails in ProcessIncomingMessages().

This allocation is for no more than 32kb, so make it infallible rather than try to recover.
Attachment #8744085 - Flags: review?(wmccloskey)
Comment on attachment 8744085 [details] [diff] [review]
OOM crash if malloc fails in ProcessIncomingMessages().

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

Thanks.
Attachment #8744085 - Flags: review?(wmccloskey) → review+

Comment 4

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98b498a402eb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 5

3 years ago
Comment on attachment 8744085 [details] [diff] [review]
OOM crash if malloc fails in ProcessIncomingMessages().

Approval Request Comment
[Feature/regressing bug #]: bug 1235633
[User impact if declined]: some crashes won't be annotated as OOM crashes
[Describe test coverage new/current, TreeHerder]: this code is tested, but not the crashing
[Risks and why]: very low, it just makes an allocation infallible
[String/UUID change made/needed]: none
Attachment #8744085 - Flags: approval-mozilla-aurora?

Comment 6

3 years ago
Comment on attachment 8744085 [details] [diff] [review]
OOM crash if malloc fails in ProcessIncomingMessages().

Correctly annotate said crashes as OOM crashes, Aurora47+
Attachment #8744085 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 7

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ef6c6539b96
status-firefox47: affected → fixed
(Assignee)

Updated

3 years ago
Summary: Null deref crash in IPC::Channel::ChannelImpl::ProcessIncomingMessages → Missing OOM crash annotation in IPC::Channel::ChannelImpl::ProcessIncomingMessages
You need to log in before you can comment on or make changes to this bug.