844996 - DeallocShmem failures should always abort

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Currently if there's a DeallocShmem failure it is ignored in OMTC configurations.

I'll be sending this to try soon. It will be interesting to see what this finds.

Comment 1

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=1a32034f781a

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 718019 [details] [diff] [review]
patch

More consistent with what we do with actor serialization.

No point in returning bool from this method anymore.  Bonus points for removing that and seeing if any callers actually checked it, but I'm fine with that in a followup too.

Comment 3

[Benoit Girard (:BenWa)]

This actually catches problem on mochitest so we're going to have to wait until they're fixed to land this :(.

Comment 4

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=4b2c7faa2dd5

Comment 5

[Benjamin Smedberg]

Comment on attachment 718019 [details] [diff] [review]
patch

Can a child ever force a parent to abort by sending a bad shmem like this? For plugins in the past we've tried to make sure that aggressive aborting like this happened in the child but not the parent.

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Can a child ever force a parent to abort by sending a bad shmem like this?
> For plugins in the past we've tried to make sure that aggressive aborting
> like this happened in the child but not the parent.

AIUI this map keeps track of any Shmem over that channel so yes the child could leak a shmem intentionally to force this behavior.

Comment 7

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=435caf66bed3

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Comment on attachment 718019 [details] [diff] [review]
> patch
> 
> Can a child ever force a parent to abort by sending a bad shmem like this?
> For plugins in the past we've tried to make sure that aggressive aborting
> like this happened in the child but not the parent.

This patch is ready to land. Should I modify how it works or land it?

Comment 9

[Benjamin Smedberg]

How likely is failure, and under what conditions? Unless I misunderstand, I don't think the premise of the bug is correct. DeallocShmem failure should not abort in Firefox processes. It is possible that it should abort in plugin processes, but I'm not even sure about that. DeallocShmem failure doesn't mean subsequent actions could result in memory corruption or security bugs, right?

Comment 10

[Benoit Girard (:BenWa)]

Not directly. If we try to release something as a shmem that isn't a shmem then our code is doing something wrong. This is more akind of a debug assert. Do you prefer that I make this a debug only check?

Comment 11

[Benjamin Smedberg]

Yes, I think so.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch

Generated code:

bool
PCompositorParent::DeallocShmem(Shmem& aMem)
{
    bool ok = DestroySharedMemory(aMem);
#ifdef DEBUG
    if ((!(ok))) {
        NS_RUNTIMEABORT("bad Shmem");
    }    
#endif // DEBUG
    (aMem).forget(Shmem::IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead());
    return ok;
}

Comment 13

[Benjamin Smedberg]

Comment on attachment 730282 [details] [diff] [review]
patch

r=me, although I think MOZ_ASSERT would probably do the same thing.

Comment 14

[Benoit Girard (:BenWa)]

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

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50725df22b05

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50725df22b05