[FFmpeg-devel] [PATCH] avformat/matroskaenc: Fix potential stack-buffer-overflow

[FFmpeg-devel] [PATCH] avformat/matroskaenc: Fix potential stack-buffer-overflow

[Andreas Rheinhardt]

Forgotten in f194f291d8082134cdce625b0ca3548067428ea9

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskaenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 216cb48884..e1adc0eba6 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1758,7 +1758,7 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
     uint8_t color_space[4], projection_private[20];
     const AVPacketSideData *sd;
     EBML_WRITER(MAX_FIELD_ORDER_ELEMS + MAX_STEREO_MODE_ELEMS      +
-                MAX_VIDEO_COLOR_ELEMS + MAX_VIDEO_PROJECTION_ELEMS + 8);
+                MAX_VIDEO_COLOR_ELEMS + MAX_VIDEO_PROJECTION_ELEMS + 12);
     int cropped_width = par->width, cropped_height = par->height;
     int ret;
 
-- 
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/2] avformat/matroskaenc: Fix and simplify check for invalid crop values

[Andreas Rheinhardt]

The check "left >= INT_MAX - right" is supposed to check for
whether left + right does not overflow/wraparound, but given that
left and top are uint32_t INT_MAX - right can already wraparound
for big values of right (and ordinary 32-bit ints):
If right == UINT32_MAX, INT_MAX - right is INT_MAX + 1;
for left in 0..par->width both checks will be passed.

Fix this and simplify the check by using 64-bit types,
where the addition is guaranteed not to overflow.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskaenc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index e1adc0eba6..4b91283119 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1786,16 +1786,14 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
                                  st->codecpar->nb_coded_side_data,
                                  AV_PKT_DATA_FRAME_CROPPING);
     if (sd && sd->size == sizeof(uint32_t) * 4) {
-        uint32_t top, bottom, left, right;
+        uint64_t top, bottom, left, right;
 
         top    = AV_RL32(sd->data +  0);
         bottom = AV_RL32(sd->data +  4);
         left   = AV_RL32(sd->data +  8);
         right  = AV_RL32(sd->data + 12);
 
-        if (left >= INT_MAX - right ||
-            top >= INT_MAX - bottom ||
-            (left + right) >= par->width ||
+        if ((left + right) >= par->width ||
             (top + bottom) >= par->height) {
             av_log(s, AV_LOG_ERROR, "Invalid cropping dimensions in stream side data\n");
             return AVERROR(EINVAL);
-- 
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 2/2] avformat/matroskaenc: Fix and simplify check for invalid crop values

[James Almer]

On 7/8/2024 2:24 PM, Andreas Rheinhardt wrote:
> The check "left >= INT_MAX - right" is supposed to check for
> whether left + right does not overflow/wraparound, but given that
> left and top are uint32_t INT_MAX - right can already wraparound
> for big values of right (and ordinary 32-bit ints):
> If right == UINT32_MAX, INT_MAX - right is INT_MAX + 1;
> for left in 0..par->width both checks will be passed.
> 
> Fix this and simplify the check by using 64-bit types,
> where the addition is guaranteed not to overflow.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/matroskaenc.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index e1adc0eba6..4b91283119 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1786,16 +1786,14 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
>                                    st->codecpar->nb_coded_side_data,
>                                    AV_PKT_DATA_FRAME_CROPPING);
>       if (sd && sd->size == sizeof(uint32_t) * 4) {
> -        uint32_t top, bottom, left, right;
> +        uint64_t top, bottom, left, right;
>   
>           top    = AV_RL32(sd->data +  0);
>           bottom = AV_RL32(sd->data +  4);
>           left   = AV_RL32(sd->data +  8);
>           right  = AV_RL32(sd->data + 12);
>   
> -        if (left >= INT_MAX - right ||
> -            top >= INT_MAX - bottom ||
> -            (left + right) >= par->width ||
> +        if ((left + right) >= par->width ||
>               (top + bottom) >= par->height) {
>               av_log(s, AV_LOG_ERROR, "Invalid cropping dimensions in stream side data\n");
>               return AVERROR(EINVAL);

LGTM.
_______________________________________________
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/4] avformat/matroskaenc: Only write useful cropping values

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskaenc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 4b91283119..a98d33abf3 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1799,10 +1799,14 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
             return AVERROR(EINVAL);
         }
 
-        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPB, bottom);
-        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPT, top);
-        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPL, left);
-        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPR, right);
+        if (bottom)
+            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPB, bottom);
+        if (top)
+            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPT, top);
+        if (left)
+            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPL, left);
+        if (right)
+            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPR, right);
 
         cropped_width  -= left + right;
         cropped_height -= top + bottom;
