| Message ID | 20170424224746.2172-2-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | cac8de2da5c4935773128335c11b806faa73e19d |
| Headers | show |
On 4/24/2017 3:47 PM, James Almer wrote: > Free coded_frame, coded_side_data and unref hw_device_ctx to prevent > potential leaks. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/options.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/libavcodec/options.c b/libavcodec/options.c > index b98da9378a..82e12179a6 100644 > --- a/libavcodec/options.c > +++ b/libavcodec/options.c > @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) > #if FF_API_COPY_CONTEXT > static void copy_context_reset(AVCodecContext *avctx) > { > + int i; > + > av_opt_free(avctx); > +#if FF_API_CODED_FRAME > +FF_DISABLE_DEPRECATION_WARNINGS > + av_frame_free(&avctx->coded_frame); > +FF_ENABLE_DEPRECATION_WARNINGS > +#endif > av_freep(&avctx->rc_override); > av_freep(&avctx->intra_matrix); > av_freep(&avctx->inter_matrix); > av_freep(&avctx->extradata); > av_freep(&avctx->subtitle_header); > av_buffer_unref(&avctx->hw_frames_ctx); > + av_buffer_unref(&avctx->hw_device_ctx); > + for (i = 0; i < avctx->nb_coded_side_data; i++) > + av_freep(&avctx->coded_side_data[i].data); > + av_freep(&avctx->coded_side_data); > avctx->subtitle_header_size = 0; > + avctx->nb_coded_side_data = 0; > avctx->extradata_size = 0; > } > > @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS > > /* reallocate values that should be allocated separately */ > dest->extradata = NULL; > + dest->coded_side_data = NULL; > dest->intra_matrix = NULL; > dest->inter_matrix = NULL; > dest->rc_override = NULL; > dest->subtitle_header = NULL; > dest->hw_frames_ctx = NULL; > + dest->hw_device_ctx = NULL; > + dest->nb_coded_side_data = 0; > > #define alloc_and_copy_or_fail(obj, size, pad) \ > if (src->obj && size > 0) { \ > I'm not sure if this patch is intended to be a replacement for your last "2/2" patch, but this version is missing the the coded_side_data population code that was in the first version of your "2/2" patch. Aaron Levinson
On 4/26/2017 2:46 AM, Aaron Levinson wrote: > On 4/24/2017 3:47 PM, James Almer wrote: >> Free coded_frame, coded_side_data and unref hw_device_ctx to prevent >> potential leaks. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/options.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/libavcodec/options.c b/libavcodec/options.c >> index b98da9378a..82e12179a6 100644 >> --- a/libavcodec/options.c >> +++ b/libavcodec/options.c >> @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) >> #if FF_API_COPY_CONTEXT >> static void copy_context_reset(AVCodecContext *avctx) >> { >> + int i; >> + >> av_opt_free(avctx); >> +#if FF_API_CODED_FRAME >> +FF_DISABLE_DEPRECATION_WARNINGS >> + av_frame_free(&avctx->coded_frame); >> +FF_ENABLE_DEPRECATION_WARNINGS >> +#endif >> av_freep(&avctx->rc_override); >> av_freep(&avctx->intra_matrix); >> av_freep(&avctx->inter_matrix); >> av_freep(&avctx->extradata); >> av_freep(&avctx->subtitle_header); >> av_buffer_unref(&avctx->hw_frames_ctx); >> + av_buffer_unref(&avctx->hw_device_ctx); >> + for (i = 0; i < avctx->nb_coded_side_data; i++) >> + av_freep(&avctx->coded_side_data[i].data); >> + av_freep(&avctx->coded_side_data); >> avctx->subtitle_header_size = 0; >> + avctx->nb_coded_side_data = 0; >> avctx->extradata_size = 0; >> } >> >> @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS >> >> /* reallocate values that should be allocated separately */ >> dest->extradata = NULL; >> + dest->coded_side_data = NULL; >> dest->intra_matrix = NULL; >> dest->inter_matrix = NULL; >> dest->rc_override = NULL; >> dest->subtitle_header = NULL; >> dest->hw_frames_ctx = NULL; >> + dest->hw_device_ctx = NULL; >> + dest->nb_coded_side_data = 0; >> >> #define alloc_and_copy_or_fail(obj, size, pad) \ >> if (src->obj && size > 0) { \ >> > > I'm not sure if this patch is intended to be a replacement for your last > "2/2" patch, but this version is missing the the coded_side_data > population code that was in the first version of your "2/2" patch. > > Aaron Levinson It is. I'm keeping the functionality of the function as is, and only making sure cleanup is complete to prevent leaks. This function is deprecated and shouldn't be used, so i'm not going to make it copy even more stuff.
On 4/26/2017 12:47 PM, James Almer wrote: > On 4/26/2017 2:46 AM, Aaron Levinson wrote: >> On 4/24/2017 3:47 PM, James Almer wrote: >>> Free coded_frame, coded_side_data and unref hw_device_ctx to prevent >>> potential leaks. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/options.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>> index b98da9378a..82e12179a6 100644 >>> --- a/libavcodec/options.c >>> +++ b/libavcodec/options.c >>> @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) >>> #if FF_API_COPY_CONTEXT >>> static void copy_context_reset(AVCodecContext *avctx) >>> { >>> + int i; >>> + >>> av_opt_free(avctx); >>> +#if FF_API_CODED_FRAME >>> +FF_DISABLE_DEPRECATION_WARNINGS >>> + av_frame_free(&avctx->coded_frame); >>> +FF_ENABLE_DEPRECATION_WARNINGS >>> +#endif >>> av_freep(&avctx->rc_override); >>> av_freep(&avctx->intra_matrix); >>> av_freep(&avctx->inter_matrix); >>> av_freep(&avctx->extradata); >>> av_freep(&avctx->subtitle_header); >>> av_buffer_unref(&avctx->hw_frames_ctx); >>> + av_buffer_unref(&avctx->hw_device_ctx); >>> + for (i = 0; i < avctx->nb_coded_side_data; i++) >>> + av_freep(&avctx->coded_side_data[i].data); >>> + av_freep(&avctx->coded_side_data); >>> avctx->subtitle_header_size = 0; >>> + avctx->nb_coded_side_data = 0; >>> avctx->extradata_size = 0; >>> } >>> >>> @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS >>> >>> /* reallocate values that should be allocated separately */ >>> dest->extradata = NULL; >>> + dest->coded_side_data = NULL; >>> dest->intra_matrix = NULL; >>> dest->inter_matrix = NULL; >>> dest->rc_override = NULL; >>> dest->subtitle_header = NULL; >>> dest->hw_frames_ctx = NULL; >>> + dest->hw_device_ctx = NULL; >>> + dest->nb_coded_side_data = 0; >>> >>> #define alloc_and_copy_or_fail(obj, size, pad) \ >>> if (src->obj && size > 0) { \ >>> >> >> I'm not sure if this patch is intended to be a replacement for your last >> "2/2" patch, but this version is missing the the coded_side_data >> population code that was in the first version of your "2/2" patch. >> >> Aaron Levinson > > It is. I'm keeping the functionality of the function as is, and only > making sure cleanup is complete to prevent leaks. > This function is deprecated and shouldn't be used, so i'm not going to > make it copy even more stuff. I've reviewed both patches 1 and 2, and both are good to commit. Technically, the behavior of the code is changed by zeroing out coded_side_data and nb_coded_side_data after doing the memcpy() call, even though, by not doing a copy of coded_side_data() previously, this resulted in the same coded_side_data pointer being owned by two AVCodecContext objects (assuming that there is any coded side data in the source AVCodecContext object). This was a bug, but if there is any code that depends on there being valid coded side data in the copied context, then this could result in problems. However, I've surveyed the code in ffmpeg proper, and I actually can find no uses of coded_side_data in AVCodecContext objects other than the code in ffmpeg.c to copy coded side data into the stream, and it is the copy of the coded side data in the stream that is actually used. This entire procedure of populating coded_side_data in AVCodecContext objects for it to be later copied into an AVStream object where it is actually used is a bit kludgy and could use some work. Aaron Levinson
On Sun, Apr 30, 2017 at 10:07:17AM -0700, Aaron Levinson wrote: > On 4/26/2017 12:47 PM, James Almer wrote: > >On 4/26/2017 2:46 AM, Aaron Levinson wrote: > >>On 4/24/2017 3:47 PM, James Almer wrote: > >>>Free coded_frame, coded_side_data and unref hw_device_ctx to prevent > >>>potential leaks. > >>> > >>>Signed-off-by: James Almer <jamrial@gmail.com> > >>>--- > >>> libavcodec/options.c | 15 +++++++++++++++ > >>> 1 file changed, 15 insertions(+) > >>> > >>>diff --git a/libavcodec/options.c b/libavcodec/options.c > >>>index b98da9378a..82e12179a6 100644 > >>>--- a/libavcodec/options.c > >>>+++ b/libavcodec/options.c > >>>@@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) > >>> #if FF_API_COPY_CONTEXT > >>> static void copy_context_reset(AVCodecContext *avctx) > >>> { > >>>+ int i; > >>>+ > >>> av_opt_free(avctx); > >>>+#if FF_API_CODED_FRAME > >>>+FF_DISABLE_DEPRECATION_WARNINGS > >>>+ av_frame_free(&avctx->coded_frame); > >>>+FF_ENABLE_DEPRECATION_WARNINGS > >>>+#endif > >>> av_freep(&avctx->rc_override); > >>> av_freep(&avctx->intra_matrix); > >>> av_freep(&avctx->inter_matrix); > >>> av_freep(&avctx->extradata); > >>> av_freep(&avctx->subtitle_header); > >>> av_buffer_unref(&avctx->hw_frames_ctx); > >>>+ av_buffer_unref(&avctx->hw_device_ctx); > >>>+ for (i = 0; i < avctx->nb_coded_side_data; i++) > >>>+ av_freep(&avctx->coded_side_data[i].data); > >>>+ av_freep(&avctx->coded_side_data); > >>> avctx->subtitle_header_size = 0; > >>>+ avctx->nb_coded_side_data = 0; > >>> avctx->extradata_size = 0; > >>> } > >>> > >>>@@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS > >>> > >>> /* reallocate values that should be allocated separately */ > >>> dest->extradata = NULL; > >>>+ dest->coded_side_data = NULL; > >>> dest->intra_matrix = NULL; > >>> dest->inter_matrix = NULL; > >>> dest->rc_override = NULL; > >>> dest->subtitle_header = NULL; > >>> dest->hw_frames_ctx = NULL; > >>>+ dest->hw_device_ctx = NULL; > >>>+ dest->nb_coded_side_data = 0; > >>> > >>> #define alloc_and_copy_or_fail(obj, size, pad) \ > >>> if (src->obj && size > 0) { \ > >>> > >> > >>I'm not sure if this patch is intended to be a replacement for your last > >>"2/2" patch, but this version is missing the the coded_side_data > >>population code that was in the first version of your "2/2" patch. > >> > >>Aaron Levinson > > > >It is. I'm keeping the functionality of the function as is, and only > >making sure cleanup is complete to prevent leaks. > >This function is deprecated and shouldn't be used, so i'm not going to > >make it copy even more stuff. > > I've reviewed both patches 1 and 2, and both are good to commit. and i couldnt find a case the patches break [...] thx
On 4/30/2017 11:10 PM, Michael Niedermayer wrote: > On Sun, Apr 30, 2017 at 10:07:17AM -0700, Aaron Levinson wrote: >> On 4/26/2017 12:47 PM, James Almer wrote: >>> On 4/26/2017 2:46 AM, Aaron Levinson wrote: >>>> On 4/24/2017 3:47 PM, James Almer wrote: >>>>> Free coded_frame, coded_side_data and unref hw_device_ctx to prevent >>>>> potential leaks. >>>>> >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavcodec/options.c | 15 +++++++++++++++ >>>>> 1 file changed, 15 insertions(+) >>>>> >>>>> diff --git a/libavcodec/options.c b/libavcodec/options.c >>>>> index b98da9378a..82e12179a6 100644 >>>>> --- a/libavcodec/options.c >>>>> +++ b/libavcodec/options.c >>>>> @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) >>>>> #if FF_API_COPY_CONTEXT >>>>> static void copy_context_reset(AVCodecContext *avctx) >>>>> { >>>>> + int i; >>>>> + >>>>> av_opt_free(avctx); >>>>> +#if FF_API_CODED_FRAME >>>>> +FF_DISABLE_DEPRECATION_WARNINGS >>>>> + av_frame_free(&avctx->coded_frame); >>>>> +FF_ENABLE_DEPRECATION_WARNINGS >>>>> +#endif >>>>> av_freep(&avctx->rc_override); >>>>> av_freep(&avctx->intra_matrix); >>>>> av_freep(&avctx->inter_matrix); >>>>> av_freep(&avctx->extradata); >>>>> av_freep(&avctx->subtitle_header); >>>>> av_buffer_unref(&avctx->hw_frames_ctx); >>>>> + av_buffer_unref(&avctx->hw_device_ctx); >>>>> + for (i = 0; i < avctx->nb_coded_side_data; i++) >>>>> + av_freep(&avctx->coded_side_data[i].data); >>>>> + av_freep(&avctx->coded_side_data); >>>>> avctx->subtitle_header_size = 0; >>>>> + avctx->nb_coded_side_data = 0; >>>>> avctx->extradata_size = 0; >>>>> } >>>>> >>>>> @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS >>>>> >>>>> /* reallocate values that should be allocated separately */ >>>>> dest->extradata = NULL; >>>>> + dest->coded_side_data = NULL; >>>>> dest->intra_matrix = NULL; >>>>> dest->inter_matrix = NULL; >>>>> dest->rc_override = NULL; >>>>> dest->subtitle_header = NULL; >>>>> dest->hw_frames_ctx = NULL; >>>>> + dest->hw_device_ctx = NULL; >>>>> + dest->nb_coded_side_data = 0; >>>>> >>>>> #define alloc_and_copy_or_fail(obj, size, pad) \ >>>>> if (src->obj && size > 0) { \ >>>>> >>>> >>>> I'm not sure if this patch is intended to be a replacement for your last >>>> "2/2" patch, but this version is missing the the coded_side_data >>>> population code that was in the first version of your "2/2" patch. >>>> >>>> Aaron Levinson >>> >>> It is. I'm keeping the functionality of the function as is, and only >>> making sure cleanup is complete to prevent leaks. >>> This function is deprecated and shouldn't be used, so i'm not going to >>> make it copy even more stuff. >> >> I've reviewed both patches 1 and 2, and both are good to commit. > > and i couldnt find a case the patches break > > [...] > > thx Pushed, thanks.
diff --git a/libavcodec/options.c b/libavcodec/options.c index b98da9378a..82e12179a6 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -190,14 +190,26 @@ void avcodec_free_context(AVCodecContext **pavctx) #if FF_API_COPY_CONTEXT static void copy_context_reset(AVCodecContext *avctx) { + int i; + av_opt_free(avctx); +#if FF_API_CODED_FRAME +FF_DISABLE_DEPRECATION_WARNINGS + av_frame_free(&avctx->coded_frame); +FF_ENABLE_DEPRECATION_WARNINGS +#endif av_freep(&avctx->rc_override); av_freep(&avctx->intra_matrix); av_freep(&avctx->inter_matrix); av_freep(&avctx->extradata); av_freep(&avctx->subtitle_header); av_buffer_unref(&avctx->hw_frames_ctx); + av_buffer_unref(&avctx->hw_device_ctx); + for (i = 0; i < avctx->nb_coded_side_data; i++) + av_freep(&avctx->coded_side_data[i].data); + av_freep(&avctx->coded_side_data); avctx->subtitle_header_size = 0; + avctx->nb_coded_side_data = 0; avctx->extradata_size = 0; } @@ -238,11 +250,14 @@ FF_ENABLE_DEPRECATION_WARNINGS /* reallocate values that should be allocated separately */ dest->extradata = NULL; + dest->coded_side_data = NULL; dest->intra_matrix = NULL; dest->inter_matrix = NULL; dest->rc_override = NULL; dest->subtitle_header = NULL; dest->hw_frames_ctx = NULL; + dest->hw_device_ctx = NULL; + dest->nb_coded_side_data = 0; #define alloc_and_copy_or_fail(obj, size, pad) \ if (src->obj && size > 0) { \
Free coded_frame, coded_side_data and unref hw_device_ctx to prevent potential leaks. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/options.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)