[FFmpeg-devel] [PATCH 0/5] JPEG XL Animation Changes

[FFmpeg-devel] [PATCH 0/5] JPEG XL Animation Changes

[Leo Izen]

Three patches from michalni copied exactly as is from the ML just placed
here for clarity.

The two authored by me, the probe change is cosmetic, and the animation
dec change fixes the issue referenced a cvslog email: <20230608002033.GF870501@pb2>

Leo Izen (2):
  avformat/jpegxl_probe: inline various ret < 0 checks
  avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

Michael Niedermayer (3):
  avformat/jpegxl_probe: Remove intermediate macro obfuscation around
    get_bits*()
  avformat/jpegxl_probe: check length instead of blindly reading
  avformat/jpegxl_probe: Forward error codes

 libavformat/jpegxl_anim_dec.c |   8 +-
 libavformat/jpegxl_probe.c    | 198 ++++++++++++++++++----------------
 2 files changed, 112 insertions(+), 94 deletions(-)

-- 
2.40.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 1/5] avformat/jpegxl_probe: Remove intermediate macro obfuscation around get_bits*()

[Leo Izen]

From: Michael Niedermayer <michael@niedermayer.cc>

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/jpegxl_probe.c | 169 +++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 84 deletions(-)

diff --git a/libavformat/jpegxl_probe.c b/libavformat/jpegxl_probe.c
index a3845b037d..1d9c014f19 100644
--- a/libavformat/jpegxl_probe.c
+++ b/libavformat/jpegxl_probe.c
@@ -57,49 +57,50 @@ enum JpegXLPrimaries {
     FF_JPEGXL_PR_P3 = 11,
 };
 
-#define jxl_bits(n) get_bits_long(gb, (n))
-#define jxl_bits_skip(n) skip_bits_long(gb, (n))
-#define jxl_u32(c0, c1, c2, c3, u0, u1, u2, u3) jpegxl_u32(gb, \
-    (const uint32_t[]){c0, c1, c2, c3}, (const uint32_t[]){u0, u1, u2, u3})
-#define jxl_u64() jpegxl_u64(gb)
-#define jxl_enum() jxl_u32(0, 1, 2, 18, 0, 0, 4, 6)
-
 /* read a U32(c_i + u(u_i)) */
-static uint32_t jpegxl_u32(GetBitContext *gb,
-                           const uint32_t constants[4], const uint32_t ubits[4])
+static av_always_inline uint32_t jxl_u32(GetBitContext *gb,
+                        uint32_t c0, uint32_t c1, uint32_t c2, uint32_t c3,
+                        uint32_t u0, uint32_t u1, uint32_t u2, uint32_t u3)
 {
-    uint32_t ret, choice = jxl_bits(2);
+    const uint32_t constants[4] = {c0, c1, c2, c3};
+    const uint32_t ubits    [4] = {u0, u1, u2, u3};
+    uint32_t ret, choice = get_bits(gb, 2);
 
     ret = constants[choice];
     if (ubits[choice])
-        ret += jxl_bits(ubits[choice]);
+        ret += get_bits_long(gb, ubits[choice]);
 
     return ret;
 }
 
