[FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX

[FFmpeg-devel] [PATCH 1/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX

[Andreas Rheinhardt]

AVCodecParameters.extradata_size is an int.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/bsf/hevc_mp4toannexb.c | 2 +-
 libavcodec/bsf/vvc_mp4toannexb.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
index 8eec18f31e..c0df2b79a6 100644
--- a/libavcodec/bsf/hevc_mp4toannexb.c
+++ b/libavcodec/bsf/hevc_mp4toannexb.c
@@ -69,7 +69,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
 
             if (!nalu_len ||
                 nalu_len > bytestream2_get_bytes_left(&gb) ||
-                4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) {
+                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
                 ret = AVERROR_INVALIDDATA;
                 goto fail;
             }
diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
index 36bdae8f49..1b851f3223 100644
--- a/libavcodec/bsf/vvc_mp4toannexb.c
+++ b/libavcodec/bsf/vvc_mp4toannexb.c
@@ -159,7 +159,7 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
 
             if (!nalu_len ||
                 nalu_len > bytestream2_get_bytes_left(&gb) ||
-                4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) {
+                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
                 ret = AVERROR_INVALIDDATA;
                 goto fail;
             }
-- 
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/5] avcodec/bsf/hevc_mp4toannexb: Factor creating new extradata out

[Andreas Rheinhardt]

This is in preparation for the next commit.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/bsf/hevc_mp4toannexb.c | 64 ++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
index c0df2b79a6..a695cba370 100644
--- a/libavcodec/bsf/hevc_mp4toannexb.c
+++ b/libavcodec/bsf/hevc_mp4toannexb.c
@@ -37,38 +37,32 @@ typedef struct HEVCBSFContext {
     int      extradata_parsed;
 } HEVCBSFContext;
 
-static int hevc_extradata_to_annexb(AVBSFContext *ctx)
+static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
+                                             uint8_t **new_extradatap,
+                                             size_t *new_extradata_sizep)
 {
-    GetByteContext gb;
-    int length_size, num_arrays, i, j;
-    int ret = 0;
-
+    int num_arrays = bytestream2_get_byte(gb);
     uint8_t *new_extradata = NULL;
-    size_t   new_extradata_size = 0;
-
-    bytestream2_init(&gb, ctx->par_in->extradata, ctx->par_in->extradata_size);
-
-    bytestream2_skip(&gb, 21);
-    length_size = (bytestream2_get_byte(&gb) & 3) + 1;
-    num_arrays  = bytestream2_get_byte(&gb);
+    size_t new_extradata_size = 0;
+    int ret;
 
-    for (i = 0; i < num_arrays; i++) {
-        int type = bytestream2_get_byte(&gb) & 0x3f;
-        int cnt  = bytestream2_get_be16(&gb);
+    for (int i = 0; i < num_arrays; i++) {
+        int type = bytestream2_get_byte(gb) & 0x3f;
+        int cnt  = bytestream2_get_be16(gb);
 
         if (!(type == HEVC_NAL_VPS || type == HEVC_NAL_SPS || type == HEVC_NAL_PPS ||
               type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
-            av_log(ctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
+            av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
                    type);
             ret = AVERROR_INVALIDDATA;
             goto fail;
         }
 
-        for (j = 0; j < cnt; j++) {
-            const int nalu_len = bytestream2_get_be16(&gb);
+        for (int j = 0; j < cnt; j++) {
+            const int nalu_len = bytestream2_get_be16(gb);
 
             if (!nalu_len ||
-                nalu_len > bytestream2_get_bytes_left(&gb) ||
+                nalu_len > bytestream2_get_bytes_left(gb) ||
                 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
                 ret = AVERROR_INVALIDDATA;
                 goto fail;
@@ -78,11 +72,38 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
                 goto fail;
 
             AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
-            bytestream2_get_buffer(&gb, new_extradata + new_extradata_size + 4, nalu_len);
+            bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
             new_extradata_size += 4 + nalu_len;
             memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
         }
     }
+    *new_extradatap = new_extradata;
+    *new_extradata_sizep = new_extradata_size;
+
+    return 0;
+fail:
+    av_freep(&new_extradata);
+    return ret;
+}
+
+static int hevc_extradata_to_annexb(AVBSFContext *ctx)
+{
+    GetByteContext gb;
+    int length_size;
+    int ret = 0;
+
+    uint8_t *new_extradata = NULL;
+    size_t   new_extradata_size;
+
+    bytestream2_init(&gb, ctx->par_in->extradata, ctx->par_in->extradata_size);
+
+    bytestream2_skip(&gb, 21);
+    length_size = (bytestream2_get_byte(&gb) & 3) + 1;
+
+    ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
+                                            &new_extradata_size);
+    if (ret < 0)
+        return ret;
 
     av_freep(&ctx->par_out->extradata);
     ctx->par_out->extradata      = new_extradata;
@@ -92,9 +113,6 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
         av_log(ctx, AV_LOG_WARNING, "No parameter sets in the extradata\n");
 
     return length_size;
-fail:
-    av_freep(&new_extradata);
-    return ret;
 }
 
 static int hevc_mp4toannexb_init(AVBSFContext *ctx)
-- 
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/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

[Andreas Rheinhardt]

AVCodecParameters.extradata is supposed to be allocated with
av_malloc(); av_realloc() and its wrappers do not guarantee
the proper alignment. Therefore parse the extradata twice:
Once to check its validity and to determine the eventual size
and a second time to actually write the new extradata.

(Of course, not reallocating the buffer is beneficial in itself.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++----------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
index a695cba370..f5424e95b8 100644
--- a/libavcodec/bsf/hevc_mp4toannexb.c
+++ b/libavcodec/bsf/hevc_mp4toannexb.c
@@ -38,13 +38,11 @@ typedef struct HEVCBSFContext {
 } HEVCBSFContext;
 
 static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
-                                             uint8_t **new_extradatap,
+                                             uint8_t *new_extradata,
                                              size_t *new_extradata_sizep)
 {
     int num_arrays = bytestream2_get_byte(gb);
-    uint8_t *new_extradata = NULL;
     size_t new_extradata_size = 0;
-    int ret;
 
     for (int i = 0; i < num_arrays; i++) {
         int type = bytestream2_get_byte(gb) & 0x3f;
@@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
               type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
             av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
                    type);
-            ret = AVERROR_INVALIDDATA;
-            goto fail;
+            return AVERROR_INVALIDDATA;
         }
 
         for (int j = 0; j < cnt; j++) {
@@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
             if (!nalu_len ||
                 nalu_len > bytestream2_get_bytes_left(gb) ||
                 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
-                ret = AVERROR_INVALIDDATA;
-                goto fail;
+                return AVERROR_INVALIDDATA;
             }
-            ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE);
-            if (ret < 0)
-                goto fail;
-
-            AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
-            bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
+            if (new_extradata) {
+                AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
+                bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len);
+            } else
+                bytestream2_skipu(gb, nalu_len);
             new_extradata_size += 4 + nalu_len;
-            memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
         }
     }
