On 28.06.2015 13:13, Ronald S. Bultje wrote:
> On Sun, Jun 28, 2015 at 5:28 AM, Andreas Cadhalpun
>> Treating one like an error, but not the other seems strange as well.
>> One could add an explode mode for both. Would that be better?
>
>
> In the first case, it's an error. If the frame size is 2 bits, the header
> is 1, and it specifies a spillover bits of 2, then the frame is clearly
> corrupt. Returning an error is fine. The ffmin() isn't necessary. I also
> don't think an explode mode check is necessary here, it's a clear error
> that is unrecoverable for this frame.
I've no strong preference. Attached is a variant just returning an error.
> In the second case, does that actually happen?
Yes.
> Wmavoice is one of the
> limited number of decoders that internally checks for overreads.
> get_bits_count() should never overread.
It doesn't check everywhere:
/* Statistics? FIXME - we don't check for length, a slight overrun
* will be caught by internal buffer padding, and anything else
* will be skipped, not read. */
if (get_bits1(gb)) {
res = get_bits(gb, 4);
skip_bits(gb, 10 * (res + 1));
}
> Do you have samples for which this happens?
Yes.
> We currently basically return an error on any possible overread
> signified in the bitstream (without actually overreading), so doing so here
> also would make sense (if it really happens at all).
OK.
> (We could also remove all the overread checks in the decoder, make it use
> the safe bitstream reader mode, and then check for overreads at the end of
> synth_superframe or in the caller, and then return an error. I have no
> specific preference, and this may lead to less code overall.)
That should work as well, but would be a larger change.
Best regards,
Andreas
>From 34aa9dd95c57039d56a07d0c9dfc837dd2199af8 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <[email protected]>
Date: Sun, 28 Jun 2015 12:40:12 +0200
Subject: [PATCH] wmavoice: limit wmavoice_decode_packet return value to packet
size
Signed-off-by: Andreas Cadhalpun <[email protected]>
---
libavcodec/wmavoice.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index ae88d4e..fff1aa8 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1982,7 +1982,14 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, void *data,
*got_frame_ptr) {
cnt += s->spillover_nbits;
s->skip_bits_next = cnt & 7;
- return cnt >> 3;
+ res = cnt >> 3;
+ if (res > avpkt->size) {
+ av_log(ctx, AV_LOG_ERROR,
+ "Trying to skip %d bytes in packet of size %d\n",
+ res, avpkt->size);
+ return AVERROR_INVALIDDATA;
+ }
+ return res;
} else
skip_bits_long (gb, s->spillover_nbits - cnt +
get_bits_count(gb)); // resync
@@ -2001,7 +2008,14 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, void *data,
} else if (*got_frame_ptr) {
int cnt = get_bits_count(gb);
s->skip_bits_next = cnt & 7;
- return cnt >> 3;
+ res = cnt >> 3;
+ if (res > avpkt->size) {
+ av_log(ctx, AV_LOG_ERROR,
+ "Trying to skip %d bytes in packet of size %d\n",
+ res, avpkt->size);
+ return AVERROR_INVALIDDATA;
+ }
+ return res;
} else if ((s->sframe_cache_size = pos) > 0) {
/* rewind bit reader to start of last (incomplete) superframe... */
init_get_bits(gb, avpkt->data, size << 3);
--
2.1.4
_______________________________________________
ffmpeg-devel mailing list
[email protected]
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
