Re: [FFmpeg-devel] [PATCH 1/2] avcodec/sanm: Check extradata_size before allocations
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 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
> > > ---
> > > 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
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/sanm: Check extradata_size before allocations
On Tue, Jul 16, 2019 at 08:27:43AM +0200, Reimar Döffinger wrote:
> On 16.07.2019, at 00:50, Michael Niedermayer 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
> > ---
> > 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 GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/sanm: Check extradata_size before allocations
On 16.07.2019, at 00:50, Michael Niedermayer 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
> ---
> 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/2] avcodec/sanm: Check extradata_size before allocations
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
---
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;
-}
-
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);
--
2.22.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
