Re: [FFmpeg-devel] [PATCH v3] avformat/mov: Don't use entry[-1] in pointer arithmetic

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

James Almer:
> On 2/26/2024 8:27 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>
>> ---
>>   libavformat/mov.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 71e8f7ae8f..97caaa7723 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 *current_index_range = NULL;
>>       int found_keyframe_after_edit = 0;
>>       int found_non_empty_edit = 0;
>>   @@ -4038,7 +4038,6 @@ static void mov_fix_index(MOVContext *mov,
>> AVStream *st)
>>           return;
>>       }
>>       msc->current_index_range = msc->index_ranges;
>> -    current_index_range = msc->index_ranges - 1;
>>         // Clean AVStream from traces of old index
>>       sti->index_entries = NULL;
>> @@ -4225,8 +4224,9 @@ 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++;
>> +            if (!current_index_range || index !=
>> current_index_range->end) {
>> +                current_index_range = current_index_range ?
>> current_index_range + 1
>> +                                                          :
>> msc->index_ranges;
>>                   current_index_range->start = index;
>>               }
>>               current_index_range->end = index + 1;
>> @@ -4289,7 +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 = current_index_range ? current_index_range + 1
>> +                                              : msc->index_ranges;
>>       current_index_range->start = 0;
>>       current_index_range->end = 0;
>>       msc->current_index = msc->index_ranges[0].start;
> 
> Can't test if this fixes the UB, but the changes lgtm.

It does:
https://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-ubsan&time=20240227105551

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