| Message ID | 20240401205607.9093-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | d157725cf726adc29385d264eaf79ae430b1f3e5 |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avformat/isom: Uninit layout in ff_mp4_read_dec_config_descr() | 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 4/1/2024 5:56 PM, Michael Niedermayer wrote: > Fixes: memleak > Fixes: 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/isom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavformat/isom.c b/libavformat/isom.c > index 9fbccd4437f..c5930bd4d87 100644 > --- a/libavformat/isom.c > +++ b/libavformat/isom.c > @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext *fc, AVStream *st, AVIOContext > st->codecpar->extradata_size, 1, fc); > if (ret < 0) > return ret; > + av_channel_layout_uninit(&st->codecpar->ch_layout); Why is there a custom layout here to begin with? The caf demuxer doesn't export one. > st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; > st->codecpar->ch_layout.nb_channels = cfg.channels; > if (cfg.object_type == 29 && cfg.sampling_index < 3) // old mp3on4
On 4/1/2024 6:25 PM, James Almer wrote: > On 4/1/2024 5:56 PM, Michael Niedermayer wrote: >> Fixes: memleak >> Fixes: >> 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 >> >> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/isom.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/libavformat/isom.c b/libavformat/isom.c >> index 9fbccd4437f..c5930bd4d87 100644 >> --- a/libavformat/isom.c >> +++ b/libavformat/isom.c >> @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext >> *fc, AVStream *st, AVIOContext >> >> st->codecpar->extradata_size, 1, fc); >> if (ret < 0) >> return ret; >> + av_channel_layout_uninit(&st->codecpar->ch_layout); > > Why is there a custom layout here to begin with? The caf demuxer doesn't > export one. Nevermind, it calls ff_mov_read_chan() which may generate one. Patch LGTM.
On 4/1/2024 5:56 PM, Michael Niedermayer wrote: > Fixes: memleak > Fixes: 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/isom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavformat/isom.c b/libavformat/isom.c > index 9fbccd4437f..c5930bd4d87 100644 > --- a/libavformat/isom.c > +++ b/libavformat/isom.c > @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext *fc, AVStream *st, AVIOContext > st->codecpar->extradata_size, 1, fc); > if (ret < 0) > return ret; > + av_channel_layout_uninit(&st->codecpar->ch_layout); > st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; Not strictly related to this fix, but should we really drop the layout here by forcing unspec? We're also not even bothering to check if cfg.channels matches st->codecpar->ch_layout.nb_channels. > st->codecpar->ch_layout.nb_channels = cfg.channels; > if (cfg.object_type == 29 && cfg.sampling_index < 3) // old mp3on4
On Mon, Apr 01, 2024 at 06:33:22PM -0300, James Almer wrote: > On 4/1/2024 5:56 PM, Michael Niedermayer wrote: > > Fixes: memleak > > Fixes: 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/isom.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavformat/isom.c b/libavformat/isom.c > > index 9fbccd4437f..c5930bd4d87 100644 > > --- a/libavformat/isom.c > > +++ b/libavformat/isom.c > > @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext *fc, AVStream *st, AVIOContext > > st->codecpar->extradata_size, 1, fc); > > if (ret < 0) > > return ret; > > + av_channel_layout_uninit(&st->codecpar->ch_layout); > > st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; > > Not strictly related to this fix, but should we really drop the layout here > by forcing unspec? We're also not even bothering to check if cfg.channels > matches st->codecpar->ch_layout.nb_channels. I was wondering the same. I was hoping someone would know the specs well enouh to just say straight "this and that isnt legal together error out if thats true" thx [...]
On 4/1/2024 8:40 PM, Michael Niedermayer wrote: > On Mon, Apr 01, 2024 at 06:33:22PM -0300, James Almer wrote: >> On 4/1/2024 5:56 PM, Michael Niedermayer wrote: >>> Fixes: memleak >>> Fixes: 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/isom.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/libavformat/isom.c b/libavformat/isom.c >>> index 9fbccd4437f..c5930bd4d87 100644 >>> --- a/libavformat/isom.c >>> +++ b/libavformat/isom.c >>> @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext *fc, AVStream *st, AVIOContext >>> st->codecpar->extradata_size, 1, fc); >>> if (ret < 0) >>> return ret; >>> + av_channel_layout_uninit(&st->codecpar->ch_layout); >>> st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; >> >> Not strictly related to this fix, but should we really drop the layout here >> by forcing unspec? We're also not even bothering to check if cfg.channels >> matches st->codecpar->ch_layout.nb_channels. > > I was wondering the same. > I was hoping someone would know the specs well enouh to just say straight > "this and that isnt legal together error out if thats true" Maybe Marton knows, since he's been working on this code.
On Mon, 1 Apr 2024, James Almer wrote: > On 4/1/2024 8:40 PM, Michael Niedermayer wrote: >> On Mon, Apr 01, 2024 at 06:33:22PM -0300, James Almer wrote: >>> On 4/1/2024 5:56 PM, Michael Niedermayer wrote: >>>> Fixes: memleak >>>> Fixes: 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 >>>> >>>> Found-by: continuous fuzzing process >>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> --- >>>> libavformat/isom.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/libavformat/isom.c b/libavformat/isom.c >>>> index 9fbccd4437f..c5930bd4d87 100644 >>>> --- a/libavformat/isom.c >>>> +++ b/libavformat/isom.c >>>> @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext >>>> *fc, AVStream *st, AVIOContext >>>> st->codecpar->extradata_size, >>>> 1, fc); >>>> if (ret < 0) >>>> return ret; >>>> + av_channel_layout_uninit(&st->codecpar->ch_layout); >>>> st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; >>> >>> Not strictly related to this fix, but should we really drop the layout >>> here >>> by forcing unspec? We're also not even bothering to check if cfg.channels >>> matches st->codecpar->ch_layout.nb_channels. >> >> I was wondering the same. >> I was hoping someone would know the specs well enouh to just say straight >> "this and that isnt legal together error out if thats true" > > Maybe Marton knows, since he's been working on this code. I don't really know for sure. But it would make sense to me to only drop the layout if the channel count is different from what we already know. Regards, Marton
On 4/2/2024 7:22 PM, Marton Balint wrote: > > > On Mon, 1 Apr 2024, James Almer wrote: > >> On 4/1/2024 8:40 PM, Michael Niedermayer wrote: >>> On Mon, Apr 01, 2024 at 06:33:22PM -0300, James Almer wrote: >>>> On 4/1/2024 5:56 PM, Michael Niedermayer wrote: >>>>> Fixes: memleak >>>>> Fixes: >>>>> 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 >>>>> >>>>> Found-by: continuous fuzzing process >>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>>> --- >>>>> libavformat/isom.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/libavformat/isom.c b/libavformat/isom.c >>>>> index 9fbccd4437f..c5930bd4d87 100644 >>>>> --- a/libavformat/isom.c >>>>> +++ b/libavformat/isom.c >>>>> @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext >>>>> *fc, AVStream *st, AVIOContext >>>>> >>>>> st->codecpar->extradata_size, >>>>> 1, fc); >>>>> if (ret < 0) >>>>> return ret; >>>>> + av_channel_layout_uninit(&st->codecpar->ch_layout); >>>>> st->codecpar->ch_layout.order = >>>>> AV_CHANNEL_ORDER_UNSPEC; >>>> >>>> Not strictly related to this fix, but should we really drop the layout >>>> here >>>> by forcing unspec? We're also not even bothering to check if >>>> cfg.channels >>>> matches st->codecpar->ch_layout.nb_channels. >>> >>> I was wondering the same. >>> I was hoping someone would know the specs well enouh to just say >>> straight >>> "this and that isnt legal together error out if thats true" >> >> Maybe Marton knows, since he's been working on this code. > > I don't really know for sure. But it would make sense to me to only drop > the layout if the channel count is different from what we already know. Ok, just did that, and also applied Michael's memleak fix. Thanks.
diff --git a/libavformat/isom.c b/libavformat/isom.c index 9fbccd4437f..c5930bd4d87 100644 --- a/libavformat/isom.c +++ b/libavformat/isom.c @@ -359,6 +359,7 @@ int ff_mp4_read_dec_config_descr(AVFormatContext *fc, AVStream *st, AVIOContext st->codecpar->extradata_size, 1, fc); if (ret < 0) return ret; + av_channel_layout_uninit(&st->codecpar->ch_layout); st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; st->codecpar->ch_layout.nb_channels = cfg.channels; if (cfg.object_type == 29 && cfg.sampling_index < 3) // old mp3on4
Fixes: memleak Fixes: 67442/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5068813261406208 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/isom.c | 1 + 1 file changed, 1 insertion(+)