Bug 967167 (ipc-big-arrays)

Faulty: MOZ_CRASH under mozilla::dom::PBrowserParent::Read as we receive a too-large nsTArray<PBlobParent*> length

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: Ehsan)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8369614 [details] [diff] [review]
Faulty session

Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067
This is the same class of bug as bug 967184
(Assignee)

Comment 2

5 years ago
We should just lower array types to FallibleTArray.  Do you agree?

(I might want to take a stab at writing a patch.)
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 3

5 years ago
Created attachment 8369770 [details] [diff] [review]
WIP

OK this is gonna be a huge patch.  Asking feedback on the general approach before I spend more time making this build.  It currently won't build anywhere.
Attachment #8369770 - Flags: feedback?(bent.mozilla)
Comment on attachment 8369770 [details] [diff] [review]
WIP

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

Hrm, this isn't really what I had in mind.

I was thinking we could change IPDL to make the Read() functions use FallibleTArray always. We'd check allocation failures and return false if anything was ever too big. But then we'd cast to (const) InfallibleTArray before passing those arrays to the subclasses' RecvFoo() functions. And we'd only need to do all those gymnastics on the parent side.

Does that make sense? Hopefully we don't have to change any existing function signatures besides IPDL-internal Read()'s.
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 5

5 years ago
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #4)
> I was thinking we could change IPDL to make the Read() functions use
> FallibleTArray always. We'd check allocation failures and return false if
> anything was ever too big. But then we'd cast to (const) InfallibleTArray
> before passing those arrays to the subclasses' RecvFoo() functions. And we'd
> only need to do all those gymnastics on the parent side.

Hmm, I see.  Casting things like that is technically incorrect since FallibeTArray and InfallibleTArray are not related (although they are currently binary compatible, but that may change.)  However, the common practice is to create an InfallibleTArray and SwapElements() it with the fallible array, which is super fast.  That will make the patch a lot easier since it will mean that these changes won't leak to the outside world.

However I don't think that it makes a lot of sense to do something different in the parent process compared to the child.  It should still be OK to do a fallible allocation on both sides, and propagate the failure, since the Read() function can fail for other reasons so the other side has to be able to deal with that.  If you still want me to do that then I will, but I'm not sure what we would gain from that additional work.
(Assignee)

Updated

5 years ago
Flags: needinfo?(bent.mozilla)
(Assignee)

Updated

5 years ago
Depends on: 967297

Comment 6

5 years ago
Would it make this easier to stop having separate fallible and infallible tarray types? I would actually much prefer us to have a single type and do the thing we're doing with the string classes:

array.Append(foo) // infallible append
array.Append(foo, fallible_t()) // fallible append

All the templating mess we currently have makes it hard to audit fallible uses anyway.

Please ignore me if I'm creating more work than I'm saving.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #5)
> However I don't think that it makes a lot of sense to do something different
> in the parent process compared to the child.

The result would be the same though. A failed Read() call triggers a forced crash of the child, just as a failed malloc would. Let's do whichever!

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Would it make this easier to stop having separate fallible and infallible
> tarray types?

That sounds nice too. No objections here, though I don't think it's needed to fix this bug.
Flags: needinfo?(bent.mozilla)
Oh, I forgot. SwapElements sounds fine. Simple, technically correct, and effective.
(Reporter)

Updated

5 years ago
Alias: ipc-big-arrays
(Reporter)

Updated

5 years ago
Duplicate of this bug: 967184
(Reporter)

Updated

5 years ago
Duplicate of this bug: 967522
(Assignee)

Comment 11

5 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Would it make this easier to stop having separate fallible and infallible
> tarray types?

OMGYESPLEASEPRETTYPLEASE!

It would also be really nice to not have *three* different distinct types (nsTArray, FallibleTArray, and InfallibleTArray.)

> I would actually much prefer us to have a single type and do
> the thing we're doing with the string classes:
> 
> array.Append(foo) // infallible append
> array.Append(foo, fallible_t()) // fallible append
> 
> All the templating mess we currently have makes it hard to audit fallible
> uses anyway.
> 
> Please ignore me if I'm creating more work than I'm saving.

I am not volunteering to do the work, but filed bug 968520 for it.  At any rate, like Ben said, that is not necessary for this bug.
(Assignee)

Updated

5 years ago
Assignee: nobody → ehsan
(Assignee)

Updated

5 years ago
Attachment #8369770 - Attachment is obsolete: true
Attachment #8369770 - Flags: feedback?(bent.mozilla)
(Assignee)

Comment 12

5 years ago
Created attachment 8371146 [details] [diff] [review]
Use fallible memory allocation when reading IPDL array types to make sure that we don't OOM for very large arrays; r=bent
(Assignee)

Comment 13

5 years ago
Comment on attachment 8371146 [details] [diff] [review]
Use fallible memory allocation when reading IPDL array types to make sure that we don't OOM for very large arrays; r=bent

This is what the generated code looks like with this patch:

2831 auto PBrowserParent::Read(
2832         InfallibleTArray<CpowEntry>* __v,
2833         const Message* __msg,
2834         void** __iter) -> bool
2835 {
2836     FallibleTArray<CpowEntry> fa;
2837     uint32_t length;
2838     if ((!(Read((&(length)), __msg, __iter)))) {
2839         FatalError("Error deserializing 'length' (uint32_t) of 'CpowEntry[]'");
2840         return false;
2841     }
2842
2843     if ((!((fa).SetLength(length)))) {
2844         FatalError("Error setting the array length");
2845         return false;
2846     }
2847     for (uint32_t i = 0; (i) < (length); (++(i))) {
2848         if ((!(Read((&(fa[i])), __msg, __iter)))) {
2849             FatalError("Error deserializing 'CpowEntry[i]'");
2850             return false;
2851         }
2852     }
2853     (__v)->SwapElements(fa);
2854     return true;
2855 }
Attachment #8371146 - Flags: review?(bent.mozilla)
Comment on attachment 8371146 [details] [diff] [review]
Use fallible memory allocation when reading IPDL array types to make sure that we don't OOM for very large arrays; r=bent

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

Looks great! Thanks.
Attachment #8371146 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/160e1cbe2fcf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.