898006 - don't copy subarrays during DestroySubtree

Status: NEW

(Core :: IPC, defect, P3)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: don't copy subarrays during DestroySubtree

We do:

    InfallibleTArray<PBlobParent*>& kids = mManagedPBlobParent;

in DeallocSubtree but for whatever reason do:

    InfallibleTArray<PBlobParent*> kids(mManagedPBlobParent);

in DestroySubtree.  AFAICS, the copying isn't buying us anything.  Fixing
DeallocSubtree to use references reduces code size by ~10k or so on ARM.

Light mochitest testing on Linux and OS X says this patch is good.  If you have
other suggested tests to run, I'd be happy to hear about them.

Comment 2

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

Well, DestroySubtree could call other IPC things that could mutate the array, right? In which case our code would walk over the end of the array.

I guess theoretically DeallocSubtree could too but since that's supposed to just do memory stuff I'd call that a problem with the caller.

Comment 3

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

Comment on attachment 781032 [details] [diff] [review]
don't copy subarrays during DestroySubtree

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

I don't think this is safe as currently written.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Ben Turner (not reading bugmail, use the needinfo flag!) from comment #2)
> Well, DestroySubtree could call other IPC things that could mutate the
> array, right? In which case our code would walk over the end of the array.

For what it's worth, there's already a footgun there — if the ActorDestroy callback destroys another actor later in the subtree being destroyed, and its Dealloc routine deletes it, then that causes use-after-free in DestroySubtree via the pointer in the copied array.  See bug 1202887 comment #2 for an example.

> I guess theoretically DeallocSubtree could too but since that's supposed to
> just do memory stuff I'd call that a problem with the caller.

I wouldn't make too many assumptions about that — even if the Dealloc routine is just `delete aActor;` it still calls destructors.