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 770951 - qtdemux: Crash with no cenc auxiliary offset available
qtdemux: Crash with no cenc auxiliary offset available
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.9.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-06 14:01 UTC by Xabier Rodríguez Calvar
Modified: 2016-09-30 07:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
1/1 gst-plugins-good patch (868 bytes, patch)
2016-09-06 14:01 UTC, Xabier Rodríguez Calvar
committed Details | Review
2/2 gst-plugins-good patch (963 bytes, patch)
2016-09-09 15:34 UTC, Xabier Rodríguez Calvar
committed Details | Review

Description Xabier Rodríguez Calvar 2016-09-06 14:01:33 UTC
Created attachment 334908 [details] [review]
1/1 gst-plugins-good patch

This bug is a side effect of bug 770107 when running http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2016.html?enablewebm=false&command=run&test_type=encryptedmedia-test&tests=16,17 . As we don't have a key at the moment this code is run, crypto_info is null and we don't have any cenc aux offset, which causes a crash some lines later when retrieving the index.
Comment 1 Sebastian Dröge (slomo) 2016-09-07 06:58:44 UTC
commit 92075e0256584b3ce22ff00d4c7a835ab296ddb0
Author: Xabier Rodriguez Calvar <calvaris@igalia.com>
Date:   Tue Sep 6 09:49:39 2016 +0200

    qtdemux: Fix crash with no cenc aux offset
    
    https://bugzilla.gnome.org/show_bug.cgi?id=770951
Comment 2 Philippe Normand 2016-09-07 07:02:11 UTC
I hope someone checked the use-case of #762516 still works :)
Comment 3 Xabier Rodríguez Calvar 2016-09-07 14:41:20 UTC
(In reply to Philippe Normand from comment #2)
> I hope someone checked the use-case of #762516 still works :)

This still works but I think it might be causing issues, before release 1.9.3, this should be reverted.
Comment 4 Xabier Rodríguez Calvar 2016-09-07 14:41:44 UTC
(In reply to Xabier Rodríguez Calvar from comment #3)
> This still works but I think it might be causing issues, before release
> 1.9.3, this should be reverted.

Maybe no rush so far, but let's keep this in mind.
Comment 5 Sebastian Dröge (slomo) 2016-09-07 15:07:01 UTC
What's the problem?
Comment 6 Xabier Rodríguez Calvar 2016-09-07 15:12:16 UTC
(In reply to Sebastian Dröge (slomo) from comment #5)
> What's the problem?

One of our customers says that something is crashing with that patch and it doesn't without it. I couldn't investigate it so I cannot provide more info atm.
Comment 7 Xabier Rodríguez Calvar 2016-09-09 15:34:12 UTC
Created attachment 335204 [details] [review]
2/2 gst-plugins-good patch

This patch applies on top of the other one so that we don't have to revert it.

I have no deep knowledge about parsing these formats but from what I see the code after this guarded if makes no sense and it will crash if info->crypt_info is null, regardless of the offset. Please correct me if I am wrong.

The tests I made run successfully now.
Comment 8 Sebastian Dröge (slomo) 2016-09-10 08:30:28 UTC
Makes sense, the offset should indeed not matter and an offset of 0 also seems
to be valid even.

commit 415ae458d27ae4be5cf2dffaef9b496bef3942b8
Author: Xabier Rodriguez Calvar <calvaris@igalia.com>
Date:   Fri Sep 9 14:02:25 2016 +0200

    qtdemux: offset is irrelevant when no crypto info
    
    Cause later it will try to use the crypto info array to get an index and
    attach on of the positions as buffer's crypto info.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=770951