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

[Michael Niedermayer]

On Sat, Jun 24, 2023 at 10:13:48PM +0200, 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);

It appears neither of the 2 fixes was applied
this is not ideal

I will apply reimars patch

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240312/bdc2b843/attachment.sig>