[FFmpeg-devel] [PATCH v2 1/2] Revert "avformat/mov: zero initialize the index ranges buffer"

[FFmpeg-devel] [PATCH v2 1/2] Revert "avformat/mov: zero initialize the index ranges buffer"

[Andreas Rheinhardt]

This reverts commit eee3b7e2fb0664b889e58c03f76e19ef23e4d1d8.
It has been made in an attempt to fix UBSan test failures with
GCC 13 (see e.g. [1]), but it did not help at all. So revert it,
but use av_malloc_array() instead of going back to av_malloc().

[1]: https://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-ubsan&time=20240226182430

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

diff --git a/libavformat/mov.c b/libavformat/mov.c
index effa46120f..71e8f7ae8f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4031,7 +4031,8 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
     }
 
     // allocate the index ranges array
-    msc->index_ranges = av_calloc((msc->elst_count + 1), sizeof(msc->index_ranges[0]));
+    msc->index_ranges = av_malloc_array(msc->elst_count + 1,
+                                        sizeof(msc->index_ranges[0]));
     if (!msc->index_ranges) {
         av_log(mov->fc, AV_LOG_ERROR, "Cannot allocate index ranges buffer\n");
         return;
-- 
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 v2 1/2] Revert "avformat/mov: zero initialize the index ranges buffer"

[James Almer]

On 2/26/2024 7:11 PM, Andreas Rheinhardt wrote:
> This reverts commit eee3b7e2fb0664b889e58c03f76e19ef23e4d1d8.
> It has been made in an attempt to fix UBSan test failures with
> GCC 13 (see e.g. [1]), but it did not help at all. So revert it,
> but use av_malloc_array() instead of going back to av_malloc().
> 
> [1]: https://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-ubsan&time=20240226182430
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/mov.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index effa46120f..71e8f7ae8f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4031,7 +4031,8 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
>       }
>   
>       // allocate the index ranges array
> -    msc->index_ranges = av_calloc((msc->elst_count + 1), sizeof(msc->index_ranges[0]));
> +    msc->index_ranges = av_malloc_array(msc->elst_count + 1,
> +                                        sizeof(msc->index_ranges[0]));
>       if (!msc->index_ranges) {
>           av_log(mov->fc, AV_LOG_ERROR, "Cannot allocate index ranges buffer\n");
>           return;

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 v2 2/2] avformat/mov: Don't use entry[-1] in pointer arithmetic

[Andreas Rheinhardt]

It is undefined behaviour.
Fixes many failed tests with UBSan and GCC 13 like
"src/libavformat/mov.c:4229:44: runtime error: store to address
0x5572abe20f80 with insufficient space for an object of type 'struct
MOVIndexRange'"
(The line number does not refer to the line where &entry[-1]
is assigned.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Now switching to next_index_range in order to avoid
having to duplicate the current_index_range == NULL check.

 libavformat/mov.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 71e8f7ae8f..d7da19998b 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4022,7 +4022,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
     int num_discarded_begin = 0;
     int first_non_zero_audio_edit = -1;
     int packet_skip_samples = 0;
-    MOVIndexRange *current_index_range;
+    MOVIndexRange *next_index_range;
     int found_keyframe_after_edit = 0;
     int found_non_empty_edit = 0;
 
@@ -4038,7 +4038,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
         return;
     }
     msc->current_index_range = msc->index_ranges;
-    current_index_range = msc->index_ranges - 1;
+    next_index_range = msc->index_ranges;
 
     // Clean AVStream from traces of old index
     sti->index_entries = NULL;
@@ -4225,11 +4225,11 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
             }
 
             // Update the index ranges array
-            if (current_index_range < msc->index_ranges || index != current_index_range->end) {
-                current_index_range++;
-                current_index_range->start = index;
+            if (next_index_range == msc->index_ranges || index != next_index_range[-1].end) {
+                next_index_range->start = index;
+                next_index_range++;
             }
-            current_index_range->end = index + 1;
+            next_index_range[-1].end = index + 1;
 
             // Only start incrementing DTS in frame_duration amounts, when we encounter a frame in edit list.
             if (edit_list_start_encountered > 0) {
@@ -4289,9 +4289,8 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
     av_freep(&frame_duration_buffer);
 
     // Null terminate the index ranges array
-    current_index_range++;
-    current_index_range->start = 0;
-    current_index_range->end = 0;
+    next_index_range->start = 0;
+    next_index_range->end   = 0;
     msc->current_index = msc->index_ranges[0].start;
 }
 
-- 
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 v2 2/2] avformat/mov: Don't use entry[-1] in pointer arithmetic

[James Almer]

On 2/26/2024 7:15 PM, Andreas Rheinhardt wrote:
> It is undefined behaviour.
> Fixes many failed tests with UBSan and GCC 13 like
> "src/libavformat/mov.c:4229:44: runtime error: store to address
> 0x5572abe20f80 with insufficient space for an object of type 'struct
> MOVIndexRange'"
> (The line number does not refer to the line where &entry[-1]
> is assigned.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Now switching to next_index_range in order to avoid
> having to duplicate the current_index_range == NULL check.

I prefer the first version. mov_fix_index() is called once per track 
during init (and then it may not be called depending on user options), 
so it's hardly a bottleneck, and next_index_range[-1] is much less clear 
and intuitive at a quick glance than current_index_range.

> 
>   libavformat/mov.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 71e8f7ae8f..d7da19998b 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4022,7 +4022,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
>       int num_discarded_begin = 0;
>       int first_non_zero_audio_edit = -1;
>       int packet_skip_samples = 0;
> -    MOVIndexRange *current_index_range;
> +    MOVIndexRange *next_index_range;
>       int found_keyframe_after_edit = 0;
>       int found_non_empty_edit = 0;
>   
> @@ -4038,7 +4038,7 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
>           return;
>       }
>       msc->current_index_range = msc->index_ranges;
> -    current_index_range = msc->index_ranges - 1;
> +    next_index_range = msc->index_ranges;
>   
>       // Clean AVStream from traces of old index
>       sti->index_entries = NULL;
> @@ -4225,11 +4225,11 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
>               }
>   
>               // Update the index ranges array
> -            if (current_index_range < msc->index_ranges || index != current_index_range->end) {
> -                current_index_range++;
> -                current_index_range->start = index;
> +            if (next_index_range == msc->index_ranges || index != next_index_range[-1].end) {
> +                next_index_range->start = index;
> +                next_index_range++;
>               }
> -            current_index_range->end = index + 1;
> +            next_index_range[-1].end = index + 1;
>   
>               // Only start incrementing DTS in frame_duration amounts, when we encounter a frame in edit list.
>               if (edit_list_start_encountered > 0) {
> @@ -4289,9 +4289,8 @@ static void mov_fix_index(MOVContext *mov, AVStream *st)
>       av_freep(&frame_duration_buffer);
>   
>       // Null terminate the index ranges array
> -    current_index_range++;
> -    current_index_range->start = 0;
> -    current_index_range->end = 0;
> +    next_index_range->start = 0;
> +    next_index_range->end   = 0;
>       msc->current_index = msc->index_ranges[0].start;
>   }
>   
_______________________________________________
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".