-    *new_extradatap = new_extradata;
     *new_extradata_sizep = new_extradata_size;
 
     return 0;
-fail:
-    av_freep(&new_extradata);
-    return ret;
 }
 
 static int hevc_extradata_to_annexb(AVBSFContext *ctx)
@@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
     bytestream2_skip(&gb, 21);
     length_size = (bytestream2_get_byte(&gb) & 3) + 1;
 
-    ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
-                                            &new_extradata_size);
-    if (ret < 0)
-        return ret;
+    while (1) {
+        GetByteContext gb_bak = gb;
+        ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
+                                                &new_extradata_size);
+        if (ret < 0)
+            return ret;
+        if (new_extradata || !new_extradata_size)
+            break;
+        new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
+        if (!new_extradata)
+            return AVERROR(ENOMEM);
+        memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+        gb = gb_bak;
+    }
 
     av_freep(&ctx->par_out->extradata);
     ctx->par_out->extradata      = new_extradata;
-- 
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 4/5] avcodec/bsf/vvc_mp4toannexb: Factor creating new extradata out

[Andreas Rheinhardt]

This is in preparation for the next commit.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/bsf/vvc_mp4toannexb.c | 116 ++++++++++++++++++-------------
 1 file changed, 67 insertions(+), 49 deletions(-)

diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
index 1b851f3223..bfb0338116 100644
--- a/libavcodec/bsf/vvc_mp4toannexb.c
+++ b/libavcodec/bsf/vvc_mp4toannexb.c
@@ -37,16 +37,77 @@ typedef struct VVCBSFContext {
     int extradata_parsed;
 } VVCBSFContext;
 
+static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
+                                            uint8_t **new_extradatap,
+                                            size_t *new_extradata_sizep)
+{
+    int num_arrays = bytestream2_get_byte(gb);
+    uint8_t *new_extradata = NULL;
+    size_t new_extradata_size = 0;
+    int ret;
+
+    for (int i = 0; i < num_arrays; i++) {
+        int cnt;
+        int type = bytestream2_get_byte(gb) & 0x1f;
+
+        if (type == VVC_OPI_NUT || type == VVC_DCI_NUT)
+            cnt = 1;
+        else
+            cnt = bytestream2_get_be16(gb);
+
+        av_log(logctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
+
+        if (!(type == VVC_OPI_NUT || type == VVC_DCI_NUT ||
+              type == VVC_VPS_NUT || type == VVC_SPS_NUT || type == VVC_PPS_NUT
+              || type == VVC_PREFIX_SEI_NUT || type == VVC_SUFFIX_SEI_NUT)) {
+            av_log(logctx, AV_LOG_ERROR,
+                   "Invalid NAL unit type in extradata: %d\n", type);
+            ret = AVERROR_INVALIDDATA;
+            goto fail;
+        }
+
+        for (int j = 0; j < cnt; j++) {
+            const int nalu_len = bytestream2_get_be16(gb);
+
+            if (!nalu_len ||
+                nalu_len > bytestream2_get_bytes_left(gb) ||
+                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
+                ret = AVERROR_INVALIDDATA;
+                goto fail;
+            }
+            ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4
+                              + AV_INPUT_BUFFER_PADDING_SIZE);
+            if (ret < 0)
+                goto fail;
+
+            AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
+            bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4,
+                                   nalu_len);
+            new_extradata_size += 4 + nalu_len;
+            memset(new_extradata + new_extradata_size, 0,
+                   AV_INPUT_BUFFER_PADDING_SIZE);
+        }
+    }
+
+    *new_extradatap      = new_extradata;
+    *new_extradata_sizep = new_extradata_size;
+
+    return 0;
+fail:
+    av_freep(&new_extradata);
+    return ret;
+}
+
 static int vvc_extradata_to_annexb(AVBSFContext *ctx)
 {
     GetByteContext gb;
-    int length_size, num_arrays, i, j;
+    int length_size, i, j;
     int ret = 0;
     int temp = 0;
     int ptl_present;
 
     uint8_t *new_extradata = NULL;
-    size_t new_extradata_size = 0;
+    size_t new_extradata_size;
 
     int max_picture_width = 0;
     int max_picture_height = 0;
@@ -132,50 +193,10 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
                max_picture_width, max_picture_height, avg_frame_rate);
     }
 
