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

[Andreas Cadhalpun]

On 06.11.2015 22:29, wm4 wrote:
> On Fri, 6 Nov 2015 22:18:04 +0100
> Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
> 
>> This fixes a stack buffer overflow.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> ---
>>  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(&output);
>>      }
>>  
>>      // 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-apng-use-correct-size-for-output-buffer.patch
Type: text/x-diff
Size: 1622 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151106/fe1a7ea1/attachment.patch>