| Message ID | 81d313d1-f9ff-3e50-b855-08eb99aa010d@googlemail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: > On 15.11.2016 03:18, Michael Niedermayer wrote: > > On Sun, Nov 13, 2016 at 11:25:32PM +0100, Andreas Cadhalpun wrote: > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > >> --- > >> libavcodec/libschroedingerdec.c | 26 +++++++++++++++++--------- > >> 1 file changed, 17 insertions(+), 9 deletions(-) > >> > >> diff --git a/libavcodec/libschroedingerdec.c b/libavcodec/libschroedingerdec.c > >> index 1e392b3..83c790c 100644 > >> --- a/libavcodec/libschroedingerdec.c > >> +++ b/libavcodec/libschroedingerdec.c > >> @@ -218,6 +218,7 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > >> int outer = 1; > >> SchroParseUnitContext parse_ctx; > >> LibSchroFrameContext *framewithpts = NULL; > >> + int ret; > >> > >> *got_frame = 0; > >> > >> @@ -236,7 +237,8 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > >> enc_buf->tag = schro_tag_new(av_malloc(sizeof(int64_t)), av_free); > >> if (!enc_buf->tag->value) { > >> av_log(avctx, AV_LOG_ERROR, "Unable to allocate SchroTag\n"); > >> - return AVERROR(ENOMEM); > >> + ret = AVERROR(ENOMEM); > >> + goto end; > >> } > >> AV_WN(64, enc_buf->tag->value, pts); > >> /* Push buffer into decoder. */ > >> @@ -267,8 +269,10 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > >> /* Decoder needs a frame - create one and push it in. */ > >> frame = ff_create_schro_frame(avctx, > >> p_schro_params->frame_format); > >> - if (!frame) > >> - return AVERROR(ENOMEM); > >> + if (!frame) { > >> + ret = AVERROR(ENOMEM); > >> + goto end; > >> + } > >> schro_decoder_add_output_picture(decoder, frame); > >> break; > >> > > > > this looks a bit strange > > framewithpts is set to newly allocated memory below which is injected > > into the que and IIUC that can occur multiple times > > the free at the end for one of multiple such que entries feels wrong > > Indeed, only the framewithpts returned from ff_schro_queue_pop needs to > be freed. New patch is attached. > > However, considering the sheer amount of crashes in libschroedinger and > that it's apparently not maintained anymore, it might be better to > simply remove this decoder. id say decoders which crash should be marked as AV_CODEC_CAP_EXPERIMENTAL > > Best regards, > Andreas > libschroedingerdec.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > 6cc9768bea564b1bf50a25198deaf95adfa3151a 0001-libschroedingerdec-fix-leaking-of-framewithpts.patch > From 9cf7226543e25e494bed768b73f39e67d89f25d1 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > Date: Sun, 13 Nov 2016 23:10:06 +0100 > Subject: [PATCH] libschroedingerdec: fix leaking of framewithpts > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> should be ok [...]
On Wed, 16 Nov 2016 13:21:34 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: > > On 15.11.2016 03:18, Michael Niedermayer wrote: > > > On Sun, Nov 13, 2016 at 11:25:32PM +0100, Andreas Cadhalpun wrote: > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > >> --- > > >> libavcodec/libschroedingerdec.c | 26 +++++++++++++++++--------- > > >> 1 file changed, 17 insertions(+), 9 deletions(-) > > >> > > >> diff --git a/libavcodec/libschroedingerdec.c b/libavcodec/libschroedingerdec.c > > >> index 1e392b3..83c790c 100644 > > >> --- a/libavcodec/libschroedingerdec.c > > >> +++ b/libavcodec/libschroedingerdec.c > > >> @@ -218,6 +218,7 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > >> int outer = 1; > > >> SchroParseUnitContext parse_ctx; > > >> LibSchroFrameContext *framewithpts = NULL; > > >> + int ret; > > >> > > >> *got_frame = 0; > > >> > > >> @@ -236,7 +237,8 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > >> enc_buf->tag = schro_tag_new(av_malloc(sizeof(int64_t)), av_free); > > >> if (!enc_buf->tag->value) { > > >> av_log(avctx, AV_LOG_ERROR, "Unable to allocate SchroTag\n"); > > >> - return AVERROR(ENOMEM); > > >> + ret = AVERROR(ENOMEM); > > >> + goto end; > > >> } > > >> AV_WN(64, enc_buf->tag->value, pts); > > >> /* Push buffer into decoder. */ > > >> @@ -267,8 +269,10 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > >> /* Decoder needs a frame - create one and push it in. */ > > >> frame = ff_create_schro_frame(avctx, > > >> p_schro_params->frame_format); > > >> - if (!frame) > > >> - return AVERROR(ENOMEM); > > >> + if (!frame) { > > >> + ret = AVERROR(ENOMEM); > > >> + goto end; > > >> + } > > >> schro_decoder_add_output_picture(decoder, frame); > > >> break; > > >> > > > > > > this looks a bit strange > > > framewithpts is set to newly allocated memory below which is injected > > > into the que and IIUC that can occur multiple times > > > the free at the end for one of multiple such que entries feels wrong > > > > Indeed, only the framewithpts returned from ff_schro_queue_pop needs to > > be freed. New patch is attached. > > > > > However, considering the sheer amount of crashes in libschroedinger and > > that it's apparently not maintained anymore, it might be better to > > simply remove this decoder. > > id say decoders which crash should be marked as > AV_CODEC_CAP_EXPERIMENTAL Experimental implies it's early work in progress which will stabilize later. libschroedinger is completely dead, and it's entirely possible that you're the person who cares most about it on this planet. Should we introduce a AV_CODEC_CAP_TRASH flag?
On Wed, Nov 16, 2016 at 01:48:05PM +0100, wm4 wrote: > On Wed, 16 Nov 2016 13:21:34 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: > > > On 15.11.2016 03:18, Michael Niedermayer wrote: > > > > On Sun, Nov 13, 2016 at 11:25:32PM +0100, Andreas Cadhalpun wrote: > > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > > >> --- > > > >> libavcodec/libschroedingerdec.c | 26 +++++++++++++++++--------- > > > >> 1 file changed, 17 insertions(+), 9 deletions(-) > > > >> > > > >> diff --git a/libavcodec/libschroedingerdec.c b/libavcodec/libschroedingerdec.c > > > >> index 1e392b3..83c790c 100644 > > > >> --- a/libavcodec/libschroedingerdec.c > > > >> +++ b/libavcodec/libschroedingerdec.c > > > >> @@ -218,6 +218,7 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > > >> int outer = 1; > > > >> SchroParseUnitContext parse_ctx; > > > >> LibSchroFrameContext *framewithpts = NULL; > > > >> + int ret; > > > >> > > > >> *got_frame = 0; > > > >> > > > >> @@ -236,7 +237,8 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > > >> enc_buf->tag = schro_tag_new(av_malloc(sizeof(int64_t)), av_free); > > > >> if (!enc_buf->tag->value) { > > > >> av_log(avctx, AV_LOG_ERROR, "Unable to allocate SchroTag\n"); > > > >> - return AVERROR(ENOMEM); > > > >> + ret = AVERROR(ENOMEM); > > > >> + goto end; > > > >> } > > > >> AV_WN(64, enc_buf->tag->value, pts); > > > >> /* Push buffer into decoder. */ > > > >> @@ -267,8 +269,10 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > > >> /* Decoder needs a frame - create one and push it in. */ > > > >> frame = ff_create_schro_frame(avctx, > > > >> p_schro_params->frame_format); > > > >> - if (!frame) > > > >> - return AVERROR(ENOMEM); > > > >> + if (!frame) { > > > >> + ret = AVERROR(ENOMEM); > > > >> + goto end; > > > >> + } > > > >> schro_decoder_add_output_picture(decoder, frame); > > > >> break; > > > >> > > > > > > > > this looks a bit strange > > > > framewithpts is set to newly allocated memory below which is injected > > > > into the que and IIUC that can occur multiple times > > > > the free at the end for one of multiple such que entries feels wrong > > > > > > Indeed, only the framewithpts returned from ff_schro_queue_pop needs to > > > be freed. New patch is attached. > > > > > > > > However, considering the sheer amount of crashes in libschroedinger and > > > that it's apparently not maintained anymore, it might be better to > > > simply remove this decoder. > > > > id say decoders which crash should be marked as > > AV_CODEC_CAP_EXPERIMENTAL > > Experimental implies it's early work in progress which will stabilize > later. > > libschroedinger is completely dead, and it's entirely possible that > you're the person who cares most about it on this planet. > > Should we introduce a AV_CODEC_CAP_TRASH flag? i dont know, but i am not against a flag marking decoders with security issues or unmaintained code. i think we should use a different term than "trash", we should not call other peoples code anything offensive or insulting. [...]
On Wed, 16 Nov 2016 15:14:59 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Nov 16, 2016 at 01:48:05PM +0100, wm4 wrote: > > On Wed, 16 Nov 2016 13:21:34 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: > > > > On 15.11.2016 03:18, Michael Niedermayer wrote: > > > > > On Sun, Nov 13, 2016 at 11:25:32PM +0100, Andreas Cadhalpun wrote: > > > > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > > > >> --- > > > > >> libavcodec/libschroedingerdec.c | 26 +++++++++++++++++--------- > > > > >> 1 file changed, 17 insertions(+), 9 deletions(-) > > > > >> > > > > >> diff --git a/libavcodec/libschroedingerdec.c b/libavcodec/libschroedingerdec.c > > > > >> index 1e392b3..83c790c 100644 > > > > >> --- a/libavcodec/libschroedingerdec.c > > > > >> +++ b/libavcodec/libschroedingerdec.c > > > > >> @@ -218,6 +218,7 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > > > >> int outer = 1; > > > > >> SchroParseUnitContext parse_ctx; > > > > >> LibSchroFrameContext *framewithpts = NULL; > > > > >> + int ret; > > > > >> > > > > >> *got_frame = 0; > > > > >> > > > > >> @@ -236,7 +237,8 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > > > >> enc_buf->tag = schro_tag_new(av_malloc(sizeof(int64_t)), av_free); > > > > >> if (!enc_buf->tag->value) { > > > > >> av_log(avctx, AV_LOG_ERROR, "Unable to allocate SchroTag\n"); > > > > >> - return AVERROR(ENOMEM); > > > > >> + ret = AVERROR(ENOMEM); > > > > >> + goto end; > > > > >> } > > > > >> AV_WN(64, enc_buf->tag->value, pts); > > > > >> /* Push buffer into decoder. */ > > > > >> @@ -267,8 +269,10 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, > > > > >> /* Decoder needs a frame - create one and push it in. */ > > > > >> frame = ff_create_schro_frame(avctx, > > > > >> p_schro_params->frame_format); > > > > >> - if (!frame) > > > > >> - return AVERROR(ENOMEM); > > > > >> + if (!frame) { > > > > >> + ret = AVERROR(ENOMEM); > > > > >> + goto end; > > > > >> + } > > > > >> schro_decoder_add_output_picture(decoder, frame); > > > > >> break; > > > > >> > > > > > > > > > > this looks a bit strange > > > > > framewithpts is set to newly allocated memory below which is injected > > > > > into the que and IIUC that can occur multiple times > > > > > the free at the end for one of multiple such que entries feels wrong > > > > > > > > Indeed, only the framewithpts returned from ff_schro_queue_pop needs to > > > > be freed. New patch is attached. > > > > > > > > > > > However, considering the sheer amount of crashes in libschroedinger and > > > > that it's apparently not maintained anymore, it might be better to > > > > simply remove this decoder. > > > > > > id say decoders which crash should be marked as > > > AV_CODEC_CAP_EXPERIMENTAL > > > > Experimental implies it's early work in progress which will stabilize > > later. > > > > libschroedinger is completely dead, and it's entirely possible that > > you're the person who cares most about it on this planet. > > > > > Should we introduce a AV_CODEC_CAP_TRASH flag? > > i dont know, but i am not against a flag marking decoders with > security issues or unmaintained code. > i think we should use a different term than "trash", we should not > call other peoples code anything offensive or insulting. > > [...] It's not "trash" because the original authors wrote bad code (they probably didn't and it was fine back then), but because it has bitrotted.
On 16.11.2016 13:21, Michael Niedermayer wrote: > On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: >> libschroedingerdec.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> 6cc9768bea564b1bf50a25198deaf95adfa3151a 0001-libschroedingerdec-fix-leaking-of-framewithpts.patch >> From 9cf7226543e25e494bed768b73f39e67d89f25d1 Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> Date: Sun, 13 Nov 2016 23:10:06 +0100 >> Subject: [PATCH] libschroedingerdec: fix leaking of framewithpts >> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > > should be ok Set pushed. Best regards, Andreas
On 16.11.2016 15:14, Michael Niedermayer wrote: > On Wed, Nov 16, 2016 at 01:48:05PM +0100, wm4 wrote: >> On Wed, 16 Nov 2016 13:21:34 +0100 >> Michael Niedermayer <michael@niedermayer.cc> wrote: >>> On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: >>>> However, considering the sheer amount of crashes in libschroedinger and >>>> that it's apparently not maintained anymore, it might be better to >>>> simply remove this decoder. >>> >>> id say decoders which crash should be marked as >>> AV_CODEC_CAP_EXPERIMENTAL >> >> Experimental implies it's early work in progress which will stabilize >> later. >> >> libschroedinger is completely dead, and it's entirely possible that >> you're the person who cares most about it on this planet. >> > >> Should we introduce a AV_CODEC_CAP_TRASH flag? > > i dont know, but i am not against a flag marking decoders with > security issues or unmaintained code. > i think we should use a different term than "trash", we should not > call other peoples code anything offensive or insulting. Maybe AV_CODEC_CAP_DEPRECATED? But I'm not sure if adding this for just one decoder makes much sense. Best regards, Andreas
On Wed, Nov 16, 2016 at 07:46:59PM +0100, Andreas Cadhalpun wrote: > On 16.11.2016 15:14, Michael Niedermayer wrote: > > On Wed, Nov 16, 2016 at 01:48:05PM +0100, wm4 wrote: > >> On Wed, 16 Nov 2016 13:21:34 +0100 > >> Michael Niedermayer <michael@niedermayer.cc> wrote: > >>> On Tue, Nov 15, 2016 at 09:56:16PM +0100, Andreas Cadhalpun wrote: > >>>> However, considering the sheer amount of crashes in libschroedinger and > >>>> that it's apparently not maintained anymore, it might be better to > >>>> simply remove this decoder. > >>> > >>> id say decoders which crash should be marked as > >>> AV_CODEC_CAP_EXPERIMENTAL > >> > >> Experimental implies it's early work in progress which will stabilize > >> later. > >> > >> libschroedinger is completely dead, and it's entirely possible that > >> you're the person who cares most about it on this planet. > >> > > > >> Should we introduce a AV_CODEC_CAP_TRASH flag? > > > > i dont know, but i am not against a flag marking decoders with > > security issues or unmaintained code. > > i think we should use a different term than "trash", we should not > > call other peoples code anything offensive or insulting. > > Maybe AV_CODEC_CAP_DEPRECATED? > > But I'm not sure if adding this for just one decoder makes much sense. i dont know either but AV_CODEC_CAP_DEPRECATED sounds reasonable to me [...]
From 9cf7226543e25e494bed768b73f39e67d89f25d1 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> Date: Sun, 13 Nov 2016 23:10:06 +0100 Subject: [PATCH] libschroedingerdec: fix leaking of framewithpts Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/libschroedingerdec.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/libavcodec/libschroedingerdec.c b/libavcodec/libschroedingerdec.c index 1e392b3..02cbe57 100644 --- a/libavcodec/libschroedingerdec.c +++ b/libavcodec/libschroedingerdec.c @@ -218,6 +218,7 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, int outer = 1; SchroParseUnitContext parse_ctx; LibSchroFrameContext *framewithpts = NULL; + int ret; *got_frame = 0; @@ -308,10 +309,9 @@ static int libschroedinger_decode_frame(AVCodecContext *avctx, framewithpts = ff_schro_queue_pop(&p_schro_params->dec_frame_queue); if (framewithpts && framewithpts->frame && framewithpts->frame->components[0].stride) { - int ret; if ((ret = ff_get_buffer(avctx, avframe, 0)) < 0) - return ret; + goto end; memcpy(avframe->data[0], framewithpts->frame->components[0].data, @@ -337,15 +337,17 @@ FF_ENABLE_DEPRECATION_WARNINGS avframe->linesize[2] = framewithpts->frame->components[2].stride; *got_frame = 1; - - /* Now free the frame resources. */ - libschroedinger_decode_frame_free(framewithpts->frame); - av_free(framewithpts); } else { data = NULL; *got_frame = 0; } - return buf_size; + ret = buf_size; +end: + /* Now free the frame resources. */ + if (framewithpts && framewithpts->frame) + libschroedinger_decode_frame_free(framewithpts->frame); + av_freep(&framewithpts); + return ret; } -- 2.10.2