[FFmpeg-devel] [libav-devel] [PATCH] webp: ensure that each transform is only used once

[Andreas Cadhalpun]

On 05.03.2015 23:12, Justin Ruggles wrote:
> On 03/05/2015 05:02 PM, Andreas Cadhalpun wrote:
>>> From d80baa7f786ca326891e145a000fbecdde55da80 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun<Andreas.Cadhalpun at googlemail.com>
>> Date: Thu, 5 Mar 2015 22:48:28 +0100
>> Subject: [PATCH] webp: ensure that each transform is only used once
>>
>> According to the WebP Lossless Bitstream Specification
>> "each transform is allowed to be used only once".
>>
>> If a transform is more than once this can lead to memory
>> corruption.
>
> Can you give more details about the memory corruption?

Yes, but I think it's not really important, because the code was never 
meant to handle the same transform twice.
Anyway: If e.g. the color indexing transform is used twice, then 
parse_transform_color_indexing is called twice, which calls 
decode_entropy_coded_image with different parameters w.
There av_frame_get_buffer(img->frame, 1) is called, which the first time 
allocates a buffer for the first w, but the second time, 
img->frame->linesize[0] is not 0, so it is not updated in 
get_video_buffer, which means a buffer of the wrong size (e.g. too 
small) is allocated.
When this buffer is later written to, it just writes after the end of 
the buffer, overwriting whatever is there, e.g. pointers to other 
buffers, which then leads to crashes if these are accessed/freed.

>> Signed-off-by: Andreas Cadhalpun<Andreas.Cadhalpun at googlemail.com>
>> ---
>>   libavcodec/webp.c | 39 +++++++++++++++++++++++++++++++++++----
>>   1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
>> index 9549c0e..2cd976f 100644
>> --- a/libavcodec/webp.c
>> +++ b/libavcodec/webp.c
>> @@ -1104,7 +1104,7 @@ static int
>> vp8_lossless_decode_frame(AVCodecContext *avctx, AVFrame *p,
>>                                        unsigned int data_size, int
>> is_alpha_chunk)
>>   {
>>       WebPContext *s = avctx->priv_data;
>> -    int w, h, ret, i;
>> +    int w, h, ret, i, used;
>>
>>       if (!is_alpha_chunk) {
>>           s->lossless = 1;
>> @@ -1154,18 +1154,49 @@ static int
>> vp8_lossless_decode_frame(AVCodecContext *avctx, AVFrame *p,
>>       /* parse transformations */
>>       s->nb_transforms = 0;
>>       s->reduced_width = 0;
>> +    used = 0;
>>       while (get_bits1(&s->gb)) {
>>           enum TransformType transform = get_bits(&s->gb, 2);
>>           s->transforms[s->nb_transforms++] = transform;
>>           switch (transform) {
>>           case PREDICTOR_TRANSFORM:
>> -            ret = parse_transform_predictor(s);
>> +            if (used & 1) {
>
> Use (1 << transform) and move all of these checks to a single check
> before the switch.

That's a good idea. Update patch attached.

Best regards,
Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-webp-ensure-that-each-transform-is-only-used-once.patch
Type: text/x-diff
Size: 1769 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150306/115dfcfc/attachment.bin>