Re: [FFmpeg-devel] [PATCH] apng: use correct size for output buffer

2015-11-07 Thread Andreas Cadhalpun 
On 07.11.2015 00:17, wm4 wrote:
> On Fri, 6 Nov 2015 23:56:52 +0100
> Andreas Cadhalpun  wrote:
>> Attached is a patch increasing the buffer size to 10 and
>> adding an assert that s->bpp is not larger.
> 
> I'm find with this, though I'm not (A)PNG maintainer.

On 07.11.2015 06:03, Paul B Mahol wrote:
> Should be fine.

Thanks, pushed.

Best regards,
Andreas

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

Re: [FFmpeg-devel] [PATCH] apng: use correct size for output buffer

2015-11-06 Thread Andreas Cadhalpun 
On 06.11.2015 22:29, wm4 wrote:
> On Fri, 6 Nov 2015 22:18:04 +0100
> Andreas Cadhalpun  wrote:
> 
>> This fixes a stack buffer overflow.
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavcodec/pngdec.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
>> index 689aa2b..c974654 100644
>> --- a/libavcodec/pngdec.c
>> +++ b/libavcodec/pngdec.c
>> @@ -1010,13 +1010,13 @@ static int handle_p_frame_apng(AVCodecContext 
>> *avctx, PNGDecContext *s,
>>  memcpy(buffer + row_start, p->data[0] + row_start, s->bpp * 
>> s->cur_w);
>>  }
>>  } else { // APNG_BLEND_OP_OVER
>> +uint8_t *output = av_malloc(s->bpp);
>>  for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) {
>>  uint8_t *foreground = p->data[0] + s->image_linesize * y + 
>> s->bpp * s->x_offset;
>>  uint8_t *background = buffer + s->image_linesize * y + s->bpp * 
>> s->x_offset;
>>  for (x = s->x_offset; x < s->x_offset + s->cur_w; ++x, 
>> foreground += s->bpp, background += s->bpp) {
>>  size_t b;
>>  uint8_t foreground_alpha, background_alpha, output_alpha;
>> -uint8_t output[4];
>>  
>>  // Since we might be blending alpha onto alpha, we use the 
>> following equations:
>>  // output_alpha = foreground_alpha + (1 - foreground_alpha) 
>> * background_alpha
>> @@ -1069,6 +1069,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
>> PNGDecContext *s,
>>  memcpy(background, output, s->bpp);
>>  }
>>  }
>> +av_freep();
>>  }
>>  
>>  // Copy blended buffer into the frame and free
> 
> This seems wasteful, can't it just be output[8]?

I think s->bpp can be up to 10:
size_t byte_depth = s->bit_depth > 8 ? 2 : 1;  // maximal 2
...
s->channels   = ff_png_get_nb_channels(s->color_type); // maximal 4
s->bits_per_pixel = s->bit_depth * s->channels;// bit_depth 
is maximal 16
s->bpp= (s->bits_per_pixel + 7) >> 3;  // maximal 8
...
if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE) {
...
s->bpp += byte_depth;  // maximal 10

> It also adds a bug (unchecked malloc).

Right, sorry for that.
Attached is a patch increasing the buffer size to 10 and
adding an assert that s->bpp is not larger.

Best regards,
Andreas

>From bf724da5da5efe778225e61a786cc9b8cb86a91f Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
Date: Fri, 6 Nov 2015 23:44:01 +0100
Subject: [PATCH] apng: use correct size for output buffer

The buffer needs s->bpp bytes, at maximum currently 10.
Assert that s->bpp is not larger.

This fixes a stack buffer overflow.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/pngdec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 689aa2b..feb1763 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -1016,7 +1016,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
 for (x = s->x_offset; x < s->x_offset + s->cur_w; ++x, foreground += s->bpp, background += s->bpp) {
 size_t b;
 uint8_t foreground_alpha, background_alpha, output_alpha;
-uint8_t output[4];
+uint8_t output[10];
 
 // Since we might be blending alpha onto alpha, we use the following equations:
 // output_alpha = foreground_alpha + (1 - foreground_alpha) * background_alpha
@@ -1056,6 +1056,8 @@ static int handle_p_frame_apng(AVCodecContext *avctx, PNGDecContext *s,
 
 output_alpha = foreground_alpha + FAST_DIV255((255 - foreground_alpha) * background_alpha);
 
+av_assert0(s->bpp <= 10);
+
 for (b = 0; b < s->bpp - 1; ++b) {
 if (output_alpha == 0) {
 output[b] = 0;
-- 
2.6.1

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

Re: [FFmpeg-devel] [PATCH] apng: use correct size for output buffer

2015-11-06 Thread wm4 
On Fri, 6 Nov 2015 23:56:52 +0100
Andreas Cadhalpun  wrote:

> On 06.11.2015 22:29, wm4 wrote:
> > On Fri, 6 Nov 2015 22:18:04 +0100
> > Andreas Cadhalpun  wrote:
> > 
> >> This fixes a stack buffer overflow.
> >>
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  libavcodec/pngdec.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> >> index 689aa2b..c974654 100644
> >> --- a/libavcodec/pngdec.c
> >> +++ b/libavcodec/pngdec.c
> >> @@ -1010,13 +1010,13 @@ static int handle_p_frame_apng(AVCodecContext 
> >> *avctx, PNGDecContext *s,
> >>  memcpy(buffer + row_start, p->data[0] + row_start, s->bpp * 
> >> s->cur_w);
> >>  }
> >>  } else { // APNG_BLEND_OP_OVER
> >> +uint8_t *output = av_malloc(s->bpp);
> >>  for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) {
> >>  uint8_t *foreground = p->data[0] + s->image_linesize * y + 
> >> s->bpp * s->x_offset;
> >>  uint8_t *background = buffer + s->image_linesize * y + s->bpp 
> >> * s->x_offset;
> >>  for (x = s->x_offset; x < s->x_offset + s->cur_w; ++x, 
> >> foreground += s->bpp, background += s->bpp) {
> >>  size_t b;
> >>  uint8_t foreground_alpha, background_alpha, output_alpha;
> >> -uint8_t output[4];
> >>  
> >>  // Since we might be blending alpha onto alpha, we use 
> >> the following equations:
> >>  // output_alpha = foreground_alpha + (1 - 
> >> foreground_alpha) * background_alpha
> >> @@ -1069,6 +1069,7 @@ static int handle_p_frame_apng(AVCodecContext 
> >> *avctx, PNGDecContext *s,
> >>  memcpy(background, output, s->bpp);
> >>  }
> >>  }
> >> +av_freep();
> >>  }
> >>  
> >>  // Copy blended buffer into the frame and free
> > 
> > This seems wasteful, can't it just be output[8]?
> 
> I think s->bpp can be up to 10:
> size_t byte_depth = s->bit_depth > 8 ? 2 : 1;  // maximal 
> 2
> ...
> s->channels   = ff_png_get_nb_channels(s->color_type); // maximal 
> 4
> s->bits_per_pixel = s->bit_depth * s->channels;// 
> bit_depth is maximal 16
> s->bpp= (s->bits_per_pixel + 7) >> 3;  // maximal 
> 8
> ...
> if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE) {
> ...
> s->bpp += byte_depth;  // maximal 
> 10
> 
> > It also adds a bug (unchecked malloc).
> 
> Right, sorry for that.
> Attached is a patch increasing the buffer size to 10 and
> adding an assert that s->bpp is not larger.

I'm find with this, though I'm not (A)PNG maintainer.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] apng: use correct size for output buffer

2015-11-06 Thread Paul B Mahol 
On 11/6/15, Andreas Cadhalpun  wrote:
> On 06.11.2015 22:29, wm4 wrote:
>> On Fri, 6 Nov 2015 22:18:04 +0100
>> Andreas Cadhalpun  wrote:
>>
>>> This fixes a stack buffer overflow.
>>>
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  libavcodec/pngdec.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
>>> index 689aa2b..c974654 100644
>>> --- a/libavcodec/pngdec.c
>>> +++ b/libavcodec/pngdec.c
>>> @@ -1010,13 +1010,13 @@ static int handle_p_frame_apng(AVCodecContext
>>> *avctx, PNGDecContext *s,
>>>  memcpy(buffer + row_start, p->data[0] + row_start, s->bpp *
>>> s->cur_w);
>>>  }
>>>  } else { // APNG_BLEND_OP_OVER
>>> +uint8_t *output = av_malloc(s->bpp);
>>>  for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) {
>>>  uint8_t *foreground = p->data[0] + s->image_linesize * y +
>>> s->bpp * s->x_offset;
>>>  uint8_t *background = buffer + s->image_linesize * y +
>>> s->bpp * s->x_offset;
>>>  for (x = s->x_offset; x < s->x_offset + s->cur_w; ++x,
>>> foreground += s->bpp, background += s->bpp) {
>>>  size_t b;
>>>  uint8_t foreground_alpha, background_alpha,
>>> output_alpha;
>>> -uint8_t output[4];
>>>
>>>  // Since we might be blending alpha onto alpha, we use
>>> the following equations:
>>>  // output_alpha = foreground_alpha + (1 -
>>> foreground_alpha) * background_alpha
>>> @@ -1069,6 +1069,7 @@ static int handle_p_frame_apng(AVCodecContext
>>> *avctx, PNGDecContext *s,
>>>  memcpy(background, output, s->bpp);
>>>  }
>>>  }
>>> +av_freep();
>>>  }
>>>
>>>  // Copy blended buffer into the frame and free
>>
>> This seems wasteful, can't it just be output[8]?
>
> I think s->bpp can be up to 10:
> size_t byte_depth = s->bit_depth > 8 ? 2 : 1;  //
> maximal 2
> ...
> s->channels   = ff_png_get_nb_channels(s->color_type); //
> maximal 4
> s->bits_per_pixel = s->bit_depth * s->channels;//
> bit_depth is maximal 16
> s->bpp= (s->bits_per_pixel + 7) >> 3;  //
> maximal 8
> ...
> if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE) {
> ...
> s->bpp += byte_depth;  //
> maximal 10
>
>> It also adds a bug (unchecked malloc).
>
> Right, sorry for that.
> Attached is a patch increasing the buffer size to 10 and
> adding an assert that s->bpp is not larger.

Should be fine.

>
> Best regards,
> Andreas
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] apng: use correct size for output buffer

2015-11-06 Thread Andreas Cadhalpun 
This fixes a stack buffer overflow.

Signed-off-by: Andreas Cadhalpun 
---
 libavcodec/pngdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 689aa2b..c974654 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -1010,13 +1010,13 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 memcpy(buffer + row_start, p->data[0] + row_start, s->bpp * 
s->cur_w);
 }
 } else { // APNG_BLEND_OP_OVER
+uint8_t *output = av_malloc(s->bpp);
 for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) {
 uint8_t *foreground = p->data[0] + s->image_linesize * y + s->bpp 
* s->x_offset;
 uint8_t *background = buffer + s->image_linesize * y + s->bpp * 
s->x_offset;
 for (x = s->x_offset; x < s->x_offset + s->cur_w; ++x, foreground 
+= s->bpp, background += s->bpp) {
 size_t b;
 uint8_t foreground_alpha, background_alpha, output_alpha;
-uint8_t output[4];
 
 // Since we might be blending alpha onto alpha, we use the 
following equations:
 // output_alpha = foreground_alpha + (1 - foreground_alpha) * 
background_alpha
@@ -1069,6 +1069,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 memcpy(background, output, s->bpp);
 }
 }
+av_freep();
 }
 
 // Copy blended buffer into the frame and free
-- 
2.6.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] apng: use correct size for output buffer

2015-11-06 Thread wm4 
On Fri, 6 Nov 2015 22:18:04 +0100
Andreas Cadhalpun  wrote:

> This fixes a stack buffer overflow.
> 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/pngdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 689aa2b..c974654 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -1010,13 +1010,13 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
> PNGDecContext *s,
>  memcpy(buffer + row_start, p->data[0] + row_start, s->bpp * 
> s->cur_w);
>  }
>  } else { // APNG_BLEND_OP_OVER
> +uint8_t *output = av_malloc(s->bpp);
>  for (y = s->y_offset; y < s->y_offset + s->cur_h; ++y) {
>  uint8_t *foreground = p->data[0] + s->image_linesize * y + 
> s->bpp * s->x_offset;
>  uint8_t *background = buffer + s->image_linesize * y + s->bpp * 
> s->x_offset;
>  for (x = s->x_offset; x < s->x_offset + s->cur_w; ++x, 
> foreground += s->bpp, background += s->bpp) {
>  size_t b;
>  uint8_t foreground_alpha, background_alpha, output_alpha;
> -uint8_t output[4];
>  
>  // Since we might be blending alpha onto alpha, we use the 
> following equations:
>  // output_alpha = foreground_alpha + (1 - foreground_alpha) 
> * background_alpha
> @@ -1069,6 +1069,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
> PNGDecContext *s,
>  memcpy(background, output, s->bpp);
>  }
>  }
> +av_freep();
>  }
>  
>  // Copy blended buffer into the frame and free

This seems wasteful, can't it just be output[8]?

It also adds a bug (unchecked malloc).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel