On 6/24/2023 5:13 PM, Reimar Döffinger wrote:
Hi!
On 24 Jun 2023, at 21:14, Andreas Rheinhardt <andreas.rheinha...@outlook.com>
wrote:
Michael Niedermayer:
Fixes: out of array access
Fixes: crash-0d640731c7da52415670eb47a2af701cbe2e1a3b
Found-by: Catena cyber <cont...@catenacyber.fr>
Signed-off-by: Michael Niedermayer <mich...@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-requ...@ffmpeg.org with subject "unsubscribe".
