Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Andreas Cadhalpun]

On 14.11.2015 03:38, Vittorio Giovara wrote:
> On Fri, Nov 13, 2015 at 10:01 PM, Andreas Cadhalpun
>  wrote:
>> On 13.11.2015 02:08, Vittorio Giovara wrote:
>>> oh I see, that can happen for a special crafted file, DDPF_FOURCC has
>>> been introduced recently while DDPF_PALETTE has been removed, so a
>>> normal file should not have both set.
>>
>> OK, that makes sense.
>>
>>> Because of that, and how rare palette dds are, I think it's quite safe
>>> to unset ctx->paletted if ctx->compressed is set, and be done with it.
>>
>> Patch doing that is attached.
> 
> lgtm, thanks

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Andreas Cadhalpun]

On 13.11.2015 02:08, Vittorio Giovara wrote:
> oh I see, that can happen for a special crafted file, DDPF_FOURCC has
> been introduced recently while DDPF_PALETTE has been removed, so a
> normal file should not have both set.

OK, that makes sense.

> Because of that, and how rare palette dds are, I think it's quite safe
> to unset ctx->paletted if ctx->compressed is set, and be done with it.

Patch doing that is attached.

> That is the only case I can see that happen, right?

Yes, that's the only case.

> Thanks for the catch, how did you find it btw? :)

Like all the other: with afl [1].
It's really good at finding weird cases. ;)

Best regards,
Andreas

1: http://lcamtuf.coredump.cx/afl/
>From 6e55b4c60e93168236c0f05e67e89f0007da Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Fri, 13 Nov 2015 21:48:27 +0100
Subject: [PATCH] dds: disable palette flag for compressed dds

Having both is not valid and can cause a NULL pointer dereference of
frame->data[1] later.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/dds.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/libavcodec/dds.c b/libavcodec/dds.c
index f04a4f5..d473fd1 100644
--- a/libavcodec/dds.c
+++ b/libavcodec/dds.c
@@ -141,6 +141,12 @@ static int parse_pixel_format(AVCodecContext *avctx)
 normal_map  = flags & DDPF_NORMALMAP;
 fourcc = bytestream2_get_le32(gbc);
 
+if (ctx->compressed && ctx->paletted) {
+av_log(avctx, AV_LOG_WARNING,
+   "Disabling invalid palette flag for compressed dds.\n");
+ctx->paletted = 0;
+}
+
 bpp = bytestream2_get_le32(gbc); // rgbbitcount
 r   = bytestream2_get_le32(gbc); // rbitmask
 g   = bytestream2_get_le32(gbc); // gbitmask
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Vittorio Giovara]

On Fri, Nov 13, 2015 at 10:01 PM, Andreas Cadhalpun
 wrote:
> On 13.11.2015 02:08, Vittorio Giovara wrote:
>> oh I see, that can happen for a special crafted file, DDPF_FOURCC has
>> been introduced recently while DDPF_PALETTE has been removed, so a
>> normal file should not have both set.
>
> OK, that makes sense.
>
>> Because of that, and how rare palette dds are, I think it's quite safe
>> to unset ctx->paletted if ctx->compressed is set, and be done with it.
>
> Patch doing that is attached.

lgtm, thanks
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 1:16 AM, Andreas Cadhalpun
 wrote:
