[FFmpeg-devel] [PATCHv3 2/2] avformat: parse iTunes gapless information

[Michael Niedermayer]

On Fri, Aug 05, 2016 at 03:54:25PM -0700, kode54 at gmail.com wrote:
> From: Chris Moeller <kode54 at gmail.com>
> 
> ---
>  libavformat/mp3dec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 56c7f8c..3055e2c 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -295,6 +295,53 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, AVStream *st, int64_t base)
>      }
>  }
>  
> +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, uint64_t *duration)
> +{
> +    uint32_t v;
> +    AVDictionaryEntry *de;
> +    MP3DecContext *mp3 = s->priv_data;
> +    size_t length;
> +    uint32_t zero, start_pad, end_pad;
> +    uint64_t last_eight_frames_offset;
> +    int i;
> +
> +    if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 0)))
> +      return;
> +
> +    length = strlen(de->value);
> +
> +    /* Minimum length is one digit per field plus the whitespace, maximum length should depend on field type
> +     * There are four fields we need in the first six, the rest are currently zero padding */
> +    if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11))
> +        return;
> +

> +    if (sscanf(de->value, "%x %x %x %llx %x %llx", &zero, &start_pad, &end_pad, duration, &zero, &last_eight_frames_offset) < 6) {
> +        *duration = 0;
> +        return;
> +    }

libavformat/mp3dec.c: In function ‘mp3_parse_itunes_tag’:
libavformat/mp3dec.c:318:5: warning: format ‘%llx’ expects argument of type ‘long long unsigned int *’, but argument 6 has type ‘uint64_t *’ [-Wformat]
libavformat/mp3dec.c:318:5: warning: format ‘%llx’ expects argument of type ‘long long unsigned int *’, but argument 8 has type ‘uint64_t *’ [-Wformat]

check for duration < 0 missing


> +
> +    mp3->start_pad = start_pad;
> +    mp3->end_pad = end_pad;

assigning unsigend to signed with no range checks could result in
overflow, though even if it doesnt overflow the value should be
checked to be within a sane range


> +    if (end_pad >= 528 + 1)
> +        mp3->end_pad = end_pad - (528 + 1);
> +    st->start_skip_samples = mp3->start_pad + 528 + 1;
> +    av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad);
> +    if (!s->pb->seekable)
> +        return;
> +

> +    *size = (unsigned int) last_eight_frames_offset;

value could be truncated, missing range check


> +    avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, SEEK_SET);

missing seek faiure check

also please provide a testcase/sample for this

(a fate test would be even better)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160807/bfc2be93/attachment.sig>