[GStreamer] Protect against null samples and samples with null buffers

• Component: Media
• Status: RESOLVED FIXED

[Enrique Ocaña]

Under some circumstances (eg: after flushCurrentBuffer()), the (GstSample)m_sample stored by MediaPlayerPrivateGStreamerBase can contain a null GstBuffer. The GstVideoFrameHolder code and the MediaPlayerPrivateGStreamerBase::pushTextureToCompositor(), copyVideoTextureToPlatformTexture() and nativeImageForCurrentTime() methods don't properly manage that scenario, triggering crashes.

[Enrique Ocaña]

Created attachment 373717 [details]
Patch

[Philippe Normand]

Comment on attachment 373717 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:141
> -        if (UNLIKELY(!getSampleVideoInfo(sample, videoInfo)))
> +        if (UNLIKELY(!(sample && getSampleVideoInfo(sample, videoInfo))))

Under which circumstances the sample can be null here? I don't see how it could happen because GstVideoFrameHolder's are created only when GST_IS_SAMPLE(m_sample.get()) in the player. So we could actually add a RELEASE_ASSERT(GST_IS_SAMPLE(sample)) in the holder constructor.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:696
> +    if (!(GST_IS_SAMPLE(m_sample.get()) && gst_sample_get_buffer(m_sample.get())))

Why add this? The FrameHolder ctor already has similar code:

        m_buffer = gst_sample_get_buffer(sample);
        if (UNLIKELY(!GST_IS_BUFFER(m_buffer)))
            return;

[Enrique Ocaña]

Comment on attachment 373717 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:141
>> +        if (UNLIKELY(!(sample && getSampleVideoInfo(sample, videoInfo))))
> 
> Under which circumstances the sample can be null here? I don't see how it could happen because GstVideoFrameHolder's are created only when GST_IS_SAMPLE(m_sample.get()) in the player. So we could actually add a RELEASE_ASSERT(GST_IS_SAMPLE(sample)) in the holder constructor.

Yes, you're right, this can't happen.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:696
>> +    if (!(GST_IS_SAMPLE(m_sample.get()) && gst_sample_get_buffer(m_sample.get())))
> 
> Why add this? The FrameHolder ctor already has similar code:
> 
>         m_buffer = gst_sample_get_buffer(sample);
>         if (UNLIKELY(!GST_IS_BUFFER(m_buffer)))
>             return;

Right, it's redundant and can be removed.

[Enrique Ocaña]

Created attachment 373830 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373830 [details]
Patch

Clearing flags on attachment: 373830

Committed r247298: <https://trac.webkit.org/changeset/247298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52891241>