GNOME Bugzilla – Bug 727955
id3v2: ignore RVA2 tags with 0 peak bits
Last modified: 2015-03-30 11:38:18 UTC
Created attachment 273976 [details] [review] ignore RVA2 tags with out of range peak bits It's not quite clear what the behavior should be for 0 and out of range peak bits. See commit message. The patch rejects, but one could also use 0 or the max representable value. The spec does not say (http://id3.org/id3v2.4.0-frames).
Comment on attachment 273976 [details] [review] ignore RVA2 tags with out of range peak bits >Subject: [PATCH] id3v2: ignore RVA2 tags with 0 peak bits > >The spec for this does not say nor imply how this should be >interpreted. One could use 0, 2<31, or reject as invalid. >This patch does the latter. Same for > 64 bits. The previous >code would try to shift by 64 bits, which is undefined. > >- if (peak_bits > 64) { >+ if (peak_bits > 64 || peak_bits == 0) { > GST_WARNING ("silly peak precision of %d bits, ignoring", (gint) peak_bits); >- peak_bits = 0; >+ g_free (id); >+ return FALSE; > } I'm not sure if I agree with this. A peak_bits value of 0 appears to be valid according to the "spec" ("0 means that there is no peak volume field"), so we should just ignore the peak stuff if peak_bits is 0 or >64, and not return FALSE (which means we'll also not add the gain tag). The big shift is an issue of course, but maybe should just be skipped if bits is 0?
Re-reading the spec bit and the code, I agree with you about 0. For > 64, I could just warn and ignore the field.
Created attachment 300565 [details] [review] ignore RVA2 tags with out of range peak bits
Comment on attachment 300565 [details] [review] ignore RVA2 tags with out of range peak bits Thanks, looks good to me, though I wonder what the rationale is for moving the if (peak_bits > 64) peak_bits = 0 out of the previous if() block? Would be great if you could also add the bug url to the commit message before you push.
The move was because the previous code would not read the declared N bytes. However, the not-reading doesn't seem to matter, as the data/datalen vars are local and no data is read after that field, so no desync seems possible anyway. I'll push with the URL and this part moved back.
Created attachment 300578 [details] [review] ignore RVA2 tags with out of range peak bits
Pushed now, thanks for the comments commit e2a9f0ef4ef54d212456c4f5c61c03513dbbb4d7 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Thu Apr 10 12:03:05 2014 +0100 id3v2: ignore RVA2 tags with more than 64 peak bits The spec for this does not say nor imply how this should be interpreted. The previous code would try to shift by 64 bits, which is undefined. Coverity 1195119 https://bugzilla.gnome.org/show_bug.cgi?id=727955