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 722567 - wavparse: loops on incorrect wav file
wavparse: loops on incorrect wav file
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.2.2
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-19 21:01 UTC by Richard Röjfors
Modified: 2015-04-07 11:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to flush at least 8 bytes. (627 bytes, patch)
2014-01-19 21:01 UTC, Richard Röjfors
reviewed Details | Review
Corrupt wav file, with valid riff, fmt and data chunks (1.01 MB, application/octet-stream)
2014-03-18 08:48 UTC, Richard Röjfors
  Details
clip chunk length to available data size (1.40 KB, patch)
2015-03-20 09:11 UTC, Vincent Penquerc'h
committed Details | Review
clip chunk size to valid maximum (1.07 KB, patch)
2015-03-20 12:20 UTC, Vincent Penquerc'h
none Details | Review
clip chunk size to valid maximum (1.12 KB, patch)
2015-03-20 12:25 UTC, Vincent Penquerc'h
committed Details | Review

Description Richard Röjfors 2014-01-19 21:01:00 UTC
Created attachment 266677 [details] [review]
Patch to flush at least 8 bytes.

I had a wave file with a correct RIFF chunk but contained some padded garbage in the end.

Depending on the data it can cause gst_waveparse_ignore_chunk to flush <= bytes.
In my case the parser started to loop and flushed 0 bytes.

The attached patch works around this by flushing at least 8 bytes.
Comment 1 Sebastian Dröge (slomo) 2014-01-20 09:09:03 UTC
Do you have a test file for this? If this happens it means that we have an overflow, I think it's better to check for size to have a sensible value. Especially that it is smaller than 2GB, as that's the maximum allowed size for RIFF chunks... and error out directly if it's larger than that.
Comment 2 Richard Röjfors 2014-01-20 09:15:38 UTC
I'll dig up a test file.

The RIFF chunk was OK. I think there is "just" some scrambled garbage padded to the end of the file. The parser tries to parse the random data as a chunk, the type is unknown and it just tries to skip. The random data happen to result in a bad size, which causes the flush amount to be 0.

Since it has a valid RIFF chunk I think it would be good not to "error out", but rather just skip parsing the rest of the file.
Comment 3 Sebastian Dröge (slomo) 2014-01-20 09:18:29 UTC
Ah it happens after the valid data chunk? It might be good to just EOS then instead of erroring out. If it's before the data chunk, erroring out is better IMHO.
Comment 4 Richard Röjfors 2014-03-18 08:48:14 UTC
Created attachment 272250 [details]
Corrupt wav file, with valid riff, fmt and data chunks
Comment 5 Richard Röjfors 2014-03-25 08:00:35 UTC
Of some reason I can't change the status from NEEDINFO to something else than RESOLVED, maybe someone else can do that? I guess it should go back to NEW until someone picks it up?
Comment 6 Tim-Philipp Müller 2014-03-25 10:21:24 UTC
Sure, thanks for the test file.
Comment 7 Vincent Penquerc'h 2015-03-20 09:11:07 UTC
Created attachment 299929 [details] [review]
clip chunk length to available data size

This patch does that. However, I think it'd be a good idea to also use Richard's patch for double checking sanity, and for cases where the upstream size is not known.

The "bad" chunk length on that file was 0xfffffff8.
Comment 8 Vincent Penquerc'h 2015-03-20 09:16:15 UTC
> Especially that it is smaller than 2GB, as that's the maximum allowed size

I'd missed that in fact. The size variables are all (or at least, mostly) guint32, which hinted 4 GB was possible. If it's not, then it makes the issue easier to fix...
Comment 9 Vincent Penquerc'h 2015-03-20 12:20:53 UTC
Created attachment 299941 [details] [review]
clip chunk size to valid maximum

Also checks that 2 GB limit.

pushfile:// (unknown upsrteam size) works fine, it doesn't seem to be upset at this wrong chunk size.
Comment 10 Vincent Penquerc'h 2015-03-20 12:25:19 UTC
Created attachment 299944 [details] [review]
clip chunk size to valid maximum

With bug URL in commit message.
Comment 11 Vincent Penquerc'h 2015-04-07 11:13:29 UTC
commit 8cfebfec8c6ca7a47dd064c8f5d3e587973f31a1
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Fri Mar 20 12:18:37 2015 +0000

    wavparse: clip chunk size above the valid maximum (0x7fffffff)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722567

commit 3ac119bbe2c360e28c087cf3852ea769d611b120
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Fri Mar 20 09:07:35 2015 +0000

    wavparse: clip chunk length to available data (when known)
    
    This prevents silly chunk lengths from possibly overflowing
    (at least when we know the actual data length).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722567