On 6/16/23 15:00, Andreas Rheinhardt wrote:
Leo Izen:
This switches the jpegxl_collect_codestream_header function to use
avcodec/bytestream2, which better enforces barriers, and should avoid
overrunning buffers with jxlp boxes if the size is zero or if the size
is so small the box is invalid.

Signed-off-by: Leo Izen <leo.i...@gmail.com>
---
  libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++----------------
  1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
index 6ea6c46d8f..ec400c955c 100644
--- a/libavformat/jpegxl_anim_dec.c
+++ b/libavformat/jpegxl_anim_dec.c
@@ -28,6 +28,7 @@
  #include <stdint.h>
  #include <string.h>
+#include "libavcodec/bytestream.h"
  #define BITSTREAM_READER_LE
  #include "libavcodec/get_bits.h"
@@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext {
   * returns the number of bytes consumed from input, may be greater than 
input_len
   * if the input doesn't end on an ISOBMFF-box boundary
   */
-static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int 
input_len, uint8_t *buffer, int buflen, int *copied) {
-    const uint8_t *b = input_buffer;
+static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int 
input_len,
+                                            uint8_t *buffer, int buflen, int 
*copied) {
+    GetByteContext gb;
      *copied = 0;
+    bytestream2_init(&gb, input_buffer, input_len);
while (1) {
          uint64_t size;
          uint32_t tag;
          int head_size = 8;
- if (b - input_buffer >= input_len - 16)
+        if (bytestream2_get_bytes_left(&gb) < 16)
              break;
- size = AV_RB32(b);
-        b += 4;
+        size = bytestream2_get_be32(&gb);
          if (size == 1) {
-            size = AV_RB64(b);
-            b += 8;
+            size = bytestream2_get_be64(&gb);
              head_size = 16;
          }
          /* invalid ISOBMFF size */
-        if (size > 0 && size <= head_size)
+        if (size && size <= head_size)
              return AVERROR_INVALIDDATA;
-        if (size > 0)
+        if (size)
              size -= head_size;
- tag = AV_RL32(b);
-        b += 4;
+        tag = bytestream2_get_le32(&gb);
          if (tag == MKTAG('j', 'x', 'l', 'p')) {
-            b += 4;
-            size -= 4;
+            if (bytestream2_get_bytes_left(&gb) < 4)
+                break;
+            bytestream2_skip(&gb, 4);
+            if (size) {
+                if (size <= 4)
+                    return AVERROR_INVALIDDATA;
+                size -= 4;
+            }
          }
+        /*
+         * size = 0 means "until EOF". this is legal but uncommon
+         * here we just set it to the remaining size of the probe buffer
+         */
+        if (!size)
+            size = bytestream2_get_bytes_left(&gb);
if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) {
-            /*
-             * size = 0 means "until EOF". this is legal but uncommon
-             * here we just set it to the remaining size of the probe buffer
-             * which at this point should always be nonnegative
-             */
-            if (size == 0 || size > input_len - (b - input_buffer))
-                size = input_len - (b - input_buffer);
-
              if (size > buflen - *copied)
                  size = buflen - *copied;
              /*
               * arbitrary chunking of the payload makes this memcpy hard to 
avoid
               * in practice this will only be performed one or two times at 
most
               */
-            memcpy(buffer + *copied, b, size);
-            *copied += size;
+            *copied += bytestream2_get_buffer(&gb, buffer + *copied, size);
+        } else {
+            bytestream2_skip(&gb, size);
          }
-        b += size;
-        if (b >= input_buffer + input_len || *copied >= buflen)
+        if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen)
              break;
      }
- return b - input_buffer;
+    return bytestream2_tell(&gb);
  }
static int jpegxl_anim_probe(const AVProbeData *p)

Is there an actual (potential) overrun or is this just a precaution?

- Andreas


Yea, michaelni sent a cvslog email about it earlier, mail ID <20230608002033.GF870501@pb2>. This fixes that and switches to bytestream2 at Anton's recommendation.

- Leo Izen

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

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

Reply via email to