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

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

[Michael Niedermayer]

Fixes: out of array access
Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b

Found-by: Catena cyber <contact@catenacyber.fr>
Signed-off-by: Michael Niedermayer <michael@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 */
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: out of array access
> Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
> 
> Found-by: Catena cyber <contact@catenacyber.fr>
> Signed-off-by: Michael Niedermayer <michael@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.)

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1773 bytes --]

On Sat, Jun 24, 2023 at 09:14:53PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: out of array access
> > Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
> > 
> > Found-by: Catena cyber <contact@catenacyber.fr>
> > Signed-off-by: Michael Niedermayer <michael@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.)

truehd with a malloc() failure
and i dont think it should happen but, it felt like a good idea to check

also *buf_size should probably be reset to 0, there was a patch about that
from reimar (in CC)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@outlook.com> wrote:
> 
> Michael Niedermayer:
>> Fixes: out of array access
>> Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
>> 
>> Found-by: Catena cyber <contact@catenacyber.fr>
>> Signed-off-by: Michael Niedermayer <michael@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);

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2610 bytes --]

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@outlook.com> wrote:
> > 
> > Michael Niedermayer:
> >> Fixes: out of array access
> >> Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
> >> 
> >> Found-by: Catena cyber <contact@catenacyber.fr>
> >> Signed-off-by: Michael Niedermayer <michael@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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@outlook.com> wrote:
>>
>> Michael Niedermayer:
>>> Fixes: out of array access
>>> Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
>>>
>>> Found-by: Catena cyber <contact@catenacyber.fr>
>>> Signed-off-by: Michael Niedermayer <michael@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?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".