| Message ID | AS8P250MB07443F8218284A1436E695F28F5A2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 1bb7d5ca9fc6dc1b961014255ada03dee40bac31 |
| Headers | show |
| Series | [FFmpeg-devel,v3] avformat/mov: Don't use entry[-1] in pointer arithmetic | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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
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;
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(-)