967167 - (ipc-big-arrays) Faulty: MOZ_CRASH under mozilla::dom::PBrowserParent::Read as we receive a too-large nsTArray<PBlobParent*> length

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Faulty session

Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This is the same class of bug as bug 967184

Comment 2

[(no longer active)]

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

(I might want to take a stab at writing a patch.)

Comment 3

[(no longer active)]

Attachment: 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.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 5

[(no longer active)]

(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.

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, I forgot. SwapElements sounds fine. Simple, technically correct, and effective.

Comment 11

[(no longer active)]

(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.

Comment 12

[(no longer active)]

Attachment: Use fallible memory allocation when reading IPDL array types to make sure that we don't OOM for very large arrays; r=bent

Comment 13

[(no longer active)]

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 }

Comment 14

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=7d305b8f6b53

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 16

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/160e1cbe2fcf

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/160e1cbe2fcf