Bug 199619 - [GStreamer] Protect against null samples and samples with null buffers
Summary: [GStreamer] Protect against null samples and samples with null buffers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-09 07:26 PDT by Enrique Ocaña
Modified: 2019-07-10 07:54 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.13 KB, patch)
2019-07-09 07:42 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (2.58 KB, patch)
2019-07-10 05:30 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2019-07-09 07:26:07 PDT
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.
Comment 1 Enrique Ocaña 2019-07-09 07:42:07 PDT
Created attachment 373717 [details]
Patch
Comment 2 Philippe Normand 2019-07-09 08:55:42 PDT
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;
Comment 3 Enrique Ocaña 2019-07-10 05:10:38 PDT
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.
Comment 4 Enrique Ocaña 2019-07-10 05:30:39 PDT
Created attachment 373830 [details]
Patch
Comment 5 WebKit Commit Bot 2019-07-10 07:53:57 PDT
Comment on attachment 373830 [details]
Patch

Clearing flags on attachment: 373830

Committed r247298: <https://trac.webkit.org/changeset/247298>
Comment 6 WebKit Commit Bot 2019-07-10 07:53:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-07-10 07:54:16 PDT
<rdar://problem/52891241>