[FFmpeg-devel] [PATCH 1/2] avcodec/sanm: Check extradata_size before allocations

[Michael Niedermayer]

On Tue, Jul 16, 2019 at 08:47:26PM +0200, Michael Niedermayer wrote:
> On Tue, Jul 16, 2019 at 08:27:43AM +0200, Reimar Döffinger wrote:
> > On 16.07.2019, at 00:50, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > 
> > > Fixes: Leaks
> > > Fixes: 15349/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SANM_fuzzer-5102530557640704
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > > libavcodec/sanm.c | 9 ++++-----
> > > 1 file changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
> > > index 25aee7220f..60e2f4c624 100644
> > > --- a/libavcodec/sanm.c
> > > +++ b/libavcodec/sanm.c
> > > @@ -491,6 +491,10 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > 
> > >     ctx->avctx   = avctx;
> > >     ctx->version = !avctx->extradata_size;
> > > +    if (!ctx->version && avctx->extradata_size < 1026) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Not enough extradata.\n");
> > > +        return AVERROR_INVALIDDATA;
> > > +    }
> > > 
> > >     avctx->pix_fmt = ctx->version ? AV_PIX_FMT_RGB565 : AV_PIX_FMT_PAL8;
> > > 
> > > @@ -506,11 +510,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > >     if (!ctx->version) {
> > >         int i;
> > > 
> > > -        if (avctx->extradata_size < 1026) {
> > > -            av_log(avctx, AV_LOG_ERROR, "Not enough extradata.\n");
> > > -            return AVERROR_INVALIDDATA;
> > > -        }
> > 
> > This seems quite a bit less obvious.
> 
> > Is that the only error return case, and is adding the cleanup code complex enough that this is the better choice?
> 
> there are 2 error cases, this one and a check for the allocations.
> It seemd logic to me to do the check in the order of minimizing cleanup
> but i can add the cleanup call if people prefer or do some other
> solution ?
> 
> 
> > Either way I'd recommend a comment like
> > // early sanity check before allocations to avoid need for deallocation code.
> 
> will add

will apply with this change

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190805/1969eb58/attachment.sig>