[FFmpeg-devel] [PATCH] avcodec/jpegxl_parser: check remaining data buffer size

[FFmpeg-devel] [PATCH] avcodec/jpegxl_parser: check remaining data buffer size

[Kacper Michajłow]

Fixes use of uninitialized value, reported by MSAN.

Found by OSS-Fuzz.

Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
---
 libavcodec/jpegxl_parser.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavcodec/jpegxl_parser.c b/libavcodec/jpegxl_parser.c
index 8c45e1a1b7..8371d78a45 100644
--- a/libavcodec/jpegxl_parser.c
+++ b/libavcodec/jpegxl_parser.c
@@ -504,9 +504,14 @@ static int read_dist_clustering(GetBitContext *gb, JXLEntropyDecoder *dec, JXLDi
         return 0;
     }
 
+    if (get_bits_left(gb) <= 0)
+        return AVERROR_BUFFER_TOO_SMALL;
+
     if (get_bits1(gb)) {
         /* simple clustering */
-        uint32_t nbits = get_bits(gb, 2);
+        int nbits = get_bits(gb, 2);
+        if (get_bits_left(gb) < nbits * bundle->num_dist)
+            return AVERROR_BUFFER_TOO_SMALL;
         for (int i = 0; i < bundle->num_dist; i++)
             bundle->cluster_map[i] = get_bitsz(gb, nbits);
     } else {
-- 
2.43.0

_______________________________________________
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] avcodec/jpegxl_parser: check remaining data buffer size

[Andreas Rheinhardt]

Kacper Michajłow:
> Fixes use of uninitialized value, reported by MSAN.
> 
> Found by OSS-Fuzz.
> 
> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> ---
>  libavcodec/jpegxl_parser.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/jpegxl_parser.c b/libavcodec/jpegxl_parser.c
> index 8c45e1a1b7..8371d78a45 100644
> --- a/libavcodec/jpegxl_parser.c
> +++ b/libavcodec/jpegxl_parser.c
> @@ -504,9 +504,14 @@ static int read_dist_clustering(GetBitContext *gb, JXLEntropyDecoder *dec, JXLDi
>          return 0;
>      }
>  
> +    if (get_bits_left(gb) <= 0)
> +        return AVERROR_BUFFER_TOO_SMALL;
> +
>      if (get_bits1(gb)) {
>          /* simple clustering */
> -        uint32_t nbits = get_bits(gb, 2);
> +        int nbits = get_bits(gb, 2);
> +        if (get_bits_left(gb) < nbits * bundle->num_dist)
> +            return AVERROR_BUFFER_TOO_SMALL;
>          for (int i = 0; i < bundle->num_dist; i++)
>              bundle->cluster_map[i] = get_bitsz(gb, nbits);
>      } else {

Where is the uninitialized value that you are speaking of? When the
implicit checks of the GetBit-API are enabled, the values when
overreading come from reading the padding which is supposed to be
initialized. Is it here?

- 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".

Re: [FFmpeg-devel] [PATCH] avcodec/jpegxl_parser: check remaining data buffer size

[Kacper Michajlow]

On Wed, 26 Jun 2024 at 21:00, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Kacper Michajłow:
> > Fixes use of uninitialized value, reported by MSAN.
> >
> > Found by OSS-Fuzz.
> >
> > Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> > ---
> >  libavcodec/jpegxl_parser.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/jpegxl_parser.c b/libavcodec/jpegxl_parser.c
> > index 8c45e1a1b7..8371d78a45 100644
> > --- a/libavcodec/jpegxl_parser.c
> > +++ b/libavcodec/jpegxl_parser.c
> > @@ -504,9 +504,14 @@ static int read_dist_clustering(GetBitContext *gb, JXLEntropyDecoder *dec, JXLDi
> >          return 0;
> >      }
> >
> > +    if (get_bits_left(gb) <= 0)
> > +        return AVERROR_BUFFER_TOO_SMALL;
> > +
> >      if (get_bits1(gb)) {
> >          /* simple clustering */
> > -        uint32_t nbits = get_bits(gb, 2);
> > +        int nbits = get_bits(gb, 2);
> > +        if (get_bits_left(gb) < nbits * bundle->num_dist)
> > +            return AVERROR_BUFFER_TOO_SMALL;
> >          for (int i = 0; i < bundle->num_dist; i++)
> >              bundle->cluster_map[i] = get_bitsz(gb, nbits);
> >      } else {
>
> Where is the uninitialized value that you are speaking of? When the
> implicit checks of the GetBit-API are enabled, the values when
> overreading come from reading the padding which is supposed to be
> initialized. Is it here?

Indeed, it makes sense. If the padding is supposed to be initialized,
I've sent small patches with memsets in the required places to fix all
my failing test cases. Let me know if it makes sense.

- Kacper
_______________________________________________
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".