| Message ID | 20220103155919.12015-1-pal@sandflow.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v1] avformat/imf: fix bad free() when directory name of the input url is empty | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | fail | Make fate failed |
On 4/1/22 01:59, pal@sandflow.com wrote: > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com> > --- > > Notes: > Found through manual fuzzing. > > libavformat/imfdec.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > index f17064cfcd..4e42db8d30 100644 > --- a/libavformat/imfdec.c > +++ b/libavformat/imfdec.c > @@ -622,11 +622,15 @@ static int imf_read_header(AVFormatContext *s) > int ret = 0; > > c->interrupt_callback = &s->interrupt_callback; > + > tmp_str = av_strdup(s->url); > if (!tmp_str) > return AVERROR(ENOMEM); > + c->base_url = av_strdup(av_dirname(tmp_str)); Is the second av_strdup() here required? You've already done it above and av_dirname() just sticks a '\0' at the last separator, so it should be safe to remove it: if (!(c->base_url = av_strdup(s->url))) return AVERROR(ENOMEM); c->base_url = av_dirname(c->base_url);
On Tue, Jan 4, 2022 at 5:39 PM Zane van Iperen <zane@zanevaniperen.com> wrote: > > > > On 4/1/22 01:59, pal@sandflow.com wrote: > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com> > > --- > > > > Notes: > > Found through manual fuzzing. > > > > libavformat/imfdec.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > index f17064cfcd..4e42db8d30 100644 > > --- a/libavformat/imfdec.c > > +++ b/libavformat/imfdec.c > > @@ -622,11 +622,15 @@ static int imf_read_header(AVFormatContext *s) > > int ret = 0; > > > > c->interrupt_callback = &s->interrupt_callback; > > + > > tmp_str = av_strdup(s->url); > > if (!tmp_str) > > return AVERROR(ENOMEM); > > + c->base_url = av_strdup(av_dirname(tmp_str)); > > Is the second av_strdup() here required? You've already done it above > and av_dirname() just sticks a '\0' at the last separator, This is what I thought. > so it should > be safe to remove it: As I understand it, av_dirname() actually returns a pointer to its own "." string when the input is either empty or does not contain, in which case we must make a copy. > > if (!(c->base_url = av_strdup(s->url))) > return AVERROR(ENOMEM); > > c->base_url = av_dirname(c->base_url); > _______________________________________________ > 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".
On 5/1/22 11:44, Pierre-Anthony Lemieux wrote: > On Tue, Jan 4, 2022 at 5:39 PM Zane van Iperen <zane@zanevaniperen.com> wrote: >> >> >> >> On 4/1/22 01:59, pal@sandflow.com wrote: >>> From: Pierre-Anthony Lemieux <pal@palemieux.com> >>> >>> Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com> >>> --- >>> >>> Notes: >>> Found through manual fuzzing. >>> >>> libavformat/imfdec.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c >>> index f17064cfcd..4e42db8d30 100644 >>> --- a/libavformat/imfdec.c >>> +++ b/libavformat/imfdec.c >>> @@ -622,11 +622,15 @@ static int imf_read_header(AVFormatContext *s) >>> int ret = 0; >>> >>> c->interrupt_callback = &s->interrupt_callback; >>> + >>> tmp_str = av_strdup(s->url); >>> if (!tmp_str) >>> return AVERROR(ENOMEM); >>> + c->base_url = av_strdup(av_dirname(tmp_str)); >> >> Is the second av_strdup() here required? You've already done it above >> and av_dirname() just sticks a '\0' at the last separator, > > This is what I thought. > >> so it should >> be safe to remove it: > > As I understand it, av_dirname() actually returns a pointer to its own > "." string when the input is either empty or does not contain, in > which case we must make a copy. > You're right. This is ugly, but I don't see a nicer way to do it. This lgtm then.
diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c index f17064cfcd..4e42db8d30 100644 --- a/libavformat/imfdec.c +++ b/libavformat/imfdec.c @@ -622,11 +622,15 @@ static int imf_read_header(AVFormatContext *s) int ret = 0; c->interrupt_callback = &s->interrupt_callback; + tmp_str = av_strdup(s->url); if (!tmp_str) return AVERROR(ENOMEM); + c->base_url = av_strdup(av_dirname(tmp_str)); + av_freep(&tmp_str); + if (!c->base_url) + return AVERROR(ENOMEM); - c->base_url = av_dirname(tmp_str); if ((ret = ffio_copy_url_options(s->pb, &c->avio_opts)) < 0) return ret;