> Otherwise it causes a NULL pointer dereference of frame->data[1].
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/dds.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
> index c918cf0..fe36709 100644
> --- a/libavcodec/dds.c
> +++ b/libavcodec/dds.c
> @@ -662,6 +662,11 @@ static int dds_decode(AVCodecContext *avctx, void *data,
>
>  if (ctx->paletted) {
>  int i;
> +if (!frame->data[1]) {
> +av_log(avctx, AV_LOG_ERROR,
> +   "Palette frame buffer is not allocated.\n");
> +return AVERROR_INVALIDDATA;
> +}
>  /* Use the first 1024 bytes as palette, then copy the rest. */
>  bytestream2_get_buffer(gbc, frame->data[1], 256 * 4);
>  for (i = 0; i < 256; i++)

how can this happen?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Vittorio Giovara]

On Wed, Nov 11, 2015 at 8:29 PM, Andreas Cadhalpun
 wrote:
> On 11.11.2015 12:28, Vittorio Giovara wrote:
>> On Wed, Nov 11, 2015 at 1:16 AM, Andreas Cadhalpun
>>  wrote:
>>> Otherwise it causes a NULL pointer dereference of frame->data[1].
>>>
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  libavcodec/dds.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
>>> index c918cf0..fe36709 100644
>>> --- a/libavcodec/dds.c
>>> +++ b/libavcodec/dds.c
>>> @@ -662,6 +662,11 @@ static int dds_decode(AVCodecContext *avctx, void 
>>> *data,
>>>
>>>  if (ctx->paletted) {
>>>  int i;
>>> +if (!frame->data[1]) {
>>> +av_log(avctx, AV_LOG_ERROR,
>>> +   "Palette frame buffer is not allocated.\n");
>>> +return AVERROR_INVALIDDATA;
>>> +}
>>>  /* Use the first 1024 bytes as palette, then copy the rest. */
>>>  bytestream2_get_buffer(gbc, frame->data[1], 256 * 4);
>>>  for (i = 0; i < 256; i++)
>>
>> how can this happen?
>
> That's best described with code:
> if (!ctx->compressed && ctx->paletted &&
> !(av_pix_fmt_desc_get(avctx->pix_fmt)->flags & (AV_PIX_FMT_FLAG_PAL | 
> AV_PIX_FMT_FLAG_PSEUDOPAL)))
>
> Attached is a patch using this expression to check for the problem.

Sorry, I wasn't clear with my question. What I meant is that in
video_get_buffer() (frame.c) explicitly allocates data[1] when any
pixel format with AV_PIX_FMT_FLAG_PAL is requested, and in dds when
(ctx->paletted) is set, then AV_PIX_FMT_PAL8 is the pixel format
chosen every time. So unless I'm missing something data[1] is always
allocated, no?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [libav-devel] [PATCH 3/4] dds: make sure pallete frame buffer exists before use

[Andreas Cadhalpun]

On 11.11.2015 12:28, Vittorio Giovara wrote:
> On Wed, Nov 11, 2015 at 1:16 AM, Andreas Cadhalpun
>  wrote:
>> Otherwise it causes a NULL pointer dereference of frame->data[1].
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavcodec/dds.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/libavcodec/dds.c b/libavcodec/dds.c
>> index c918cf0..fe36709 100644
>> --- a/libavcodec/dds.c
>> +++ b/libavcodec/dds.c
>> @@ -662,6 +662,11 @@ static int dds_decode(AVCodecContext *avctx, void *data,
>>
>>  if (ctx->paletted) {
>>  int i;
>> +if (!frame->data[1]) {
>> +av_log(avctx, AV_LOG_ERROR,
>> +   "Palette frame buffer is not allocated.\n");
>> +return AVERROR_INVALIDDATA;
>> +}
>>  /* Use the first 1024 bytes as palette, then copy the rest. */
>>  bytestream2_get_buffer(gbc, frame->data[1], 256 * 4);
>>  for (i = 0; i < 256; i++)
> 
> how can this happen?

That's best described with code:
if (!ctx->compressed && ctx->paletted &&
!(av_pix_fmt_desc_get(avctx->pix_fmt)->flags & (AV_PIX_FMT_FLAG_PAL | 
AV_PIX_FMT_FLAG_PSEUDOPAL)))

Attached is a patch using this expression to check for the problem.

Best regards,
Andreas
>From 299189530856da6c3b57e3fcfac034f15244b36b Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Wed, 11 Nov 2015 19:43:28 +0100
Subject: [PATCH] dds: validate palette pixel format

If it doesn't have one of the necessary flags, it can cause a NULL
pointer dereference of frame->data[1] in dds_decode.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/dds.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/libavcodec/dds.c b/libavcodec/dds.c
index f04a4f5..6c4c88a 100644
--- a/libavcodec/dds.c
+++ b/libavcodec/dds.c
@@ -373,6 +373,13 @@ static int parse_pixel_format(AVCodecContext *avctx)
 }
 }
 
+if (!ctx->compressed && ctx->paletted &&
+!(av_pix_fmt_desc_get(avctx->pix_fmt)->flags & (AV_PIX_FMT_FLAG_PAL | AV_PIX_FMT_FLAG_PSEUDOPAL))) {
+av_log(avctx, AV_LOG_ERROR, "Unsupported palette pixel format: %s\n",
+   av_get_pix_fmt_name(avctx->pix_fmt));
+return AVERROR_INVALIDDATA;
+}
+
 /* Set any remaining post-proc that should happen before frame is ready. */
 if (alpha_exponent)
 ctx->postproc = DDS_ALPHA_EXP;
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel