| Message ID | 20230622003038.20969-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/parser: Check next against buffer index | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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 [...]
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);
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 [...]
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?
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 */
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(-)