-    num_arrays = bytestream2_get_byte(&gb);
-
-    for (i = 0; i < num_arrays; i++) {
-        int cnt;
-        int type = bytestream2_get_byte(&gb) & 0x1f;
-
-        if (type == VVC_OPI_NUT || type == VVC_DCI_NUT)
-            cnt = 1;
-        else
-            cnt = bytestream2_get_be16(&gb);
-
-        av_log(ctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
-
-        if (!(type == VVC_OPI_NUT || type == VVC_DCI_NUT ||
-              type == VVC_VPS_NUT || type == VVC_SPS_NUT || type == VVC_PPS_NUT
-              || type == VVC_PREFIX_SEI_NUT || type == VVC_SUFFIX_SEI_NUT)) {
-            av_log(ctx, AV_LOG_ERROR,
-                   "Invalid NAL unit type in extradata: %d\n", type);
-            ret = AVERROR_INVALIDDATA;
-            goto fail;
-        }
-
-        for (j = 0; j < cnt; j++) {
-            const int nalu_len = bytestream2_get_be16(&gb);
-
-            if (!nalu_len ||
-                nalu_len > bytestream2_get_bytes_left(&gb) ||
-                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
-                ret = AVERROR_INVALIDDATA;
-                goto fail;
-            }
-            ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4
-                              + AV_INPUT_BUFFER_PADDING_SIZE);
-            if (ret < 0)
-                goto fail;
-
-            AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
-            bytestream2_get_buffer(&gb, new_extradata + new_extradata_size + 4,
-                                   nalu_len);
-            new_extradata_size += 4 + nalu_len;
-            memset(new_extradata + new_extradata_size, 0,
-                   AV_INPUT_BUFFER_PADDING_SIZE);
-        }
-    }
+    ret = vvc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
+                                           &new_extradata_size);
+    if (ret < 0)
+        return ret;
 
     av_freep(&ctx->par_out->extradata);
     ctx->par_out->extradata = new_extradata;
@@ -185,9 +206,6 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
         av_log(ctx, AV_LOG_WARNING, "No parameter sets in the extradata\n");
 
     return length_size;
-  fail:
-    av_freep(&new_extradata);
-    return ret;
 }
 
 static int vvc_mp4toannexb_init(AVBSFContext *ctx)
-- 
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 5/5] avcodec/bsf/vvc_mp4toannexb: Don't realloc when creating new extradata

[Andreas Rheinhardt]

AVCodecParameters.extradata is supposed to be allocated with
av_malloc(); av_realloc() and its wrappers do not guarantee
the proper alignment. Therefore parse the extradata twice:
Once to check its validity and to determine the eventual size
and a second time to actually write the new extradata.

(Of course, not reallocating the buffer is beneficial in itself.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/bsf/vvc_mp4toannexb.c | 51 ++++++++++++++++----------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
index bfb0338116..1879c1fab9 100644
--- a/libavcodec/bsf/vvc_mp4toannexb.c
+++ b/libavcodec/bsf/vvc_mp4toannexb.c
@@ -38,13 +38,11 @@ typedef struct VVCBSFContext {
 } VVCBSFContext;
 
 static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
-                                            uint8_t **new_extradatap,
+                                            uint8_t *new_extradata,
                                             size_t *new_extradata_sizep)
 {
     int num_arrays = bytestream2_get_byte(gb);
-    uint8_t *new_extradata = NULL;
     size_t new_extradata_size = 0;
-    int ret;
 
     for (int i = 0; i < num_arrays; i++) {
         int cnt;
@@ -55,15 +53,15 @@ static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
         else
             cnt = bytestream2_get_be16(gb);
 
-        av_log(logctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
+        if (!new_extradata)
+            av_log(logctx, AV_LOG_DEBUG, "nalu_type %d cnt %d\n", type, cnt);
 
         if (!(type == VVC_OPI_NUT || type == VVC_DCI_NUT ||
               type == VVC_VPS_NUT || type == VVC_SPS_NUT || type == VVC_PPS_NUT
               || type == VVC_PREFIX_SEI_NUT || type == VVC_SUFFIX_SEI_NUT)) {
             av_log(logctx, AV_LOG_ERROR,
                    "Invalid NAL unit type in extradata: %d\n", type);
-            ret = AVERROR_INVALIDDATA;
-            goto fail;
+            return AVERROR_INVALIDDATA;
         }
 
         for (int j = 0; j < cnt; j++) {
@@ -72,30 +70,21 @@ static int vvc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
             if (!nalu_len ||
                 nalu_len > bytestream2_get_bytes_left(gb) ||
                 4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
-                ret = AVERROR_INVALIDDATA;
-                goto fail;
+                return AVERROR_INVALIDDATA;
             }
-            ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4
-                              + AV_INPUT_BUFFER_PADDING_SIZE);
-            if (ret < 0)
-                goto fail;
-
-            AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
-            bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4,
-                                   nalu_len);
+            if (new_extradata) {
+                AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
+                bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4,
+                                        nalu_len);
+            } else
+                bytestream2_skipu(gb, nalu_len);
             new_extradata_size += 4 + nalu_len;
-            memset(new_extradata + new_extradata_size, 0,
-                   AV_INPUT_BUFFER_PADDING_SIZE);
         }
     }
 
