Open Bug 898006 Opened 8 years ago Updated 4 years ago

don't copy subarrays during DestroySubtree

Categories

(Core :: IPC, defect, P3)

defect

Tracking

()

People

(Reporter: froydnj, Unassigned)

Details

Attachments

(1 file)

No description provided.
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.
Attachment #781032 - Flags: review?(bent.mozilla)
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 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.
Attachment #781032 - Flags: review?(bent.mozilla) → review-
(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.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.