On Wed, 16 Nov 2016 15:14:59 +0100
Michael Niedermayer <mich...@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 <mich...@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.cadhal...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