-    *new_extradatap      = new_extradata;
     *new_extradata_sizep = new_extradata_size;
 
     return 0;
-fail:
-    av_freep(&new_extradata);
-    return ret;
 }
 
 static int vvc_extradata_to_annexb(AVBSFContext *ctx)
@@ -193,10 +182,20 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
                max_picture_width, max_picture_height, avg_frame_rate);
     }
 
-    ret = vvc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
-                                           &new_extradata_size);
-    if (ret < 0)
-        return ret;
+    while (1) {
+        GetByteContext gb_bak = gb;
+        ret = vvc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
+                                               &new_extradata_size);
+        if (ret < 0)
+            return ret;
+        if (new_extradata || !new_extradata_size)
+            break;
+        new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
+        if (!new_extradata)
+            return AVERROR(ENOMEM);
+        memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+        gb = gb_bak;
+    }
 
     av_freep(&ctx->par_out->extradata);
     ctx->par_out->extradata = new_extradata;
-- 
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/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX

[James Almer]

On 2/17/2024 11:41 PM, Andreas Rheinhardt wrote:
> AVCodecParameters.extradata_size is an int.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/bsf/hevc_mp4toannexb.c | 2 +-
>   libavcodec/bsf/vvc_mp4toannexb.c  | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
> index 8eec18f31e..c0df2b79a6 100644
> --- a/libavcodec/bsf/hevc_mp4toannexb.c
> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
> @@ -69,7 +69,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
>   
>               if (!nalu_len ||
>                   nalu_len > bytestream2_get_bytes_left(&gb) ||
> -                4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) {
> +                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
>                   ret = AVERROR_INVALIDDATA;
>                   goto fail;
>               }
> diff --git a/libavcodec/bsf/vvc_mp4toannexb.c b/libavcodec/bsf/vvc_mp4toannexb.c
> index 36bdae8f49..1b851f3223 100644
> --- a/libavcodec/bsf/vvc_mp4toannexb.c
> +++ b/libavcodec/bsf/vvc_mp4toannexb.c
> @@ -159,7 +159,7 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
>   
>               if (!nalu_len ||
>                   nalu_len > bytestream2_get_bytes_left(&gb) ||
> -                4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len > SIZE_MAX - new_extradata_size) {
> +                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {

Just use INT_MAX, there's no point in this check. Do you expect a system 
where an int is smaller than the type meant to store size of buffers in 
memory?

>                   ret = AVERROR_INVALIDDATA;
>                   goto fail;
>               }
_______________________________________________
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/5] avcodec/bsf/(hevc|vvc)_mp4toannexb: Ensure extradata_size < INT_MAX

[Andreas Rheinhardt]

