[FFmpeg-devel] [PATCH] Fix DV uninitialized reads

[Baptiste Coudurier]

On 9/29/09 11:51 AM, Reimar D?ffinger wrote:
> On Tue, Sep 29, 2009 at 11:24:53AM -0700, Baptiste Coudurier wrote:
>> On 9/29/09 4:52 AM, Reimar D?ffinger wrote:
>>> On Mon, Sep 21, 2009 at 02:40:51PM +0200, Reimar D?ffinger wrote:
>>>> Hello,
>>>> I think this fixes the uninitialized data in the DV encoder that causes
>>>> sporadic "make test" failures, at least valgrind complains no longer.
>>>> Quick measurements with "time" indicate a slowdown by about 0.8%.
>>>> regression test values for the encoded files changes (memset to 0
>>>> instead of 0xff might avoid that though), but the decoded data
>>>> stays the same - so at least for the cases "make test" covers it is
>>>> correct.
>>>
>>> Ok, a different version.
>>> It uses s->sys->block_sizes even though I think it pointlessly bloats
>>> and complexifies the code, it memsets to 0xff still because the
>>> regression tests change anyway and the spec does not say anything about
>>> which value to use, and it returns -1 if the bitstream wrote too far,
>>> even though the return value is not checked anywhere.
>>
>> Humm, yet it bloats the code indeed, sorry, commit this patch with the
>> old check against size_in_bits.
>
> The bloat is partially due to other changes (like checking overread on
> bit-level, which I just realize is pointless), it is basically only the %
> s->sys->bpm it adds (though I find that very ugly).
> Which variant do you want, this one for example?:
>
> Index: libavcodec/dv.c
> ===================================================================
> --- libavcodec/dv.c     (revision 20081)
> +++ libavcodec/dv.c     (working copy)
> @@ -1102,8 +1102,17 @@
>               av_log(NULL, AV_LOG_ERROR, "ac bitstream overflow\n");
>       }
>
> -    for (j=0; j<5*s->sys->bpm; j++)
> +    for (j=0; j<5*s->sys->bpm; j++) {
> +       int pos;
> +       int size = pbs[j].size_in_bits>>  3;
>          flush_put_bits(&pbs[j]);
> +       pos = put_bits_count(&pbs[j])>>  3;
> +       if (pos>  size) {
> +           av_log(NULL, AV_LOG_ERROR, "bitstream written beyond buffer size\n");
> +           return -1;
> +       }
> +       memset(pbs[j].buf + pos, 0xff, size - pos);
> +    }
>
>       return 0;
>   }

Yes, this one is nice, I guess we do not have avctx available for av_log 
? If we don't, commit, we can add the context to av_log later.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org