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

[Baptiste Coudurier]

Hi,

On 09/22/2009 01:20 AM, Reimar D?ffinger wrote:
> On Tue, Sep 22, 2009 at 12:19:29AM -0700, Baptiste Coudurier wrote:
>> Hi Reimar,
>>
>> On 09/21/2009 05:40 AM, 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.
>>
>> I assume 0xff is correct according to specs, right ?
>
> I don't think the specs say anything about it (except for the header
> DIFs), but since the current code pads the header and audio DIFs by 0xff
> it is consistent... (though flush_put_bits will pad with 0, so it is not
> that consistent).

Well I'd prefer being stricly correct according to specs.

>>> Index: libavcodec/dv.c
>>> ===================================================================
>>> --- libavcodec/dv.c     (revision 19948)
>>> +++ libavcodec/dv.c     (working copy)
>>> @@ -1102,8 +1102,14 @@
>>>                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, size;
>>>           flush_put_bits(&pbs[j]);
>>> +       pos = put_bits_count(&pbs[j])>>   3;
>>> +       size = pbs[j].size_in_bits>>   3;
>>
>> Maybe s->sys->block_sizes[j] makes more sense ?
>
> Not in my opinion. Like this it is clear that it simply pads the
> remainder of the put_bit context. If you use s->sys->block_sizes[j]
> the code is only understandable to someone who knows that this put_bit
> context is somehow related to the block sizes.

In my opinion it is unclear why we are padding the put_bit context and 
for how much. Using s->sys->block_sizes makes it clear that it's a block 
and its size is fixed and where the size is setup.

>>> +       if (pos<   size)
>>> +           memset(pbs[j].buf + pos, 0xff, size - pos);
>>
>> Is it worth to check for pos<  size ?
>
> For speed? No. But if for some reason ever the decoder writes beyond the
> buffer end without the check it would try to overwrite all memory with
> 0xff.

IMHO if this happen it's an error and encoder should return -1 to 
indicate it.

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