James Almer:
> On 2/17/2024 11:41 PM, Andreas Rheinhardt wrote:
>> AVCodecParameters.extradata_size is an int.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavcodec/bsf/hevc_mp4toannexb.c | 2 +-
>>   libavcodec/bsf/vvc_mp4toannexb.c  | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c
>> b/libavcodec/bsf/hevc_mp4toannexb.c
>> index 8eec18f31e..c0df2b79a6 100644
>> --- a/libavcodec/bsf/hevc_mp4toannexb.c
>> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
>> @@ -69,7 +69,7 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
>>                 if (!nalu_len ||
>>                   nalu_len > bytestream2_get_bytes_left(&gb) ||
>> -                4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len >
>> SIZE_MAX - new_extradata_size) {
>> +                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) -
>> AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
>>                   ret = AVERROR_INVALIDDATA;
>>                   goto fail;
>>               }
>> diff --git a/libavcodec/bsf/vvc_mp4toannexb.c
>> b/libavcodec/bsf/vvc_mp4toannexb.c
>> index 36bdae8f49..1b851f3223 100644
>> --- a/libavcodec/bsf/vvc_mp4toannexb.c
>> +++ b/libavcodec/bsf/vvc_mp4toannexb.c
>> @@ -159,7 +159,7 @@ static int vvc_extradata_to_annexb(AVBSFContext *ctx)
>>                 if (!nalu_len ||
>>                   nalu_len > bytestream2_get_bytes_left(&gb) ||
>> -                4 + AV_INPUT_BUFFER_PADDING_SIZE + nalu_len >
>> SIZE_MAX - new_extradata_size) {
>> +                4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) -
>> AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
> 
> Just use INT_MAX, there's no point in this check. Do you expect a system
> where an int is smaller than the type meant to store size of buffers in
> memory?
> 

We typically try to avoid such assumptions (although we actually have
lots of INT_MAX <= SIZE_MAX assumptions in the codebase, namely lots of
instances where one uses an int and an allocation function).
In this case using FFMIN(INT_MAX, SIZE_MAX) will make it easier to find
these lines via git grep when extradata_size is switched to size_t.
Alternatively, one could add an AV_CODECPAR_MAX_EXTRADATA_SIZE define,
but this is even longer than FFMIN(INT_MAX, SIZE_MAX).

- 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 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

[James Almer]

On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote:
> AVCodecParameters.extradata is supposed to be allocated with
> av_malloc(); av_realloc() and its wrappers do not guarantee
> the proper alignment. Therefore parse the extradata twice:
> Once to check its validity and to determine the eventual size
> and a second time to actually write the new extradata.

Why would av_malloc alignment be needed for extradata?
I see the doxy says "Must be allocated with av_malloc()" but I'm fairly 
sure that was meant to be "Must be allocated with av_malloc() family of 
functions", like its AVCodecContext counterpart. The idea is that 
library users don't use normal malloc as extradata will be freed with 
av_free(), and not because it will be accessed by SIMD code.