-- 
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 3/4] avformat/matroskaenc: Only write useful cropping values

[James Almer]

On 7/8/2024 2:52 PM, Andreas Rheinhardt wrote:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/matroskaenc.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 4b91283119..a98d33abf3 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1799,10 +1799,14 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
>               return AVERROR(EINVAL);
>           }
>   
> -        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPB, bottom);
> -        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPT, top);
> -        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPL, left);
> -        ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPR, right);
> +        if (bottom)
> +            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPB, bottom);
> +        if (top)
> +            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPT, top);
> +        if (left)
> +            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPL, left);
> +        if (right)
> +            ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOPIXELCROPR, right);
>   
>           cropped_width  -= left + right;
>           cropped_height -= top + bottom;

LGTM.
_______________________________________________
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/4] avformat/matroskaenc: Avoid indirection via st->codecpar

[Andreas Rheinhardt]

Use the already available AVCodecParameters pointer instead.
Shortens lines.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskaenc.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index a98d33abf3..60c896e964 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1369,7 +1369,7 @@ static void mkv_write_video_color(EbmlWriter *writer, const AVStream *st,
                              (ypos >> 7) + 1);
     }
 
-    side_data = av_packet_side_data_get(st->codecpar->coded_side_data, st->codecpar->nb_coded_side_data,
+    side_data = av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
                                         AV_PKT_DATA_CONTENT_LIGHT_LEVEL);
     if (side_data) {
         const AVContentLightMetadata *metadata = (AVContentLightMetadata *)side_data->data;
@@ -1379,7 +1379,7 @@ static void mkv_write_video_color(EbmlWriter *writer, const AVStream *st,
                              metadata->MaxFALL);
     }
 
