[FFmpeg-devel] [PATCH 1/4] avcodec/prosumer: Check for bytestream eof in decompress()

[James Almer]

On 10/31/2018 9:18 AM, Michael Niedermayer wrote:
> On Wed, Oct 31, 2018 at 12:06:28PM +0100, Hendrik Leppkes wrote:
>> On Wed, Oct 31, 2018 at 12:03 PM Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>>>
>>> Fixes: Infinite loop
>>> Fixes: 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>>  libavcodec/prosumer.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/prosumer.c b/libavcodec/prosumer.c
>>> index 6e98677b55..2fd9880ee1 100644
>>> --- a/libavcodec/prosumer.c
>>> +++ b/libavcodec/prosumer.c
>>> @@ -57,7 +57,7 @@ static int decompress(GetByteContext *gb, int size, PutByteContext *pb, const ui
>>>      b = lut[2 * idx];
>>>
>>>      while (1) {
>>> -        if (bytestream2_get_bytes_left_p(pb) <= 0)
>>> +        if (bytestream2_get_bytes_left_p(pb) <= 0 || bytestream2_get_eof(pb))
>>>              return 0;
>>>          if (((b & 0xFF00u) != 0x8000u) || (b & 0xFFu)) {
>>>              if ((b & 0xFF00u) != 0x8000u) {
>>
>> Why does bytestream2_get_bytes_left_p not return <= 0 at eof, where
>> there certainly arent any bytes left?
> 
> there are 2 bytes left because a 4 byte write failed to fit in 2 bytes
> 
> maybe bytestream2_get_bytes_left_p() should be made to return 0 once eof is
> set even if space remains

Unrelated to this, but seeing how all the main GetByteContext functions
start with bytestream2_get and all the PutByteContext functions with
bytestream2_put, the fact these two functions from the latter start with
bytestream2_get is not ideal.
You have bytestream2_get_bytes_left for GetByteContext and
bytestream2_get_bytes_left_p with the suffix p hinting it's for
PutByteContext, but then you have bytestream2_get_eof for PutByteContext
without a suffix and nothing for GetByteContext. It's confusing and not
exactly intuitive.

In GetBitContext and PutBitContext they are properly called
get_bits_left() and put_bits_left() respectively, for example.