[FFmpeg-devel] [PATCH 1/3] avcodec/wmavoice: Don't initialize GetBitContext with buf == NULL

[FFmpeg-devel] [PATCH 1/3] avcodec/wmavoice: Don't initialize GetBitContext with buf == NULL

[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
     }
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/3] avcodec/wmavoice: Check init_get_bits8()

[Andreas Rheinhardt]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/3] avcodec/wmalosslessdec: Simplify flushing, avoid NULL + 0

[Andreas Rheinhardt]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/wmavoice: Don't initialize GetBitContext with buf == NULL

[Andreas Rheinhardt]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".