| Message ID | GV1P250MB0737496BD19AEE2E6CC073CC8F342@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/rtmpproto: Don't free AVOpt-strings manually, fix crash | 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 |
Andreas Rheinhardt: > Besides being redundant, freeing manually is actually harmful here, > as rtmp_close() may call gen_fcunpublish_stream() which dereferences > rt->playpath. > > Reported-by: Armin Hasitzka <armin@grabyo.com> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavformat/rtmpproto.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c > index 4b01b67d28..b1d73b3d75 100644 > --- a/libavformat/rtmpproto.c > +++ b/libavformat/rtmpproto.c > @@ -2917,9 +2917,6 @@ reconnect: > return 0; > > fail: > - av_freep(&rt->playpath); > - av_freep(&rt->tcurl); > - av_freep(&rt->flashver); > av_dict_free(opts); > rtmp_close(s); > return ret; I am pinging this and explicitly cc'ing Steven Liu, whose commit 991cf95fdeebc3af added the av_freeps to be removed above. Steven, did you just feel that there was missing freeing code for the buffers above or was there an actually confirmed memleak (there shouldn't be)? - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Besides being redundant, freeing manually is actually harmful here, >> as rtmp_close() may call gen_fcunpublish_stream() which dereferences >> rt->playpath. >> >> Reported-by: Armin Hasitzka <armin@grabyo.com> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavformat/rtmpproto.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c >> index 4b01b67d28..b1d73b3d75 100644 >> --- a/libavformat/rtmpproto.c >> +++ b/libavformat/rtmpproto.c >> @@ -2917,9 +2917,6 @@ reconnect: >> return 0; >> >> fail: >> - av_freep(&rt->playpath); >> - av_freep(&rt->tcurl); >> - av_freep(&rt->flashver); >> av_dict_free(opts); >> rtmp_close(s); >> return ret; > > I am pinging this and explicitly cc'ing Steven Liu, whose commit > 991cf95fdeebc3af added the av_freeps to be removed above. Steven, did > you just feel that there was missing freeing code for the buffers above > or was there an actually confirmed memleak (there shouldn't be)? > > - Andreas > Another ping explicitly cc'ing Steven Liu, this time with a more recent email. - Andreas
> On Mar 30, 2024, at 12:21, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Andreas Rheinhardt: Hi Andreas, >> Besides being redundant, freeing manually is actually harmful here, >> as rtmp_close() may call gen_fcunpublish_stream() which dereferences >> rt->playpath. >> >> Reported-by: Armin Hasitzka <armin@grabyo.com> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavformat/rtmpproto.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c >> index 4b01b67d28..b1d73b3d75 100644 >> --- a/libavformat/rtmpproto.c >> +++ b/libavformat/rtmpproto.c >> @@ -2917,9 +2917,6 @@ reconnect: >> return 0; >> >> fail: >> - av_freep(&rt->playpath); >> - av_freep(&rt->tcurl); >> - av_freep(&rt->flashver); >> av_dict_free(opts); >> rtmp_close(s); >> return ret; > > I am pinging this and explicitly cc'ing Steven Liu, whose commit > 991cf95fdeebc3af added the av_freeps to be removed above. Steven, did > you just feel that there was missing freeing code for the buffers above > or was there an actually confirmed memleak (there shouldn't be)? Confirmed memleak, but it’s long time i cannot sure how to reproduce that, I test rtmp those years use SRS. Thanks Steven
Liu Steven <lq@chinaffmpeg.org> 于2024年3月31日周日 10:16写道： > > > > > On Mar 30, 2024, at 12:21, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > > > Andreas Rheinhardt: > Hi Andreas, > > >> Besides being redundant, freeing manually is actually harmful here, > >> as rtmp_close() may call gen_fcunpublish_stream() which dereferences > >> rt->playpath. > >> > >> Reported-by: Armin Hasitzka <armin@grabyo.com> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > >> --- > >> libavformat/rtmpproto.c | 3 --- > >> 1 file changed, 3 deletions(-) > >> > >> diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c > >> index 4b01b67d28..b1d73b3d75 100644 > >> --- a/libavformat/rtmpproto.c > >> +++ b/libavformat/rtmpproto.c > >> @@ -2917,9 +2917,6 @@ reconnect: > >> return 0; > >> > >> fail: > >> - av_freep(&rt->playpath); > >> - av_freep(&rt->tcurl); > >> - av_freep(&rt->flashver); > >> av_dict_free(opts); > >> rtmp_close(s); > >> return ret; > > > > I am pinging this and explicitly cc'ing Steven Liu, whose commit > > 991cf95fdeebc3af added the av_freeps to be removed above. Steven, did > > you just feel that there was missing freeing code for the buffers above > > or was there an actually confirmed memleak (there shouldn't be)? > > Confirmed memleak, but it’s long time i cannot sure how to reproduce that, I test rtmp those years use SRS. May be the memleak was reported by fuzz program, the test case can injection failed result make the workflow into the failed, The gen_fcunpublish_stream should after av_malloc rt->playpath, rt->tcurl, rt->flashver av_malloc, and there cloud get failed in the gen_connect. Steven Thanks
diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c index 4b01b67d28..b1d73b3d75 100644 --- a/libavformat/rtmpproto.c +++ b/libavformat/rtmpproto.c @@ -2917,9 +2917,6 @@ reconnect: return 0; fail: - av_freep(&rt->playpath); - av_freep(&rt->tcurl); - av_freep(&rt->flashver); av_dict_free(opts); rtmp_close(s); return ret;
Besides being redundant, freeing manually is actually harmful here, as rtmp_close() may call gen_fcunpublish_stream() which dereferences rt->playpath. Reported-by: Armin Hasitzka <armin@grabyo.com> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavformat/rtmpproto.c | 3 --- 1 file changed, 3 deletions(-)