After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762516 - qtdemux: cenc auxiliary info parsing crashes
qtdemux: cenc auxiliary info parsing crashes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal blocker
: 1.6.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
1.6.4
Depends on:
Blocks:
 
 
Reported: 2016-02-23 09:00 UTC by Sebastian Dröge (slomo)
Modified: 2016-04-14 17:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: don't push encrypted buffer without cenc metadata (1.28 KB, patch)
2016-02-24 08:40 UTC, Philippe Normand
committed Details | Review
qtdemux: prevent buffer flow if any stream failed to be exposed (3.72 KB, patch)
2016-02-24 10:35 UTC, Philippe Normand
committed Details | Review
qtdemux: cenc aux info parsing from mdat support in PULL mode (2.41 KB, patch)
2016-02-25 10:38 UTC, Philippe Normand
committed Details | Review

Description Sebastian Dröge (slomo) 2016-02-23 09:00:17 UTC
+++ This bug was initially created as a clone of Bug #755614 +++

+++ This bug was initially created as a clone of Bug #705991 +++

https://github.com/youtube/js_mse_eme/blob/master/media/oops_cenc-20121114-145-no-clear-start.mp4

Crashes when played as a local file but not over HTTP:


Program received signal SIGSEGV, Segmentation fault.

Thread 140737202661120 (LWP 745)

  • #0 gst_qtdemux_decorate_and_push_buffer
    at qtdemux.c line 4970
  • #1 gst_qtdemux_loop
    at qtdemux.c line 5235
  • #2 gst_qtdemux_loop
    at qtdemux.c line 5311
  • #3 gst_task_func
    at gsttask.c line 332
  • #4 g_thread_pool_thread_proxy
    at /build/glib2.0-rHXeJh/glib2.0-2.47.6/./glib/gthreadpool.c line 307
  • #5 g_thread_proxy
    at /build/glib2.0-rHXeJh/glib2.0-2.47.6/./glib/gthread.c line 780
  • #6 start_thread
    at pthread_create.c line 333
  • #7 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 109

Comment 1 Philippe Normand 2016-02-24 08:02:19 UTC
I'm preparing a patch for this issue :)
The problem is that the cenc metadata is stored outside of the moof box and we try to push a buffer before the metadata has been parsed.
Comment 2 Philippe Normand 2016-02-24 08:40:41 UTC
Created attachment 322211 [details] [review]
qtdemux: don't push encrypted buffer without cenc metadata

When the cenc metadata is stored outside of the moof box and the
stream is exposed it is possible that the cenc metadata hasn't been
processed yet while the first buffer is being pushed. When this
happens the buffer can't possibly be decrypted downstream so don't
push it.
Comment 3 Sebastian Dröge (slomo) 2016-02-24 08:54:27 UTC
Shouldn't the cenc metadata come in any case before the first actual data that has to be decrypted? And if not, would it make sense to queue up buffers until it is available?
Comment 4 Philippe Normand 2016-02-24 09:15:36 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> Shouldn't the cenc metadata come in any case before the first actual data
> that has to be decrypted? And if not, would it make sense to queue up
> buffers until it is available?

When the file is accessed through HTTP the metadata is parsed before the first buffer has to be pushed. But this is not the case if the file is local. Perhaps this is related with how the src element operates? I wonder :)

To answer your question, yeah, it might be good indeed to queue buffers until the metadata is parsed, I'll revise the patch.
Comment 5 Sebastian Dröge (slomo) 2016-02-24 09:47:22 UTC
That sounds like a problem. So you're saying this only happens in PULL mode? Seems like it does things wrong then :) Especially in PULL mode it should be able to collect all headers before the buffers.
Comment 6 Philippe Normand 2016-02-24 09:57:06 UTC
Seems so yes, if I use pushfilesrc instead of filesrc it doesn't crash.
Comment 7 Philippe Normand 2016-02-24 10:09:46 UTC
I wonder why we try to push a buffer anyway, even after 4 errors were raised:

0:00:00.292522760   983 0x75e01c00 ERROR                qtdemux qtdemux.c:7212:gst_qtdemux_configure_protected_caps:<qtdemux0> stream is protected, but no suitable decryptor element has been found
0:00:00.292721510   983 0x75e01c00 ERROR                qtdemux qtdemux.c:7364:gst_qtdemux_configure_stream:<qtdemux0> Failed to configure protected stream caps.
...
0:00:00.297545573   983 0x75e01c00 ERROR                qtdemux qtdemux.c:7212:gst_qtdemux_configure_protected_caps:<qtdemux0> stream is protected, but no suitable decryptor element has been found
0:00:00.297735625   983 0x75e01c00 ERROR                qtdemux qtdemux.c:7364:gst_qtdemux_configure_stream:<qtdemux0> Failed to configure protected stream caps.

Shouldn't the demuxer abort if no decryptor was found? In that case the demuxer is pretty much unlinked anyway.
Comment 8 Sebastian Dröge (slomo) 2016-02-24 10:25:15 UTC
I think it should just fail then, yes. But that's independent of this bug, or not?
Comment 9 Philippe Normand 2016-02-24 10:29:05 UTC
Yes.

So I think we need two patches:

- one checking for NULL crypto_info pointer to prevent any possible crash in this part of the code. Maybe we can assert in this case?
- another change making qtdemux_expose_streams() return GST_FLOW_ERROR if any of the streams couldn't be added.
Comment 10 Philippe Normand 2016-02-24 10:35:14 UTC
Created attachment 322224 [details] [review]
qtdemux: prevent buffer flow if any stream failed to be exposed

In some cases the stream configuration can fail, for instance if the
stream is protected and no decryptor was found. For those situations
the demuxer shouldn't emit any data on the corresponding source pad of
the stream and bail out.
Comment 11 Sebastian Dröge (slomo) 2016-02-24 10:41:11 UTC
And understanding why it does that (reading buffers before reading the headers) in PULL mode but not PUSH mode?
Comment 12 Philippe Normand 2016-02-24 11:28:07 UTC
That's going to take time for me. Debugging qtdemux is not so much fun, but I'll compare the logs between both modes, anyway :)
Comment 13 Philippe Normand 2016-02-24 14:34:48 UTC
Ok I think I understand this a bit better now. Part of the solution would be to parse the cenc data from the loop_state_movie() function (PULL mode), like it's done in process_adapter() in PUSH mode.
Comment 14 Sebastian Dröge (slomo) 2016-02-24 15:15:27 UTC
Can you provide a patch for that too?
Comment 15 Philippe Normand 2016-02-24 15:19:31 UTC
Yes, I hope so :)
Comment 16 Sebastian Dröge (slomo) 2016-02-25 09:41:40 UTC
Great, thanks for looking into this :)
Comment 17 Philippe Normand 2016-02-25 10:38:01 UTC
Created attachment 322348 [details] [review]
qtdemux: cenc aux info parsing from mdat support in PULL mode

This is already supported for PUSH mode but was failing in PULL mode.
The aux info is sometimes stored in the mdat before the first sample,
so the loop task needs to pull data stored at that location and
perform the aux info cenc parsing.

https://bugzilla.gnome.org/show_bug.cgi?id=761700
Comment 18 Sebastian Dröge (slomo) 2016-02-25 10:48:23 UTC
Attachment 322211 [details] pushed as fb5d50c - qtdemux: don't push encrypted buffer without cenc metadata
Attachment 322224 [details] pushed as 67f3fc1 - qtdemux: prevent buffer flow if any stream failed to be exposed
Attachment 322348 [details] pushed as 9c47c0d - qtdemux: cenc aux info parsing from mdat support in PULL mode