999697 - Faulty: Assertion failure: false (should be implemented or not used) in CompositableHost::CreatedIncrementalTexture

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Attachment: Faulty session

Found while fuzzing.

Comment 1

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

Ouch, this is another "PReinterpretCast" type bug.

The parent stack frame is in CompositableParentManager::ReceiveCompositableUpdate, and is doing:

bool
CompositableParentManager::ReceiveCompositableUpdate(const CompositableOperation& aEdit,
                                                     EditReplyVector& replyv)
{
  switch (aEdit.type()) {
    case CompositableOperation::TOpCreatedIncrementalTexture: {
      MOZ_LAYERS_LOG(("[ParentSide] Created texture"));
      const OpCreatedIncrementalTexture& op = aEdit.get_OpCreatedIncrementalTexture();

      CompositableParent* compositableParent =
        static_cast<CompositableParent*>(op.compositableParent());
      CompositableHost* compositable = compositableParent->GetCompositableHost();

      compositable->CreatedIncrementalTexture(compositableParent->GetCompositableManager(),
                                              op.textureInfo(),
                                              op.bufferRect());
      break;
    }

The CreatedIncrementalTexture above is the one crashing with this "unimplemented" assert.


From the log:

 2:51.30 [Faulty] pickle field {int} of value: 3 changed to: 2


From LayersMessages.h in the objdir:

class CompositableOperation MOZ_FINAL
{
public:
    enum Type {
        T__None,
        TOpUpdatePictureRect = 1,
        TOpCreatedIncrementalTexture,
        TOpPaintTextureRegion,

So "3 changed to 2" means: TOpPaintTextureRegion changed to TOpCreatedIncrementalTexture.

So we are, again, using an enum value from an untrusted IPC message, to decide how to reinterpret an existing object...

I'm removing the sec-critical keyword for now because I need some time to actually think about the security implications. Not fully clear to me here yet if the virtual call here is unsafe.

Comment 2

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

OK, this is not actually a PReinterpretCast pattern, and not actually security sensitive.

All what's happening here is that we're calling a virtual method that, when not overridden, crashes. Instead, we just need to make this gracefully fail and propagate an error.

Comment 3

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

Attachment: Make CreatedIncrementalTexture fail gracefully and return bool

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bc878ad761

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3bc878ad761