+static av_always_inline uint32_t jxl_enum(GetBitContext *gb)
+{
+    return jxl_u32(gb, 0, 1, 2, 18, 0, 0, 4, 6);
+}
+
 /* read a U64() */
 static uint64_t jpegxl_u64(GetBitContext *gb)
 {
     uint64_t shift = 12, ret;
 
-    switch (jxl_bits(2)) {
+    switch (get_bits(gb, 2)) {
     case 0:
         ret = 0;
         break;
     case 1:
-        ret = 1 + jxl_bits(4);
+        ret = 1 + get_bits(gb, 4);
         break;
     case 2:
-        ret = 17 + jxl_bits(8);
+        ret = 17 + get_bits(gb, 8);
         break;
     case 3:
-        ret = jxl_bits(12);
-        while (jxl_bits(1)) {
+        ret = get_bits(gb, 12);
+        while (get_bits1(gb)) {
             if (shift < 60) {
-                ret |= (uint64_t)jxl_bits(8) << shift;
+                ret |= (uint64_t)get_bits(gb, 8) << shift;
                 shift += 8;
             } else {
-                ret |= (uint64_t)jxl_bits(4) << shift;
+                ret |= (uint64_t)get_bits(gb, 4) << shift;
                 break;
             }
         }
@@ -142,18 +143,18 @@ static int jpegxl_read_size_header(GetBitContext *gb)
 {
     uint32_t width, height;
 
-    if (jxl_bits(1)) {
+    if (get_bits1(gb)) {
         /* small size header */
-        height = (jxl_bits(5) + 1) << 3;
-        width = jpegxl_width_from_ratio(height, jxl_bits(3));
+        height = (get_bits(gb, 5) + 1) << 3;
+        width = jpegxl_width_from_ratio(height, get_bits(gb, 3));
         if (!width)
-            width = (jxl_bits(5) + 1) << 3;
+            width = (get_bits(gb, 5) + 1) << 3;
     } else {
         /* large size header */
-        height = 1 + jxl_u32(0, 0, 0, 0, 9, 13, 18, 30);
-        width = jpegxl_width_from_ratio(height, jxl_bits(3));
+        height = 1 + jxl_u32(gb, 0, 0, 0, 0, 9, 13, 18, 30);
+        width = jpegxl_width_from_ratio(height, get_bits(gb, 3));
         if (!width)
-            width = 1 + jxl_u32(0, 0, 0, 0, 9, 13, 18, 30);
+            width = 1 + jxl_u32(gb, 0, 0, 0, 0, 9, 13, 18, 30);
     }
     if (width > (1 << 18) || height > (1 << 18)
         || (width >> 4) * (height >> 4) > (1 << 20))
@@ -170,18 +171,18 @@ static int jpegxl_read_preview_header(GetBitContext *gb)
 {
     uint32_t width, height;
 
-    if (jxl_bits(1)) {
+    if (get_bits1(gb)) {
         /* coded height and width divided by eight */
-        height = jxl_u32(16, 32, 1, 33, 0, 0, 5, 9) << 3;
-        width = jpegxl_width_from_ratio(height, jxl_bits(3));
+        height = jxl_u32(gb, 16, 32, 1, 33, 0, 0, 5, 9) << 3;
+        width = jpegxl_width_from_ratio(height, get_bits(gb, 3));
         if (!width)
-            width = jxl_u32(16, 32, 1, 33, 0, 0, 5, 9) << 3;
+            width = jxl_u32(gb, 16, 32, 1, 33, 0, 0, 5, 9) << 3;
     } else {
         /* full height and width coded */
-        height = jxl_u32(1, 65, 321, 1345, 6, 8, 10, 12);
-        width = jpegxl_width_from_ratio(height, jxl_bits(3));
+        height = jxl_u32(gb, 1, 65, 321, 1345, 6, 8, 10, 12);
+        width = jpegxl_width_from_ratio(height, get_bits(gb, 3));
         if (!width)
-            width = jxl_u32(1, 65, 321, 1345, 6, 8, 10, 12);
+            width = jxl_u32(gb, 1, 65, 321, 1345, 6, 8, 10, 12);
     }
     if (width > 4096 || height > 4096)
         return -1;
@@ -194,13 +195,13 @@ static int jpegxl_read_preview_header(GetBitContext *gb)
  */
 static void jpegxl_skip_bit_depth(GetBitContext *gb)
 {
-    if (jxl_bits(1)) {
+    if (get_bits1(gb)) {
         /* float samples */
-        jxl_u32(32, 16, 24, 1, 0, 0, 0, 6); /* mantissa */
-        jxl_bits_skip(4); /* exponent */
+        jxl_u32(gb, 32, 16, 24, 1, 0, 0, 0, 6); /* mantissa */
+        skip_bits_long(gb, 4); /* exponent */
     } else {
         /* integer samples */
-        jxl_u32(8, 10, 12, 1, 0, 0, 0, 6);
+        jxl_u32(gb, 8, 10, 12, 1, 0, 0, 0, 6);
     }
 }
 
@@ -210,34 +211,34 @@ static void jpegxl_skip_bit_depth(GetBitContext *gb)
  */
 static int jpegxl_read_extra_channel_info(GetBitContext *gb, int validate_level)
 {
-    int all_default = jxl_bits(1);
+    int all_default = get_bits1(gb);
     uint32_t type, name_len = 0;
 
     if (!all_default) {
-        type = jxl_enum();
+        type = jxl_enum(gb);
         if (type > 63)
             return -1; /* enum types cannot be 64+ */
         if (type == FF_JPEGXL_CT_BLACK && validate_level)
             return -1;
         jpegxl_skip_bit_depth(gb);
-        jxl_u32(0, 3, 4, 1, 0, 0, 0, 3); /* dim-shift */
+        jxl_u32(gb, 0, 3, 4, 1, 0, 0, 0, 3); /* dim-shift */
         /* max of name_len is 1071 = 48 + 2^10 - 1 */
-        name_len = jxl_u32(0, 0, 16, 48, 0, 4, 5, 10);
+        name_len = jxl_u32(gb, 0, 0, 16, 48, 0, 4, 5, 10);
     } else {
         type = FF_JPEGXL_CT_ALPHA;
     }
 
     /* skip over the name */
-    jxl_bits_skip(8 * name_len);
+    skip_bits_long(gb, 8 * name_len);
 
     if (!all_default && type == FF_JPEGXL_CT_ALPHA)
-        jxl_bits_skip(1);
+        skip_bits1(gb);
 
     if (type == FF_JPEGXL_CT_SPOT_COLOR)
-        jxl_bits_skip(16 * 4);
+        skip_bits_long(gb, 16 * 4);
 
     if (type == FF_JPEGXL_CT_CFA)
-        jxl_u32(1, 0, 3, 19, 0, 2, 4, 8);
+        jxl_u32(gb, 1, 0, 3, 19, 0, 2, 4, 8);
 
     return 0;
 }
@@ -256,40 +257,40 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
     if (ret < 0)
         return ret;
 
-    if (jxl_bits(16) != FF_JPEGXL_CODESTREAM_SIGNATURE_LE)
+    if (get_bits_long(gb, 16) != FF_JPEGXL_CODESTREAM_SIGNATURE_LE)
         return -1;
 
     if (jpegxl_read_size_header(gb) < 0 && validate_level)
         return -1;
 
-    all_default = jxl_bits(1);
+    all_default = get_bits1(gb);
     if (!all_default)
-        extra_fields = jxl_bits(1);
+        extra_fields = get_bits1(gb);
 
     if (extra_fields) {
-        jxl_bits_skip(3); /* orientation */
+        skip_bits_long(gb, 3); /* orientation */
 
         /*
          * intrinstic size
          * any size header here is valid, but as it
          * is variable length we have to read it
          */
-        if (jxl_bits(1))
+        if (get_bits1(gb))
             jpegxl_read_size_header(gb);
 
         /* preview header */
-        if (jxl_bits(1)) {
+        if (get_bits1(gb)) {
             if (jpegxl_read_preview_header(gb) < 0)
                 return -1;
         }
 
         /* animation header */
-        if (jxl_bits(1)) {
+        if (get_bits1(gb)) {
             animation_offset = get_bits_count(gb);
-            jxl_u32(100, 1000, 1, 1, 0, 0, 10, 30);
-            jxl_u32(1, 1001, 1, 1, 0, 0, 8, 10);
-            jxl_u32(0, 0, 0, 0, 0, 3, 16, 32);
-            jxl_bits_skip(1);
+            jxl_u32(gb, 100, 1000, 1, 1, 0, 0, 10, 30);
+            jxl_u32(gb, 1, 1001, 1, 1, 0, 0, 8, 10);
+            jxl_u32(gb, 0, 0, 0, 0, 0, 3, 16, 32);
+            skip_bits_long(gb, 1);
         }
     }
 
@@ -297,10 +298,10 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
         jpegxl_skip_bit_depth(gb);
 
         /* modular_16bit_buffers must equal 1 */
-        if (!jxl_bits(1) && validate_level)
+        if (!get_bits1(gb) && validate_level)
             return -1;
 
-        num_extra_channels = jxl_u32(0, 1, 2, 1, 0, 0, 4, 12);
+        num_extra_channels = jxl_u32(gb, 0, 1, 2, 1, 0, 0, 4, 12);
         if (num_extra_channels > 4 && validate_level)
             return -1;
         for (uint32_t i = 0; i < num_extra_channels; i++) {
@@ -308,85 +309,85 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
                 return -1;
         }
 
-        xyb_encoded = jxl_bits(1);
+        xyb_encoded = get_bits1(gb);
 
         /* color encoding bundle */
-        if (!jxl_bits(1)) {
+        if (!get_bits1(gb)) {
             uint32_t color_space;
-            have_icc_profile = jxl_bits(1);
-            color_space = jxl_enum();
+            have_icc_profile = get_bits1(gb);
+            color_space = jxl_enum(gb);
             if (color_space > 63)
                 return -1;
 
             if (!have_icc_profile) {
                 if (color_space != FF_JPEGXL_CS_XYB) {
-                    uint32_t white_point = jxl_enum();
+                    uint32_t white_point = jxl_enum(gb);
                     if (white_point > 63)
                         return -1;
                     if (white_point == FF_JPEGXL_WP_CUSTOM) {
                         /* ux and uy values */
-                        jxl_u32(0, 524288, 1048576, 2097152, 19, 19, 20, 21);
-                        jxl_u32(0, 524288, 1048576, 2097152, 19, 19, 20, 21);
+                        jxl_u32(gb, 0, 524288, 1048576, 2097152, 19, 19, 20, 21);
+                        jxl_u32(gb, 0, 524288, 1048576, 2097152, 19, 19, 20, 21);
                     }
                     if (color_space != FF_JPEGXL_CS_GRAY) {
                         /* primaries */
-                        uint32_t primaries = jxl_enum();
+                        uint32_t primaries = jxl_enum(gb);
                         if (primaries > 63)
                             return -1;
                         if (primaries == FF_JPEGXL_PR_CUSTOM) {
                             /* ux/uy values for r,g,b */
                             for (int i = 0; i < 6; i++)
-                                jxl_u32(0, 524288, 1048576, 2097152, 19, 19, 20, 21);
+                                jxl_u32(gb, 0, 524288, 1048576, 2097152, 19, 19, 20, 21);
                         }
                     }
                 }
 
                 /* transfer characteristics */
-                if (jxl_bits(1)) {
+                if (get_bits1(gb)) {
                     /* gamma */
-                    jxl_bits_skip(24);
+                    skip_bits_long(gb, 24);
                 } else {
                     /* transfer function */
-                    if (jxl_enum() > 63)
+                    if (jxl_enum(gb) > 63)
                         return -1;
                 }
 
                 /* rendering intent */
-                if (jxl_enum() > 63)
+                if (jxl_enum(gb) > 63)
                     return -1;
             }
         }
 
         /* tone mapping bundle */
-        if (extra_fields && !jxl_bits(1))
-            jxl_bits_skip(16 + 16 + 1 + 16);
+        if (extra_fields && !get_bits1(gb))
+            skip_bits_long(gb, 16 + 16 + 1 + 16);
 
-        extensions = jxl_u64();
+        extensions = jpegxl_u64(gb);
         if (extensions) {
             for (int i = 0; i < 64; i++) {
                 if (extensions & (UINT64_C(1) << i))
-                    jxl_u64();
+                    jpegxl_u64(gb);
             }
         }
     }
 
     /* default transform */
-    if (!jxl_bits(1)) {
+    if (!get_bits1(gb)) {
         /* opsin inverse matrix */
-        if (xyb_encoded && !jxl_bits(1))
-            jxl_bits_skip(16 * 16);
+        if (xyb_encoded && !get_bits1(gb))
+            skip_bits_long(gb, 16 * 16);
         /* cw_mask and default weights */
-        if (jxl_bits(1))
-            jxl_bits_skip(16 * 15);
-        if (jxl_bits(1))
-            jxl_bits_skip(16 * 55);
-        if (jxl_bits(1))
-            jxl_bits_skip(16 * 210);
+        if (get_bits1(gb))
+            skip_bits_long(gb, 16 * 15);
+        if (get_bits1(gb))
+            skip_bits_long(gb, 16 * 55);
+        if (get_bits1(gb))
+            skip_bits_long(gb, 16 * 210);
     }
 
     if (!have_icc_profile) {
         int bits_remaining = 7 - (get_bits_count(gb) - 1) % 8;
-        if (bits_remaining && jxl_bits(bits_remaining))
+        if (bits_remaining && get_bits(gb, bits_remaining))
             return -1;
     }
 
-- 
2.40.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/5] avformat/jpegxl_probe: check length instead of blindly reading

[Leo Izen]

From: Michael Niedermayer <michael@niedermayer.cc>

Enable the checked bitreader to avoid overread.
Also add a few checks in loops and between blocks so we exit instead of continued
execution.
Alternatively we could add manual checks so that no overread can happen. This would be
slightly faster but a bit more work and a bit more fragile

Fixes: Out of array accesses
Fixes: 59640/clusterfuzz-testcase-minimized-ffmpeg_dem_JPEGXL_ANIM_fuzzer-6584117345779712

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/jpegxl_probe.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/libavformat/jpegxl_probe.c b/libavformat/jpegxl_probe.c
index 1d9c014f19..e15e9eee49 100644
--- a/libavformat/jpegxl_probe.c
+++ b/libavformat/jpegxl_probe.c
@@ -21,6 +21,7 @@
 
 #include "jpegxl_probe.h"
 
+#define UNCHECKED_BITSTREAM_READER 0
 #define BITSTREAM_READER_LE
 #include "libavcodec/get_bits.h"
 
@@ -293,6 +294,8 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
             skip_bits_long(gb, 1);
         }
     }
+    if (get_bits_left(gb) < 1)
+        return AVERROR_INVALIDDATA;
 
     if (!all_default) {
         jpegxl_skip_bit_depth(gb);
@@ -307,6 +310,8 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
         for (uint32_t i = 0; i < num_extra_channels; i++) {
             if (jpegxl_read_extra_channel_info(gb, validate_level) < 0)
                 return -1;
+            if (get_bits_left(gb) < 1)
+                return AVERROR_INVALIDDATA;
         }
 
         xyb_encoded = get_bits1(gb);
@@ -336,8 +341,11 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
                             return -1;
                         if (primaries == FF_JPEGXL_PR_CUSTOM) {
                             /* ux/uy values for r,g,b */
-                            for (int i = 0; i < 6; i++)
+                            for (int i = 0; i < 6; i++) {
                                 jxl_u32(gb, 0, 524288, 1048576, 2097152, 19, 19, 20, 21);
+                                if (get_bits_left(gb) < 1)
+                                    return AVERROR_INVALIDDATA;
+                            }
                         }
                     }
                 }
@@ -363,10 +371,14 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
             skip_bits_long(gb, 16 + 16 + 1 + 16);
 
         extensions = jpegxl_u64(gb);
+        if (get_bits_left(gb) < 1)
+            return AVERROR_INVALIDDATA;
         if (extensions) {
             for (int i = 0; i < 64; i++) {
                 if (extensions & (UINT64_C(1) << i))
                     jpegxl_u64(gb);
+                if (get_bits_left(gb) < 1)
+                    return AVERROR_INVALIDDATA;
             }
         }
     }
-- 
2.40.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/5] avformat/jpegxl_probe: Forward error codes

[Leo Izen]

From: Michael Niedermayer <michael@niedermayer.cc>

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/jpegxl_probe.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/libavformat/jpegxl_probe.c b/libavformat/jpegxl_probe.c
index e15e9eee49..88492cb772 100644
--- a/libavformat/jpegxl_probe.c
+++ b/libavformat/jpegxl_probe.c
@@ -261,8 +261,8 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
     if (get_bits_long(gb, 16) != FF_JPEGXL_CODESTREAM_SIGNATURE_LE)
         return -1;
 
-    if (jpegxl_read_size_header(gb) < 0 && validate_level)
-        return -1;
+    if ((ret = jpegxl_read_size_header(gb)) < 0 && validate_level)
+        return ret;
 
     all_default = get_bits1(gb);
     if (!all_default)
@@ -281,8 +281,9 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
 
         /* preview header */
         if (get_bits1(gb)) {
-            if (jpegxl_read_preview_header(gb) < 0)
-                return -1;
+            ret = jpegxl_read_preview_header(gb);
+            if (ret < 0)
+                return ret;
         }
 
         /* animation header */
@@ -308,8 +309,9 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
         if (num_extra_channels > 4 && validate_level)
             return -1;
         for (uint32_t i = 0; i < num_extra_channels; i++) {
-            if (jpegxl_read_extra_channel_info(gb, validate_level) < 0)
-                return -1;
+            ret = jpegxl_read_extra_channel_info(gb, validate_level);
+            if (ret < 0)
+                return ret;
             if (get_bits_left(gb) < 1)
                 return AVERROR_INVALIDDATA;
         }
-- 
2.40.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 4/5] avformat/jpegxl_probe: inline various ret < 0 checks

[Leo Izen]

Inlines some ret < 0 checks to look like:
    if ((ret = func()) < 0)
        return ret;

which clarifies code slightly.

Signed-off-by: Leo Izen <leo.izen@gmail.com>
---
 libavformat/jpegxl_probe.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/libavformat/jpegxl_probe.c b/libavformat/jpegxl_probe.c
index 88492cb772..b4ab45518a 100644
--- a/libavformat/jpegxl_probe.c
+++ b/libavformat/jpegxl_probe.c
@@ -254,8 +254,7 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
     uint64_t extensions;
     int ret;
 
-    ret = init_get_bits8(gb, buf, buflen);
-    if (ret < 0)
+    if ((ret = init_get_bits8(gb, buf, buflen)) < 0)
         return ret;
 
     if (get_bits_long(gb, 16) != FF_JPEGXL_CODESTREAM_SIGNATURE_LE)
@@ -281,8 +280,7 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
 
         /* preview header */
         if (get_bits1(gb)) {
-            ret = jpegxl_read_preview_header(gb);
-            if (ret < 0)
+            if ((ret = jpegxl_read_preview_header(gb)) < 0)
                 return ret;
         }
 
@@ -309,8 +307,7 @@ int ff_jpegxl_verify_codestream_header(const uint8_t *buf, int buflen, int valid
         if (num_extra_channels > 4 && validate_level)
             return -1;
         for (uint32_t i = 0; i < num_extra_channels; i++) {
-            ret = jpegxl_read_extra_channel_info(gb, validate_level);
-            if (ret < 0)
+            if ((ret = jpegxl_read_extra_channel_info(gb, validate_level)) < 0)
                 return ret;
             if (get_bits_left(gb) < 1)
                 return AVERROR_INVALIDDATA;
-- 
2.40.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 4/5] avformat/jpegxl_probe: inline various ret < 0 checks

[Anton Khirnov]

Quoting Leo Izen (2023-06-08 16:26:36)
> Inlines some ret < 0 checks to look like:
>     if ((ret = func()) < 0)
>         return ret;
> 
> which clarifies code slightly.

FWIW I find this variant less readable.
But it's your code, so up to you.

-- 
Anton Khirnov
_______________________________________________
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 4/5] avformat/jpegxl_probe: inline various ret < 0 checks

[James Almer]

On 6/8/2023 11:32 PM, Anton Khirnov wrote:
> Quoting Leo Izen (2023-06-08 16:26:36)
>> Inlines some ret < 0 checks to look like:
>>      if ((ret = func()) < 0)
>>          return ret;
>>
>> which clarifies code slightly.
> 
> FWIW I find this variant less readable.
> But it's your code, so up to you.

Agree. It's both less readable and more prone to mistakes.
_______________________________________________
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 4/5] avformat/jpegxl_probe: inline various ret < 0 checks

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 769 bytes --]

On Thu, Jun 08, 2023 at 11:44:38PM -0300, James Almer wrote:
> On 6/8/2023 11:32 PM, Anton Khirnov wrote:
> > Quoting Leo Izen (2023-06-08 16:26:36)
> > > Inlines some ret < 0 checks to look like:
> > >      if ((ret = func()) < 0)
> > >          return ret;
> > > 
> > > which clarifies code slightly.
> > 
> > FWIW I find this variant less readable.
> > But it's your code, so up to you.
> 
> Agree. It's both less readable

> and more prone to mistakes.

yes, we had bugs with people misplacing a () in exactly this
style of checking the return

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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 5/5] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Leo Izen]

This 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.izen@gmail.com>
---
 libavformat/jpegxl_anim_dec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
index 6ea6c46d8f..c9e4dcd5fc 100644
--- a/libavformat/jpegxl_anim_dec.c
+++ b/libavformat/jpegxl_anim_dec.c
@@ -76,8 +76,14 @@ static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int inp
         tag = AV_RL32(b);
         b += 4;
         if (tag == MKTAG('j', 'x', 'l', 'p')) {
+            if (b - input_buffer >= input_len - 4)
+                break;
             b += 4;
-            size -= 4;
+            if (size) {
+                if (size < 4)
+                    return AVERROR_INVALIDDATA;
+                size -= 4;
+            }
         }
 
         if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) {
-- 
2.40.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 5/5] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Anton Khirnov]

