GNOME Bugzilla – Bug 722567
wavparse: loops on incorrect wav file
Last modified: 2015-04-07 11:14:15 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.
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.
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.
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.
Created attachment 272250 [details] Corrupt wav file, with valid riff, fmt and data chunks
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?
Sure, thanks for the test file.
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.
> 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...
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.
Created attachment 299944 [details] [review] clip chunk size to valid maximum With bug URL in commit message.
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