[PATCH 1/3] avcodec/wmavoice: Don't initialize GetBitContext with buf == NULL
Happens when flushing. This triggers NULL + 0 (which is UB) in init_get_bits_xe (which previously errored out, but the return value has not been checked) and in copy_bits(). This fixes the wmavoice-(7|11|19)k FATE-tests with UBSan. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/wmavoice.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c index 4438089e51..26744719e6 100644 --- a/libavcodec/wmavoice.c +++ b/libavcodec/wmavoice.c @@ -1900,6 +1900,8 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, { WMAVoiceContext *s = ctx->priv_data; GetBitContext *gb = &s->gb; + const uint8_t *buf = avpkt->data; + uint8_t dummy[1]; int size, res, pos; /* Packets are sometimes a multiple of ctx->block_align, with a packet @@ -1908,7 +1910,8 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, * in a single "muxer" packet, so we artificially emulate that by * capping the packet size at ctx->block_align. */ for (size = avpkt->size; size > ctx->block_align; size -= ctx->block_align); - init_get_bits8(&s->gb, avpkt->data, size); + buf = size ? buf : dummy; + init_get_bits8(&s->gb, buf, size); /* size == ctx->block_align is used to indicate whether we are dealing with * a new packet or a packet of which we already read the packet header @@ -1931,7 +1934,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, if (cnt + s->spillover_nbits > avpkt->size * 8) { s->spillover_nbits = avpkt->size * 8 - cnt; } - copy_bits(&s->pb, avpkt->data, size, gb, s->spillover_nbits); + copy_bits(&s->pb, buf, size, gb, s->spillover_nbits); flush_put_bits(&s->pb); s->sframe_cache_size += s->spillover_nbits; if ((res = synth_superframe(ctx, frame, got_frame_ptr)) == 0 && @@ -1968,7 +1971,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, } else if ((s->sframe_cache_size = pos) > 0) { /* ... cache it for spillover in next packet */ init_put_bits(&s->pb, s->sframe_cache, SFRAME_CACHE_MAXSIZE); - copy_bits(&s->pb, avpkt->data, size, gb, s->sframe_cache_size); + copy_bits(&s->pb, buf, size, gb, s->sframe_cache_size); // FIXME bad - just copy bytes as whole and add use the // skip_bits_next field } -- 2.34.1
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/wmavoice.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c index 26744719e6..bb98f841a5 100644 --- a/libavcodec/wmavoice.c +++ b/libavcodec/wmavoice.c @@ -1911,7 +1911,9 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, * capping the packet size at ctx->block_align. */ for (size = avpkt->size; size > ctx->block_align; size -= ctx->block_align); buf = size ? buf : dummy; - init_get_bits8(&s->gb, buf, size); + res = init_get_bits8(&s->gb, buf, size); + if (res < 0) + return res; /* size == ctx->block_align is used to indicate whether we are dealing with * a new packet or a packet of which we already read the packet header -- 2.34.1
Return immediately if not enough leftover bits are available when flushing. This is simpler and also avoids an init_get_bits(gb, NULL, 0) (which currently leads to NULL + 0, which is UB; this affects the lossless-wma(|-1|-2|-rawtile) FATE tests). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/wmalosslessdec.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c index 5112b763fa..d545d848e2 100644 --- a/libavcodec/wmalosslessdec.c +++ b/libavcodec/wmalosslessdec.c @@ -1192,16 +1192,15 @@ static int decode_packet(AVCodecContext *avctx, AVFrame *rframe, s->frame->nb_samples = 0; - if (!buf_size && s->num_saved_bits > get_bits_count(&s->gb)) { + if (!buf_size) { s->packet_done = 0; + if (s->num_saved_bits <= get_bits_count(&s->gb)) + return 0; if (!decode_frame(s)) s->num_saved_bits = 0; } else if (s->packet_done || s->packet_loss) { s->packet_done = 0; - if (!buf_size) - return 0; - s->next_packet_start = buf_size - FFMIN(avctx->block_align, buf_size); buf_size = FFMIN(avctx->block_align, buf_size); s->buf_bit_size = buf_size << 3; @@ -1299,7 +1298,7 @@ static int decode_packet(AVCodecContext *avctx, AVFrame *rframe, s->packet_offset = get_bits_count(gb) & 7; - return (s->packet_loss) ? AVERROR_INVALIDDATA : buf_size ? get_bits_count(gb) >> 3 : 0; + return (s->packet_loss) ? AVERROR_INVALIDDATA : get_bits_count(gb) >> 3; } static void flush(AVCodecContext *avctx) -- 2.34.1
Andreas Rheinhardt:
Happens when flushing. This triggers NULL + 0 (which is UB) in init_get_bits_xe (which previously errored out, but the return value has not been checked) and in copy_bits().
This fixes the wmavoice-(7|11|19)k FATE-tests with UBSan.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/wmavoice.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c index 4438089e51..26744719e6 100644 --- a/libavcodec/wmavoice.c +++ b/libavcodec/wmavoice.c @@ -1900,6 +1900,8 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, { WMAVoiceContext *s = ctx->priv_data; GetBitContext *gb = &s->gb; + const uint8_t *buf = avpkt->data; + uint8_t dummy[1]; int size, res, pos;
/* Packets are sometimes a multiple of ctx->block_align, with a packet @@ -1908,7 +1910,8 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, * in a single "muxer" packet, so we artificially emulate that by * capping the packet size at ctx->block_align. */ for (size = avpkt->size; size > ctx->block_align; size -= ctx->block_align); - init_get_bits8(&s->gb, avpkt->data, size); + buf = size ? buf : dummy; + init_get_bits8(&s->gb, buf, size);
/* size == ctx->block_align is used to indicate whether we are dealing with * a new packet or a packet of which we already read the packet header @@ -1931,7 +1934,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, if (cnt + s->spillover_nbits > avpkt->size * 8) { s->spillover_nbits = avpkt->size * 8 - cnt; } - copy_bits(&s->pb, avpkt->data, size, gb, s->spillover_nbits); + copy_bits(&s->pb, buf, size, gb, s->spillover_nbits); flush_put_bits(&s->pb); s->sframe_cache_size += s->spillover_nbits; if ((res = synth_superframe(ctx, frame, got_frame_ptr)) == 0 && @@ -1968,7 +1971,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, AVFrame *frame, } else if ((s->sframe_cache_size = pos) > 0) { /* ... cache it for spillover in next packet */ init_put_bits(&s->pb, s->sframe_cache, SFRAME_CACHE_MAXSIZE); - copy_bits(&s->pb, avpkt->data, size, gb, s->sframe_cache_size); + copy_bits(&s->pb, buf, size, gb, s->sframe_cache_size); // FIXME bad - just copy bytes as whole and add use the // skip_bits_next field }
Will apply this patchset tomorrow unless there are objections. - Andreas
participants (1)
-
Andreas Rheinhardt