[EME] Create CDMProxyFactory

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[EME] Create CDMProxyFactory

[Xabier Rodríguez Calvar]

Created attachment 400920 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 400926 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 401004 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 401005 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 401006 [details]
Patch

[Xabier Rodríguez Calvar]

Created attachment 401008 [details]
Patch

[Charlie Turner]

I see this (some of the crashes are racey after apply this). The setProxy call has important timing preconditions which the ctor does not meet.

Regressions: Unexpected crashes (5)
  imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-playback-temporary-waitingforkey.https.html [ Crash ]
  imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-setmediakeys-again-after-resetting-src.https.html [ Crash ]
  imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-syntax-mediakeys.https.html [ Crash ]
  imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-syntax-mediakeysession.https.html [ Crash ]
  media/encrypted-media/clearKey/clearKey-encrypted-cenc-event.html [ Crash ]

[Xabier Rodríguez Calvar]

Created attachment 401527 [details]
Patch

(In reply to Charlie Turner from comment #7)
> I see this (some of the crashes are racey after apply this). The setProxy
> call has important timing preconditions which the ctor does not meet.
> 
> Regressions: Unexpected crashes (5)
>  
> imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-playback-
> temporary-waitingforkey.https.html [ Crash ]
>  
> imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-setmediakeys-
> again-after-resetting-src.https.html [ Crash ]
>  
> imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-syntax-
> mediakeys.https.html [ Crash ]
>  
> imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-syntax-
> mediakeysession.https.html [ Crash ]
>   media/encrypted-media/clearKey/clearKey-encrypted-cenc-event.html [ Crash ]

With the obsoleted patch these tests are flaky but not because of the key stuff, only because we initialize GCrypt too soon. For that I created LazilyInitialized and get that initialize when needed (I had the manual lazy initalization version but writing a class for this seems possibly useful for more people). I'm writing tests for it but I wanted to post it here to get earlier reviews about it.

[Xabier Rodríguez Calvar]

Created attachment 401530 [details]
Patch

Added a test for the LazilyInitialized class.

[Xabier Rodríguez Calvar]

Created attachment 401532 [details]
Patch

This the style issues. The remaining one is a false positive.

[youenn fablet]

Comment on attachment 401532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401532&action=review

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:91
> +        gcry_cipher_close(*m_gCryptHandle);

How about having a unique_ptr or an Optional of a class which would wrap a gcry_cipher_hd_t. Class constructor would call gcry_cipher_open and destructor would call gcry_cipher_close.
You could lazily initialise it in the gCryptHandle() getter that you would use everywhere instead of m_gCryptHandle.

[Xabier Rodríguez Calvar]

(In reply to youenn fablet from comment #11)
> Comment on attachment 401532 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401532&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:91
> > +        gcry_cipher_close(*m_gCryptHandle);
> 
> How about having a unique_ptr or an Optional of a class which would wrap a
> gcry_cipher_hd_t. Class constructor would call gcry_cipher_open and
> destructor would call gcry_cipher_close.
> You could lazily initialise it in the gCryptHandle() getter that you would
> use everywhere instead of m_gCryptHandle.

Class constructor can't call gcry_cipher_open because that creates test flakiness (getting too much unnecessary secure memory) and that's why we have to initialize it lazily. Of course I can initialize it manually with an Optional or std::unique_ptr but I thought that creating a class at WTF for that could be useful for more developers.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #12)
> > > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:91
> > > +        gcry_cipher_close(*m_gCryptHandle);
> > 
> > How about having a unique_ptr or an Optional of a class which would wrap a
> > gcry_cipher_hd_t. Class constructor would call gcry_cipher_open and
> > destructor would call gcry_cipher_close.
> > You could lazily initialise it in the gCryptHandle() getter that you would
> > use everywhere instead of m_gCryptHandle.
> 
> Class constructor can't call gcry_cipher_open because that creates test
> flakiness (getting too much unnecessary secure memory) and that's why we
> have to initialize it lazily. Of course I can initialize it manually with an
> Optional or std::unique_ptr but I thought that creating a class at WTF for
> that could be useful for more developers.

About checking in the destructor for the handle to be initialized, it is because if is not initialized it does not make sense to close it.

[youenn fablet]

(In reply to Xabier Rodríguez Calvar from comment #12)
> (In reply to youenn fablet from comment #11)
> > Comment on attachment 401532 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=401532&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:91
> > > +        gcry_cipher_close(*m_gCryptHandle);
> > 
> > How about having a unique_ptr or an Optional of a class which would wrap a
> > gcry_cipher_hd_t. Class constructor would call gcry_cipher_open and
> > destructor would call gcry_cipher_close.
> > You could lazily initialise it in the gCryptHandle() getter that you would
> > use everywhere instead of m_gCryptHandle.
> 
> Class constructor can't call gcry_cipher_open because that creates test
> flakiness (getting too much unnecessary secure memory) and that's why we
> have to initialize it lazily. Of course I can initialize it manually with an
> Optional or std::unique_ptr but I thought that creating a class at WTF for
> that could be useful for more developers.

I was meaning something like:
class CDMProxyClearKey {
...
Optional<GCryptHandle> m_cipherHandle;
};

gcry_cipher_hd_t& CDMProxyClearKey::cipherHandle()
{
    // Lazily created.
    if (! m_cipherHandle)
        m_handle = GCryptHandle();
    return m_cipherHandle->handle();
}

class GCryptHandle {
 GCryptHandle() // constructor will call gcry_cipher_open
 ~GCryptHandle() // destructor will call gcry_cipher_close
 ...
 gcry_cipher_hd_t m_handle;
};

[Xabier Rodríguez Calvar]

(In reply to youenn fablet from comment #14)
> (In reply to Xabier Rodríguez Calvar from comment #12)
> > (In reply to youenn fablet from comment #11)
> > > Comment on attachment 401532 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=401532&action=review
> > > 
> > > > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:91
> > > > +        gcry_cipher_close(*m_gCryptHandle);
> > > 
> > > How about having a unique_ptr or an Optional of a class which would wrap a
> > > gcry_cipher_hd_t. Class constructor would call gcry_cipher_open and
> > > destructor would call gcry_cipher_close.
> > > You could lazily initialise it in the gCryptHandle() getter that you would
> > > use everywhere instead of m_gCryptHandle.
> > 
> > Class constructor can't call gcry_cipher_open because that creates test
> > flakiness (getting too much unnecessary secure memory) and that's why we
> > have to initialize it lazily. Of course I can initialize it manually with an
> > Optional or std::unique_ptr but I thought that creating a class at WTF for
> > that could be useful for more developers.
> 
> I was meaning something like:
> class CDMProxyClearKey {
> ...
> Optional<GCryptHandle> m_cipherHandle;
> };
> 
> gcry_cipher_hd_t& CDMProxyClearKey::cipherHandle()
> {
>     // Lazily created.
>     if (! m_cipherHandle)
>         m_handle = GCryptHandle();
>     return m_cipherHandle->handle();
> }
> 
> class GCryptHandle {
>  GCryptHandle() // constructor will call gcry_cipher_open
>  ~GCryptHandle() // destructor will call gcry_cipher_close
>  ...
>  gcry_cipher_hd_t m_handle;
> };

To make this, I prefer a more general class that brings already the lazy implementation. Creating a class brings nothing if you just use an Optional or a unique_ptr.

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #15)
> (In reply to youenn fablet from comment #14)
> > (In reply to Xabier Rodríguez Calvar from comment #12)
> > > (In reply to youenn fablet from comment #11)
> > > > Comment on attachment 401532 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=401532&action=review
> > > > 
> > > > > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:91
> > > > > +        gcry_cipher_close(*m_gCryptHandle);
> > > > 
> > > > How about having a unique_ptr or an Optional of a class which would wrap a
> > > > gcry_cipher_hd_t. Class constructor would call gcry_cipher_open and
> > > > destructor would call gcry_cipher_close.
> > > > You could lazily initialise it in the gCryptHandle() getter that you would
> > > > use everywhere instead of m_gCryptHandle.
> > > 
> > > Class constructor can't call gcry_cipher_open because that creates test
> > > flakiness (getting too much unnecessary secure memory) and that's why we
> > > have to initialize it lazily. Of course I can initialize it manually with an
> > > Optional or std::unique_ptr but I thought that creating a class at WTF for
> > > that could be useful for more developers.
> > 
> > I was meaning something like:
> > class CDMProxyClearKey {
> > ...
> > Optional<GCryptHandle> m_cipherHandle;
> > };
> > 
> > gcry_cipher_hd_t& CDMProxyClearKey::cipherHandle()
> > {
> >     // Lazily created.
> >     if (! m_cipherHandle)
> >         m_handle = GCryptHandle();
> >     return m_cipherHandle->handle();
> > }
> > 
> > class GCryptHandle {
> >  GCryptHandle() // constructor will call gcry_cipher_open
> >  ~GCryptHandle() // destructor will call gcry_cipher_close
> >  ...
> >  gcry_cipher_hd_t m_handle;
> > };
> 
> To make this, I prefer a more general class that brings already the lazy
> implementation. Creating a class brings nothing if you just use an Optional
> or a unique_ptr.

Maybe a class that inherits from LazilyInitialized that forces you to have an virtual initialize = 0 method that is automatically called when you access the operator* or operator->

[Xabier Rodríguez Calvar]

Created attachment 401637 [details]
Patch

This is the solution I like most. Added an Optional parameter to the LazilyInitialized class to be able to provide a deinitializer. I think code looks nice and smooth now.

[Xabier Rodríguez Calvar]

Created attachment 401639 [details]
Patch

And now with the test for the deinitializer and Apple build fix (I hope).

[Xabier Rodríguez Calvar]

Created attachment 401713 [details]
Patch

I think this is now ready for final review and landing

[youenn fablet]

Comment on attachment 401713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401713&action=review

> Source/WTF/wtf/LazilyInitialized.h:51
> +        , m_deinitializer(WTFMove(deinitializer)) { }

I am not really convinced by LazilyInitialized.
I think it would be best to introduce it as a separate follow-up patch.
And keep in this patch the usual approach of a class wrapper or a CDMProxyClearKey getter like:
CDMProxyClearKey::cryptHandle()
{
    if (!m_handleInitialized) {
        m_handleInitialized = true;
        initializeGcrypt();
    }
    return m_gcryHandle;
}

> Source/WTF/wtf/LazilyInitialized.h:75
> +        if (!m_isInitialized.exchange(true))

This is probably not thread safe. If two threads call operator*() at the same time, there is a risk that m_value is used by one of the thread without being initialized.

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:43
> +    static NeverDestroyed<Vector<CDMProxyFactory*>> factories;

Maybe this could be a WeakHashSet to prevent UAF.
If this is supposed to be keyed by keySystem, maybe it should be a HashMap?

> Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:52
> +void CDMProxyFactory::registerFactory(CDMProxyFactory& factory)

It seems these methods are not called in this patch.
It would be safer to register/unregister in CDMProxyFactory constructor/destructor if possible.

[Xabier Rodríguez Calvar]

(In reply to youenn fablet from comment #20)
> Comment on attachment 401713 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401713&action=review
> 
> > Source/WTF/wtf/LazilyInitialized.h:51
> > +        , m_deinitializer(WTFMove(deinitializer)) { }
> 
> I am not really convinced by LazilyInitialized.
> I think it would be best to introduce it as a separate follow-up patch.
> And keep in this patch the usual approach of a class wrapper or a
> CDMProxyClearKey getter like:
> CDMProxyClearKey::cryptHandle()
> {
>     if (!m_handleInitialized) {
>         m_handleInitialized = true;
>         initializeGcrypt();
>     }
>     return m_gcryHandle;
> }

If you have a strong opinion I'll do it, but I don't agree. I think this is a good solution.

> > Source/WTF/wtf/LazilyInitialized.h:75
> > +        if (!m_isInitialized.exchange(true))
> 
> This is probably not thread safe. If two threads call operator*() at the
> same time, there is a risk that m_value is used by one of the thread without
> being initialized.

You're right. Maybe we can forget about thread safety and make it just a bool.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:43
> > +    static NeverDestroyed<Vector<CDMProxyFactory*>> factories;
> 
> Maybe this could be a WeakHashSet to prevent UAF.
> If this is supposed to be keyed by keySystem, maybe it should be a HashMap?

It could but I don't follow why it should be a WeahHashSet. Who would own the factories then?

> > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:52
> > +void CDMProxyFactory::registerFactory(CDMProxyFactory& factory)
> 
> It seems these methods are not called in this patch.
> It would be safer to register/unregister in CDMProxyFactory
> constructor/destructor if possible.

They are called for GStreamer.

[Xabier Rodríguez Calvar]

Created attachment 401897 [details]
Patch

(In reply to Xabier Rodríguez Calvar from comment #21)
> > > Source/WTF/wtf/LazilyInitialized.h:51
> > > +        , m_deinitializer(WTFMove(deinitializer)) { }
> > 
> > I am not really convinced by LazilyInitialized.
> > I think it would be best to introduce it as a separate follow-up patch.
> > And keep in this patch the usual approach of a class wrapper or a
> > CDMProxyClearKey getter like:
> > CDMProxyClearKey::cryptHandle()
> > {
> >     if (!m_handleInitialized) {
> >         m_handleInitialized = true;
> >         initializeGcrypt();
> >     }
> >     return m_gcryHandle;
> > }
> 
> If you have a strong opinion I'll do it, but I don't agree. I think this is
> a good solution.

When I think LazilyInitialized is a more elegant solution, I refactored as you wanted.
 
> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:43
> > > +    static NeverDestroyed<Vector<CDMProxyFactory*>> factories;
> > 
> > Maybe this could be a WeakHashSet to prevent UAF.
> > If this is supposed to be keyed by keySystem, maybe it should be a HashMap?
> 
> It could but I don't follow why it should be a WeahHashSet. Who would own
> the factories then?

Finally, this can't be done. Things can't be keyed by key system because a factory can support more than one system (well, it could be but I think the alternative of adding the same factory more than once would be worse than this).

> > > Source/WebCore/platform/encryptedmedia/CDMProxy.cpp:52
> > > +void CDMProxyFactory::registerFactory(CDMProxyFactory& factory)
> > 
> > It seems these methods are not called in this patch.
> > It would be safer to register/unregister in CDMProxyFactory
> > constructor/destructor if possible.
> 
> They are called for GStreamer.

I think now we would be ready for landing if you agree.

[youenn fablet]

Comment on attachment 401897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401897&action=review

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:161
> +    virtual ~CDMProxyFactory() { };

We could add an ASSERT that the factory is not contained in the list of factories.
Or call unregisterFactory.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:169
> +    WEBCORE_EXPORT static void platformRegisterFactories(Vector<CDMProxyFactory*>&);

Should we make it private then?
Maybe rename it to initializeRegisteredFactories().
Also we do not really like out parameters.
We could change it to return a Vector<> that we would move into NeverDestroyed<Vector<CDMProxyFactory*>> factories.

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:186
> +    CDMInstanceProxy(const String& keySystem)

explicit

> Source/WebCore/platform/encryptedmedia/CDMProxy.h:-177
> -            m_cdmProxy->updateKeyStore(m_keyStore);

We remove that code, is it ok?

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:222
> +        m_isGCryptInitialized = true;

This is very similar to an Optional.
I would make m_gCryptHandle an Optional so that we are sure we never use it uninitialised.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.h:48
> +    virtual ~CDMProxyFactoryClearKey();

No need for virtual here.

> Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.h:51
> +    bool supportsKeySystem(const String&) final;

Can we make them private?

[Xabier Rodríguez Calvar]

Created attachment 402003 [details]
Patch

(In reply to youenn fablet from comment #23)
> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:161
> > +    virtual ~CDMProxyFactory() { };
> 
> We could add an ASSERT that the factory is not contained in the list of
> factories.
> Or call unregisterFactory.

Done.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:169
> > +    WEBCORE_EXPORT static void platformRegisterFactories(Vector<CDMProxyFactory*>&);
> 
> Should we make it private then?
> Maybe rename it to initializeRegisteredFactories().
> Also we do not really like out parameters.
> We could change it to return a Vector<> that we would move into
> NeverDestroyed<Vector<CDMProxyFactory*>> factories.

I changed everything but the name. I really think current one is better than initializeRegisteredFactories, since it is platform related and it is actually what registers the factories, they are not registered yet.
 
> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:186
> > +    CDMInstanceProxy(const String& keySystem)
> 
> explicit

Done.

> > Source/WebCore/platform/encryptedmedia/CDMProxy.h:-177
> > -            m_cdmProxy->updateKeyStore(m_keyStore);
> 
> We remove that code, is it ok?

Yes, because this was necessary when we were setting the proxy later. Now that it's created at the same time than the instance, there is no need for this. This was a Charlie's concern as well but the tests he mentions are not flaky at all with this code.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.cpp:222
> > +        m_isGCryptInitialized = true;
> 
> This is very similar to an Optional.
> I would make m_gCryptHandle an Optional so that we are sure we never use it
> uninitialised.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.h:48
> > +    virtual ~CDMProxyFactoryClearKey();
> 
> No need for virtual here.

Done.

> > Source/WebCore/platform/graphics/gstreamer/eme/CDMProxyClearKey.h:51
> > +    bool supportsKeySystem(const String&) final;
> 
> Can we make them private?

Done.

[EWS]

Committed r263087: <https://trac.webkit.org/changeset/263087>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402003 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64407032>

[Radar WebKit Bug Importer]

<rdar://problem/64407033>