Closed Bug 967167 (ipc-big-arrays) Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: IPC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bjacob, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch Faulty sessionSplinter Review
Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067
This is the same class of bug as bug 967184
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)
Attached patch WIP (obsolete) — Splinter Review
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)
(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.
Flags: needinfo?(bent.mozilla)
Depends on: 967297
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.
Alias: ipc-big-arrays
Duplicate of this bug: 967184
Duplicate of this bug: 967522
(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: nobody → ehsan
Attachment #8369770 - Attachment is obsolete: true
Attachment #8369770 - Flags: feedback?(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

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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.