[FFmpeg-devel] [PATCH] avcodec/parser: Check next against buffer index

[James Almer]

On 6/24/2023 5:13 PM, Reimar Döffinger wrote:
> Hi!
> 
>> On 24 Jun 2023, at 21:14, Andreas Rheinhardt <andreas.rheinhardt at outlook.com> wrote:
>>
>> Michael Niedermayer:
>>> Fixes: out of array access
>>> Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
>>>
>>> Found-by: Catena cyber <contact at catenacyber.fr>
>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> ---
>>> libavcodec/parser.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>>> index efc28b8918..db39e698ab 100644
>>> --- a/libavcodec/parser.c
>>> +++ b/libavcodec/parser.c
>>> @@ -214,7 +214,7 @@ int ff_combine_frame(ParseContext *pc, int next,
>>>      for (; pc->overread > 0; pc->overread--)
>>>          pc->buffer[pc->index++] = pc->buffer[pc->overread_index++];
>>>
>>> -    if (next > *buf_size)
>>> +    if (next > *buf_size || (next < -pc->index && next != END_NOT_FOUND))
>>>          return AVERROR(EINVAL);
>>>
>>>      /* flush remaining if EOF */
>>
>> Could you provide more details about this? E.g. which parser is this
>> about at all? And how can we actually come in this situation at all?
>> (Whenever I looked at ff_combine_frame() I do not really understand what
>> its invariants are supposed to be.)
> 
> Yeah, when I looked at it I also felt like it has all kinds of
> assumptions/preconditions without which it will break, but those
> are not documented. Not really reviewable for correctness.
> The change I proposed to fix the same issue was as below. But
> note that it is not based on actual understanding, just that generally
> index, overread_index and buf_size are updated together so I
> thought it suspicious buf_size was not updated on realloc failure.
> --- a/libavcodec/parser.c
> +++ b/libavcodec/parser.c
> @@ -252,6 +252,7 @@ int ff_combine_frame(ParseContext *pc, int next,
>                                             AV_INPUT_BUFFER_PADDING_SIZE);
>          if (!new_buffer) {
>              av_log(NULL, AV_LOG_ERROR, "Failed to reallocate parser buffer to %d\n", next + pc->index + AV_INPUT_BUFFER_PADDING_SIZE);
> +            *buf_size =
>              pc->overread_index =
>              pc->index = 0;
>              return AVERROR(ENOMEM);

*buf_size is, by most parsers, the value returned to the caller if 
ff_combine_frame() fails. This change means that they will now return 0 
on realloc failure, which is interpreted as 0 bytes having been read 
from the input. Is this desirable and/or intended?