| Message ID | 20220111063856.15699-1-yshaw1999@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/asfdec: fix crash caused by free wlid pointers | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/commit_msg_ppc | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Yang Xiao: > From: Yang Xiao <yshaw99@outlook.com> > > This commit fixed a crash when seeking wma frames, asf decoder will try to demux in function asf_read_pts(). > Pointer member side_data of AVPacket that allocated by stack may be wild pointer. > Prevent releasing wild pointers in AVPacket when some functions try to call av_packet_unref, example av_read_frame(). > --- > libavformat/asfdec_f.c | 2 +- > libavformat/mpc.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > index a8f36ed286..bae0ecfc7c 100644 > --- a/libavformat/asfdec_f.c > +++ b/libavformat/asfdec_f.c > @@ -1433,7 +1433,7 @@ static int64_t asf_read_pts(AVFormatContext *s, int stream_index, > { > FFFormatContext *const si = ffformatcontext(s); > ASFContext *asf = s->priv_data; > - AVPacket pkt1, *pkt = &pkt1; > + AVPacket pkt1 = {0}, *pkt = &pkt1; > ASFStream *asf_st; > int64_t pts; > int64_t pos = *ppos; > diff --git a/libavformat/mpc.c b/libavformat/mpc.c > index b5b2bab33c..ad0d693152 100644 > --- a/libavformat/mpc.c > +++ b/libavformat/mpc.c > @@ -189,7 +189,7 @@ static int mpc_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp > AVStream *st = s->streams[stream_index]; > FFStream *const sti = ffstream(st); > MPCContext *c = s->priv_data; > - AVPacket pkt1, *pkt = &pkt1; > + AVPacket pkt1 = {0}, *pkt = &pkt1; > int ret; > int index = av_index_search_timestamp(st, FFMAX(timestamp - DELAY_FRAMES, 0), flags); > uint32_t lastframe; > Do you have FF_API_INIT_PACKET set to 0 (it should still be set to 1)? Because av_read_frame() is supposed to (and documented to) treat the packet it is given as uninitialized. - Andreas
At 2022-01-11 17:29:35, "Andreas Rheinhardt" <andreas.rheinhardt@outlook.com> wrote: >Yang Xiao: >> From: Yang Xiao <yshaw99@outlook.com> >> >> This commit fixed a crash when seeking wma frames, asf decoder will try to demux in function asf_read_pts(). >> Pointer member side_data of AVPacket that allocated by stack may be wild pointer. >> Prevent releasing wild pointers in AVPacket when some functions try to call av_packet_unref, example av_read_frame(). >> --- >> libavformat/asfdec_f.c | 2 +- >> libavformat/mpc.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c >> index a8f36ed286..bae0ecfc7c 100644 >> --- a/libavformat/asfdec_f.c >> +++ b/libavformat/asfdec_f.c >> @@ -1433,7 +1433,7 @@ static int64_t asf_read_pts(AVFormatContext *s, int stream_index, >> { >> FFFormatContext *const si = ffformatcontext(s); >> ASFContext *asf = s->priv_data; >> - AVPacket pkt1, *pkt = &pkt1; >> + AVPacket pkt1 = {0}, *pkt = &pkt1; >> ASFStream *asf_st; >> int64_t pts; >> int64_t pos = *ppos; >> diff --git a/libavformat/mpc.c b/libavformat/mpc.c >> index b5b2bab33c..ad0d693152 100644 >> --- a/libavformat/mpc.c >> +++ b/libavformat/mpc.c >> @@ -189,7 +189,7 @@ static int mpc_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp >> AVStream *st = s->streams[stream_index]; >> FFStream *const sti = ffstream(st); >> MPCContext *c = s->priv_data; >> - AVPacket pkt1, *pkt = &pkt1; >> + AVPacket pkt1 = {0}, *pkt = &pkt1; >> int ret; >> int index = av_index_search_timestamp(st, FFMAX(timestamp - DELAY_FRAMES, 0), flags); >> uint32_t lastframe; >> > >Do you have FF_API_INIT_PACKET set to 0 (it should still be set to 1)? >Because av_read_frame() is supposed to (and documented to) treat the >packet it is given as uninitialized. > >- Andreas Thanks for your comment! But I have a question, av_init_packet() has been markedattribute_deprecated, If av_init_packet() is deprecated completely in the future, how to ensure ff_read_packet() safely (support to input a uninitialized packet) ? AVPacket->side_data will not be safely initialized with only av_packet_unref(). And as a user of the FFmpeg API, I want to disable some of the functions marked attribute_deprecated early, because upgrading FFmpeg has a large impact. Do I have other ways to achieve this goal? >_______________________________________________ >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".
XiaoYang: > > > > At 2022-01-11 17:29:35, "Andreas Rheinhardt" <andreas.rheinhardt@outlook.com> wrote: >> Yang Xiao: >>> From: Yang Xiao <yshaw99@outlook.com> >>> >>> This commit fixed a crash when seeking wma frames, asf decoder will try to demux in function asf_read_pts(). >>> Pointer member side_data of AVPacket that allocated by stack may be wild pointer. >>> Prevent releasing wild pointers in AVPacket when some functions try to call av_packet_unref, example av_read_frame(). >>> --- >>> libavformat/asfdec_f.c | 2 +- >>> libavformat/mpc.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c >>> index a8f36ed286..bae0ecfc7c 100644 >>> --- a/libavformat/asfdec_f.c >>> +++ b/libavformat/asfdec_f.c >>> @@ -1433,7 +1433,7 @@ static int64_t asf_read_pts(AVFormatContext *s, int stream_index, >>> { >>> FFFormatContext *const si = ffformatcontext(s); >>> ASFContext *asf = s->priv_data; >>> - AVPacket pkt1, *pkt = &pkt1; >>> + AVPacket pkt1 = {0}, *pkt = &pkt1; >>> ASFStream *asf_st; >>> int64_t pts; >>> int64_t pos = *ppos; >>> diff --git a/libavformat/mpc.c b/libavformat/mpc.c >>> index b5b2bab33c..ad0d693152 100644 >>> --- a/libavformat/mpc.c >>> +++ b/libavformat/mpc.c >>> @@ -189,7 +189,7 @@ static int mpc_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp >>> AVStream *st = s->streams[stream_index]; >>> FFStream *const sti = ffstream(st); >>> MPCContext *c = s->priv_data; >>> - AVPacket pkt1, *pkt = &pkt1; >>> + AVPacket pkt1 = {0}, *pkt = &pkt1; >>> int ret; >>> int index = av_index_search_timestamp(st, FFMAX(timestamp - DELAY_FRAMES, 0), flags); >>> uint32_t lastframe; >>> >> >> Do you have FF_API_INIT_PACKET set to 0 (it should still be set to 1)? >> Because av_read_frame() is supposed to (and documented to) treat the >> packet it is given as uninitialized. > >> >> - Andreas > > > > Thanks for your comment! > But I have a question, av_init_packet() has been markedattribute_deprecated, If av_init_packet() is deprecated completely in the future, how to ensure ff_read_packet() safely (support to input a uninitialized packet) ? AVPacket->side_data will not be safely initialized with only av_packet_unref(). > Uninitialized packets won't be a thing in the future because all packets will in the future originate from av_packet_alloc() (save for some in libavcodec itself). (This does not mean that I like the av_packet_unref(); it'd rather document that the packet has to be blank (as if coming from av_packet_alloc(), av_packet_unref() or as if it had been used as src packet in av_packet_move_ref()).) > > And as a user of the FFmpeg API, I want to disable some of the functions marked attribute_deprecated early, because upgrading FFmpeg has a large impact. > Do I have other ways to achieve this goal? > Actually, you are not really supposed to set the FF_API_* defines yourself. Although adding a configure option that already disables as many of these as possible would be nice for users to test their compatibility with future versions as well as for users that don't need the old API already. - Andreas
At 2022-01-12 19:07:44, "Andreas Rheinhardt" <andreas.rheinhardt@outlook.com> wrote: >XiaoYang: >> >> >> >> At 2022-01-11 17:29:35, "Andreas Rheinhardt" <andreas.rheinhardt@outlook.com> wrote: >>> Yang Xiao: >>>> From: Yang Xiao <yshaw99@outlook.com> >>>> >>>> This commit fixed a crash when seeking wma frames, asf decoder will try to demux in function asf_read_pts(). >>>> Pointer member side_data of AVPacket that allocated by stack may be wild pointer. >>>> Prevent releasing wild pointers in AVPacket when some functions try to call av_packet_unref, example av_read_frame(). >>>> --- >>>> libavformat/asfdec_f.c | 2 +- >>>> libavformat/mpc.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c >>>> index a8f36ed286..bae0ecfc7c 100644 >>>> --- a/libavformat/asfdec_f.c >>>> +++ b/libavformat/asfdec_f.c >>>> @@ -1433,7 +1433,7 @@ static int64_t asf_read_pts(AVFormatContext *s, int stream_index, >>>> { >>>> FFFormatContext *const si = ffformatcontext(s); >>>> ASFContext *asf = s->priv_data; >>>> - AVPacket pkt1, *pkt = &pkt1; >>>> + AVPacket pkt1 = {0}, *pkt = &pkt1; >>>> ASFStream *asf_st; >>>> int64_t pts; >>>> int64_t pos = *ppos; >>>> diff --git a/libavformat/mpc.c b/libavformat/mpc.c >>>> index b5b2bab33c..ad0d693152 100644 >>>> --- a/libavformat/mpc.c >>>> +++ b/libavformat/mpc.c >>>> @@ -189,7 +189,7 @@ static int mpc_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp >>>> AVStream *st = s->streams[stream_index]; >>>> FFStream *const sti = ffstream(st); >>>> MPCContext *c = s->priv_data; >>>> - AVPacket pkt1, *pkt = &pkt1; >>>> + AVPacket pkt1 = {0}, *pkt = &pkt1; >>>> int ret; >>>> int index = av_index_search_timestamp(st, FFMAX(timestamp - DELAY_FRAMES, 0), flags); >>>> uint32_t lastframe; >>>> >>> >>> Do you have FF_API_INIT_PACKET set to 0 (it should still be set to 1)? >>> Because av_read_frame() is supposed to (and documented to) treat the >>> packet it is given as uninitialized. >> >>> >>> - Andreas >> >> >> >> Thanks for your comment! >> But I have a question, av_init_packet() has been markedattribute_deprecated, If av_init_packet() is deprecated completely in the future, how to ensure ff_read_packet() safely (support to input a uninitialized packet) ? AVPacket->side_data will not be safely initialized with only av_packet_unref(). >> > >Uninitialized packets won't be a thing in the future because all packets >will in the future originate from av_packet_alloc() (save for some in >libavcodec itself). >(This does not mean that I like the av_packet_unref(); it'd rather >document that the packet has to be blank (as if coming from >av_packet_alloc(), av_packet_unref() or as if it had been used as src >packet in av_packet_move_ref()).) > >> >> And as a user of the FFmpeg API, I want to disable some of the functions marked attribute_deprecated early, because upgrading FFmpeg has a large impact. >> Do I have other ways to achieve this goal? >> > >Actually, you are not really supposed to set the FF_API_* defines >yourself. Although adding a configure option that already disables as >many of these as possible would be nice for users to test their >compatibility with future versions as well as for users that don't need >the old API already. > >- Andreas We are deep users of ffmpeg. Banning the use of deprecated API can reduce unnecessary upgrades, collect and report problems with new version during use. If there is no performance improvement, upgrading a stable old version FFmpeg is time-consuming but not beneficial. If it's necessary to replace all packet allocated by stack with packet allocated by av_packet_alloc() , I would like to submit some patchs to fix all such initialization problems (still save for some in libavcodec itself) , or if FFmpeg has another way to handle it? >_______________________________________________ >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".
At 2022-01-12 19:07:44, "Andreas Rheinhardt" <andreas.rheinhardt@outlook.com> wrote: >XiaoYang: >> >> >> >> At 2022-01-11 17:29:35, "Andreas Rheinhardt" <andreas.rheinhardt@outlook.com> wrote: >>> Yang Xiao: >>>> From: Yang Xiao <yshaw99@outlook.com> >>>> >>>> This commit fixed a crash when seeking wma frames, asf decoder will try to demux in function asf_read_pts(). >>>> Pointer member side_data of AVPacket that allocated by stack may be wild pointer. >>>> Prevent releasing wild pointers in AVPacket when some functions try to call av_packet_unref, example av_read_frame(). >>>> --- >>>> libavformat/asfdec_f.c | 2 +- >>>> libavformat/mpc.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c >>>> index a8f36ed286..bae0ecfc7c 100644 >>>> --- a/libavformat/asfdec_f.c >>>> +++ b/libavformat/asfdec_f.c >>>> @@ -1433,7 +1433,7 @@ static int64_t asf_read_pts(AVFormatContext *s, int stream_index, >>>> { >>>> FFFormatContext *const si = ffformatcontext(s); >>>> ASFContext *asf = s->priv_data; >>>> - AVPacket pkt1, *pkt = &pkt1; >>>> + AVPacket pkt1 = {0}, *pkt = &pkt1; >>>> ASFStream *asf_st; >>>> int64_t pts; >>>> int64_t pos = *ppos; >>>> diff --git a/libavformat/mpc.c b/libavformat/mpc.c >>>> index b5b2bab33c..ad0d693152 100644 >>>> --- a/libavformat/mpc.c >>>> +++ b/libavformat/mpc.c >>>> @@ -189,7 +189,7 @@ static int mpc_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp >>>> AVStream *st = s->streams[stream_index]; >>>> FFStream *const sti = ffstream(st); >>>> MPCContext *c = s->priv_data; >>>> - AVPacket pkt1, *pkt = &pkt1; >>>> + AVPacket pkt1 = {0}, *pkt = &pkt1; >>>> int ret; >>>> int index = av_index_search_timestamp(st, FFMAX(timestamp - DELAY_FRAMES, 0), flags); >>>> uint32_t lastframe; >>>> >>> >>> Do you have FF_API_INIT_PACKET set to 0 (it should still be set to 1)? >>> Because av_read_frame() is supposed to (and documented to) treat the >>> packet it is given as uninitialized. >> >>> >>> - Andreas >> >> >> >> Thanks for your comment! >> But I have a question, av_init_packet() has been markedattribute_deprecated, If av_init_packet() is deprecated completely in the future, how to ensure ff_read_packet() safely (support to input a uninitialized packet) ? AVPacket->side_data will not be safely initialized with only av_packet_unref(). >> > >Uninitialized packets won't be a thing in the future because all packets >will in the future originate from av_packet_alloc() (save for some in >libavcodec itself). >(This does not mean that I like the av_packet_unref(); it'd rather >document that the packet has to be blank (as if coming from >av_packet_alloc(), av_packet_unref() or as if it had been used as src >packet in av_packet_move_ref()).) > >> >> And as a user of the FFmpeg API, I want to disable some of the functions marked attribute_deprecated early, because upgrading FFmpeg has a large impact. >> Do I have other ways to achieve this goal? >> > >Actually, you are not really supposed to set the FF_API_* defines >yourself. Although adding a configure option that already disables as >many of these as possible would be nice for users to test their >compatibility with future versions as well as for users that don't need >the old API already. > >- Andreas Thank you for your advice :) We do this because we can better collect and report problems during use new version. Is there any other effective method unless degrade, such as making avpackets allocated by the heap?
diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c index a8f36ed286..bae0ecfc7c 100644 --- a/libavformat/asfdec_f.c +++ b/libavformat/asfdec_f.c @@ -1433,7 +1433,7 @@ static int64_t asf_read_pts(AVFormatContext *s, int stream_index, { FFFormatContext *const si = ffformatcontext(s); ASFContext *asf = s->priv_data; - AVPacket pkt1, *pkt = &pkt1; + AVPacket pkt1 = {0}, *pkt = &pkt1; ASFStream *asf_st; int64_t pts; int64_t pos = *ppos; diff --git a/libavformat/mpc.c b/libavformat/mpc.c index b5b2bab33c..ad0d693152 100644 --- a/libavformat/mpc.c +++ b/libavformat/mpc.c @@ -189,7 +189,7 @@ static int mpc_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp AVStream *st = s->streams[stream_index]; FFStream *const sti = ffstream(st); MPCContext *c = s->priv_data; - AVPacket pkt1, *pkt = &pkt1; + AVPacket pkt1 = {0}, *pkt = &pkt1; int ret; int index = av_index_search_timestamp(st, FFMAX(timestamp - DELAY_FRAMES, 0), flags); uint32_t lastframe;
From: Yang Xiao <yshaw99@outlook.com> This commit fixed a crash when seeking wma frames, asf decoder will try to demux in function asf_read_pts(). Pointer member side_data of AVPacket that allocated by stack may be wild pointer. Prevent releasing wild pointers in AVPacket when some functions try to call av_packet_unref, example av_read_frame(). --- libavformat/asfdec_f.c | 2 +- libavformat/mpc.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)