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

[Reimar Döffinger]

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);