> 
> (Of course, not reallocating the buffer is beneficial in itself.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/bsf/hevc_mp4toannexb.c | 44 +++++++++++++++----------------
>   1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/libavcodec/bsf/hevc_mp4toannexb.c b/libavcodec/bsf/hevc_mp4toannexb.c
> index a695cba370..f5424e95b8 100644
> --- a/libavcodec/bsf/hevc_mp4toannexb.c
> +++ b/libavcodec/bsf/hevc_mp4toannexb.c
> @@ -38,13 +38,11 @@ typedef struct HEVCBSFContext {
>   } HEVCBSFContext;
>   
>   static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
> -                                             uint8_t **new_extradatap,
> +                                             uint8_t *new_extradata,
>                                                size_t *new_extradata_sizep)
>   {
>       int num_arrays = bytestream2_get_byte(gb);
> -    uint8_t *new_extradata = NULL;
>       size_t new_extradata_size = 0;
> -    int ret;
>   
>       for (int i = 0; i < num_arrays; i++) {
>           int type = bytestream2_get_byte(gb) & 0x3f;
> @@ -54,8 +52,7 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
>                 type == HEVC_NAL_SEI_PREFIX || type == HEVC_NAL_SEI_SUFFIX)) {
>               av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit type in extradata: %d\n",
>                      type);
> -            ret = AVERROR_INVALIDDATA;
> -            goto fail;
> +            return AVERROR_INVALIDDATA;
>           }
>   
>           for (int j = 0; j < cnt; j++) {
> @@ -64,26 +61,19 @@ static int hevc_extradata_to_annexb_internal(void *logctx, GetByteContext *gb,
>               if (!nalu_len ||
>                   nalu_len > bytestream2_get_bytes_left(gb) ||
>                   4 + nalu_len > FFMIN(INT_MAX, SIZE_MAX) - AV_INPUT_BUFFER_PADDING_SIZE - new_extradata_size) {
> -                ret = AVERROR_INVALIDDATA;
> -                goto fail;
> +                return AVERROR_INVALIDDATA;
>               }
> -            ret = av_reallocp(&new_extradata, new_extradata_size + nalu_len + 4 + AV_INPUT_BUFFER_PADDING_SIZE);
> -            if (ret < 0)
> -                goto fail;
> -
> -            AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
> -            bytestream2_get_buffer(gb, new_extradata + new_extradata_size + 4, nalu_len);
> +            if (new_extradata) {
> +                AV_WB32(new_extradata + new_extradata_size, 1); // add the startcode
> +                bytestream2_get_bufferu(gb, new_extradata + new_extradata_size + 4, nalu_len);
> +            } else
> +                bytestream2_skipu(gb, nalu_len);
>               new_extradata_size += 4 + nalu_len;
> -            memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>           }
>       }
> -    *new_extradatap = new_extradata;
>       *new_extradata_sizep = new_extradata_size;
>   
>       return 0;
> -fail:
> -    av_freep(&new_extradata);
> -    return ret;
>   }
>   
>   static int hevc_extradata_to_annexb(AVBSFContext *ctx)
> @@ -100,10 +90,20 @@ static int hevc_extradata_to_annexb(AVBSFContext *ctx)
>       bytestream2_skip(&gb, 21);
>       length_size = (bytestream2_get_byte(&gb) & 3) + 1;
>   
> -    ret = hevc_extradata_to_annexb_internal(ctx, &gb, &new_extradata,
> -                                            &new_extradata_size);
> -    if (ret < 0)
> -        return ret;
> +    while (1) {
> +        GetByteContext gb_bak = gb;
> +        ret = hevc_extradata_to_annexb_internal(ctx, &gb, new_extradata,
> +                                                &new_extradata_size);
> +        if (ret < 0)
> +            return ret;
> +        if (new_extradata || !new_extradata_size)
> +            break;
> +        new_extradata = av_malloc(new_extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!new_extradata)
> +            return AVERROR(ENOMEM);
> +        memset(new_extradata + new_extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> +        gb = gb_bak;
> +    }
>   
>       av_freep(&ctx->par_out->extradata);
>       ctx->par_out->extradata      = new_extradata;
_______________________________________________
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 3/5] avcodec/bsf/hevc_mp4toannexb: Don't realloc when creating new extradata

[Andreas Rheinhardt]

James Almer:
> On 2/17/2024 11:44 PM, Andreas Rheinhardt wrote:
>> AVCodecParameters.extradata is supposed to be allocated with
>> av_malloc(); av_realloc() and its wrappers do not guarantee
>> the proper alignment. Therefore parse the extradata twice:
>> Once to check its validity and to determine the eventual size
>> and a second time to actually write the new extradata.
> 
> Why would av_malloc alignment be needed for extradata?
> I see the doxy says "Must be allocated with av_malloc()" but I'm fairly
> sure that was meant to be "Must be allocated with av_malloc() family of
> functions", like its AVCodecContext counterpart. The idea is that
> library users don't use normal malloc as extradata will be freed with
> av_free(), and not because it will be accessed by SIMD code.
> 

I thought that potential accesses by SIMD code was the point? After all,
the value of AV_INPUT_BUFFER_PADDING_SIZE (which is used for both packet
data and extradata) is chosen so big to accommodate reading via SIMD.
You incremented the constant for this very purpose in 6e80079a28.
(Granted, I don't think we have code where extradata is being parsed by
SIMD.)

Apart from that, given its differing alignment, I am not sure that
av_realloc() is really part of the av_malloc() family of functions. We
should probably replace "av_malloc() family" by "compatible with
av_free()" wherever we want to allow av_realloc(), too.

Anyway, there is another advantage of this patch:
>>
>> (Of course, not reallocating the buffer is beneficial in itself.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>

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