Quoting Leo Izen (2023-06-08 16:26:37)
> This 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.izen@gmail.com>
> ---
>  libavformat/jpegxl_anim_dec.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
> index 6ea6c46d8f..c9e4dcd5fc 100644
> --- a/libavformat/jpegxl_anim_dec.c
> +++ b/libavformat/jpegxl_anim_dec.c
> @@ -76,8 +76,14 @@ static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int inp
>          tag = AV_RL32(b);
>          b += 4;
>          if (tag == MKTAG('j', 'x', 'l', 'p')) {
> +            if (b - input_buffer >= input_len - 4)
> +                break;
>              b += 4;
> -            size -= 4;
> +            if (size) {
> +                if (size < 4)
> +                    return AVERROR_INVALIDDATA;
> +                size -= 4;
> +            }

This looks like it should be using bytestream2. Is there a good reason
it is not?

-- 
Anton Khirnov
_______________________________________________
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 5/5] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Leo Izen]

On 6/8/23 22:30, Anton Khirnov wrote:
> Quoting Leo Izen (2023-06-08 16:26:37)
>> This 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.izen@gmail.com>
>> ---
>>   libavformat/jpegxl_anim_dec.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
>> index 6ea6c46d8f..c9e4dcd5fc 100644
>> --- a/libavformat/jpegxl_anim_dec.c
>> +++ b/libavformat/jpegxl_anim_dec.c
>> @@ -76,8 +76,14 @@ static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int inp
>>           tag = AV_RL32(b);
>>           b += 4;
>>           if (tag == MKTAG('j', 'x', 'l', 'p')) {
>> +            if (b - input_buffer >= input_len - 4)
>> +                break;
>>               b += 4;
>> -            size -= 4;
>> +            if (size) {
>> +                if (size < 4)
>> +                    return AVERROR_INVALIDDATA;
>> +                size -= 4;
>> +            }
> 
> This looks like it should be using bytestream2. Is there a good reason
> it is not?
> 

No particular reason, I'll send an updated patch using it.

Also, I pushed the first three patches that michaelni authored, but not 
the 4th or this one.

- 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-request@ffmpeg.org with subject "unsubscribe".