-    side_data = av_packet_side_data_get(st->codecpar->coded_side_data, st->codecpar->nb_coded_side_data,
+    side_data = av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
                                         AV_PKT_DATA_MASTERING_DISPLAY_METADATA);
     if (side_data) {
         const AVMasteringDisplayMetadata *metadata = (AVMasteringDisplayMetadata *)side_data->data;
@@ -1415,12 +1415,12 @@ static void mkv_write_video_color(EbmlWriter *writer, const AVStream *st,
 }
 
 #define MAX_VIDEO_PROJECTION_ELEMS 6
-static void mkv_handle_rotation(void *logctx, const AVStream *st,
+static void mkv_handle_rotation(void *logctx, const AVCodecParameters *par,
                                 double *yaw, double *roll)
 {
     const int32_t *matrix;
     const AVPacketSideData *side_data =
-        av_packet_side_data_get(st->codecpar->coded_side_data, st->codecpar->nb_coded_side_data,
+        av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
                                 AV_PKT_DATA_DISPLAYMATRIX);
 
     if (!side_data)
@@ -1471,11 +1471,11 @@ ignore:
 }
 
 static int mkv_handle_spherical(void *logctx, EbmlWriter *writer,
-                                const AVStream *st, uint8_t private[],
+                                const AVCodecParameters *par, uint8_t private[],
                                 double *yaw, double *pitch, double *roll)
 {
-    const AVPacketSideData *sd = av_packet_side_data_get(st->codecpar->coded_side_data,
-                                                         st->codecpar->nb_coded_side_data,
+    const AVPacketSideData *sd = av_packet_side_data_get(par->coded_side_data,
+                                                         par->nb_coded_side_data,
                                                          AV_PKT_DATA_SPHERICAL);
     const AVSphericalMapping *spherical;
 
@@ -1531,16 +1531,17 @@ static int mkv_handle_spherical(void *logctx, EbmlWriter *writer,
 }
 
 static void mkv_write_video_projection(void *logctx, EbmlWriter *wr,
-                                       const AVStream *st, uint8_t private[])
+                                       const AVCodecParameters *par,
+                                       uint8_t private[])
 {
     double yaw = 0, pitch = 0, roll = 0;
     int ret;
 
     ebml_writer_open_master(wr, MATROSKA_ID_VIDEOPROJECTION);
 
-    ret = mkv_handle_spherical(logctx, wr, st, private, &yaw, &pitch, &roll);
+    ret = mkv_handle_spherical(logctx, wr, par, private, &yaw, &pitch, &roll);
     if (!ret)
-        mkv_handle_rotation(logctx, st, &yaw, &roll);
+        mkv_handle_rotation(logctx, par, &yaw, &roll);
 
     if (yaw)
         ebml_writer_add_float(wr, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, yaw);
@@ -1594,6 +1595,7 @@ static void mkv_write_field_order(EbmlWriter *writer, int is_webm,
 
 #define MAX_STEREO_MODE_ELEMS 1
 static int mkv_write_stereo_mode(AVFormatContext *s, EbmlWriter *writer,
+                                 const AVCodecParameters *par,
                                  const AVStream *st, int is_webm,
                                  int *h_width, int *h_height)
 {
@@ -1654,7 +1656,7 @@ static int mkv_write_stereo_mode(AVFormatContext *s, EbmlWriter *writer,
         };
         int fmt;
 
-        sd = av_packet_side_data_get(st->codecpar->coded_side_data, st->codecpar->nb_coded_side_data,
+        sd = av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
                                      AV_PKT_DATA_STEREO3D);
         if (!sd)
             return 0;
@@ -1715,7 +1717,7 @@ static void mkv_write_blockadditionmapping(AVFormatContext *s, const MatroskaMux
         }
     }
 
-    sd = av_packet_side_data_get(st->codecpar->coded_side_data, st->codecpar->nb_coded_side_data,
+    sd = av_packet_side_data_get(par->coded_side_data, par->nb_coded_side_data,
                                  AV_PKT_DATA_DOVI_CONF);
 
     if (!sd)
@@ -1771,7 +1773,7 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
 
     // check both side data and metadata for stereo information,
     // write the result to the bitstream if any is found
-    ret = mkv_write_stereo_mode(s, &writer, st, IS_WEBM(mkv),
+    ret = mkv_write_stereo_mode(s, &writer, par, st, IS_WEBM(mkv),
                                 &display_width_div,
                                 &display_height_div);
     if (ret < 0)
@@ -1782,8 +1784,8 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
          (tag = av_dict_get( s->metadata, "alpha_mode", NULL, 0))) && strtol(tag->value, NULL, 0))
         ebml_writer_add_uint(&writer, MATROSKA_ID_VIDEOALPHAMODE, 1);
 
-    sd = av_packet_side_data_get(st->codecpar->coded_side_data,
-                                 st->codecpar->nb_coded_side_data,
+    sd = av_packet_side_data_get(par->coded_side_data,
+                                 par->nb_coded_side_data,
                                  AV_PKT_DATA_FRAME_CROPPING);
     if (sd && sd->size == sizeof(uint32_t) * 4) {
         uint64_t top, bottom, left, right;
@@ -1855,7 +1857,7 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
                             color_space, sizeof(color_space));
     }
     mkv_write_video_color(&writer, st, par);
-    mkv_write_video_projection(s, &writer, st, projection_private);
+    mkv_write_video_projection(s, &writer, par, projection_private);
 
     return ebml_writer_write(&writer, pb);
 }
-- 
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] avformat/matroskaenc: Fix potential stack-buffer-overflow

[James Almer]

On 7/8/2024 2:09 PM, Andreas Rheinhardt wrote:
> Forgotten in f194f291d8082134cdce625b0ca3548067428ea9
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/matroskaenc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 216cb48884..e1adc0eba6 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1758,7 +1758,7 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
>       uint8_t color_space[4], projection_private[20];
>       const AVPacketSideData *sd;
>       EBML_WRITER(MAX_FIELD_ORDER_ELEMS + MAX_STEREO_MODE_ELEMS      +
> -                MAX_VIDEO_COLOR_ELEMS + MAX_VIDEO_PROJECTION_ELEMS + 8);
> +                MAX_VIDEO_COLOR_ELEMS + MAX_VIDEO_PROJECTION_ELEMS + 12);
>       int cropped_width = par->width, cropped_height = par->height;
>       int ret;
>   

LGTM, thanks.
_______________________________________________
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".