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

Submitted by Michael Niedermayer on July 15, 2019, 10:50 p.m.

Details

Message ID 20190715225039.9689-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer July 15, 2019, 10:50 p.m.
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@niedermayer.cc>
---
 libavcodec/sanm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Reimar Döffinger July 16, 2019, 6:27 a.m.
On 16.07.2019, at 00:50, Michael Niedermayer <michael@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@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?
Either way I'd recommend a comment like
// early sanity check before allocations to avoid need for deallocation code.
Michael Niedermayer July 16, 2019, 6:47 p.m.
On Tue, Jul 16, 2019 at 08:27:43AM +0200, Reimar Döffinger wrote:
> On 16.07.2019, at 00:50, Michael Niedermayer <michael@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@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

thanks

[...]
Michael Niedermayer Aug. 5, 2019, 4:43 p.m.
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@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@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

[...]

Patch hide | download patch | download mbox

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;
-        }
-
         ctx->subversion = AV_RL16(avctx->extradata);
         for (i = 0; i < PALETTE_SIZE; i++)
             ctx->pal[i] = 0xFFU << 24 | AV_RL32(avctx->extradata + 2 + i * 4);