| Message ID | 20240504235200.2875183-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/flac_parser: Assert that we do not overrun the link_penalty array | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Le 5 mai 2024 02:51:59 GMT+03:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : >Fixes: CID1454676 Out-of-bounds read It's a stretch to call this "fixing". It just asserts that the situation doesn't happen, in other words, that it is a false positive from the static analyser. The code change looks OK, but the commit description seems misleading. > >Sponsored-by: Sovereign Tech Fund >Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >--- > libavcodec/flac_parser.c | 2 ++ > 1 file changed, 2 insertions(+) > >diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c >index 47904d515a6..d9c47801f83 100644 >--- a/libavcodec/flac_parser.c >+++ b/libavcodec/flac_parser.c >@@ -518,6 +518,8 @@ static int check_header_mismatch(FLACParseContext *fpc, > for (i = 0; i < FLAC_MAX_SEQUENTIAL_HEADERS && curr != child; i++) > curr = curr->next; > >+ av_assert0(i < FLAC_MAX_SEQUENTIAL_HEADERS); >+ > if (header->link_penalty[i] < FLAC_HEADER_CRC_FAIL_PENALTY || > header->link_penalty[i] == FLAC_HEADER_NOT_PENALIZED_YET) { > FLACHeaderMarker *start, *end;
Hi On Mon, May 13, 2024 at 09:07:50AM +0300, Rémi Denis-Courmont wrote: > > > Le 5 mai 2024 02:51:59 GMT+03:00, Michael Niedermayer <michael@niedermayer.cc> a écrit : > >Fixes: CID1454676 Out-of-bounds read > > It's a stretch to call this "fixing". It just asserts that the situation doesn't happen, yes > in other words, that it is a false positive from the static analyser. thanks for reviewing > > The code change looks OK, but the commit description seems misleading. will apply with "Helps: CID1454676 Out-of-bounds read" instead if "Fixing: ..." thx [...]
Michael Niedermayer: > Fixes: CID1454676 Out-of-bounds read > > Sponsored-by: Sovereign Tech Fund > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/flac_parser.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c > index 47904d515a6..d9c47801f83 100644 > --- a/libavcodec/flac_parser.c > +++ b/libavcodec/flac_parser.c > @@ -518,6 +518,8 @@ static int check_header_mismatch(FLACParseContext *fpc, > for (i = 0; i < FLAC_MAX_SEQUENTIAL_HEADERS && curr != child; i++) > curr = curr->next; > > + av_assert0(i < FLAC_MAX_SEQUENTIAL_HEADERS); > + > if (header->link_penalty[i] < FLAC_HEADER_CRC_FAIL_PENALTY || > header->link_penalty[i] == FLAC_HEADER_NOT_PENALIZED_YET) { > FLACHeaderMarker *start, *end; If this is only supposed to mark an issue as invalid for the sanitizer, why are you adding an av_assert0 instead of av_assert1 here (and in other patches)? - Andreas
On Mon, May 13, 2024 at 10:45:16PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: CID1454676 Out-of-bounds read > > > > Sponsored-by: Sovereign Tech Fund > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/flac_parser.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c > > index 47904d515a6..d9c47801f83 100644 > > --- a/libavcodec/flac_parser.c > > +++ b/libavcodec/flac_parser.c > > @@ -518,6 +518,8 @@ static int check_header_mismatch(FLACParseContext *fpc, > > for (i = 0; i < FLAC_MAX_SEQUENTIAL_HEADERS && curr != child; i++) > > curr = curr->next; > > > > + av_assert0(i < FLAC_MAX_SEQUENTIAL_HEADERS); > > + > > if (header->link_penalty[i] < FLAC_HEADER_CRC_FAIL_PENALTY || > > header->link_penalty[i] == FLAC_HEADER_NOT_PENALIZED_YET) { > > FLACHeaderMarker *start, *end; > > If this is only supposed to mark an issue as invalid for the sanitizer, > why are you adding an av_assert0 instead of av_assert1 here The flac parser code is complex and confusing me a bit If i would write av_assert1() then i would be saying that iam 100% sure this is true and i certainly do not feel that confident. Thats why its av_assert0 and also why i have neither marked this in coverity a false positive nor a bug. I was hoping posting this to the mailing list would result in either someone confirming it to be correct or telling me that iam an idiot and that this is wrong. And it seemed remi agreed that the change is correct so i intended to push it but iam happy to wait if you or someone else wants to take a look thx > (and in > other patches)? > > - 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". >
diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c index 47904d515a6..d9c47801f83 100644 --- a/libavcodec/flac_parser.c +++ b/libavcodec/flac_parser.c @@ -518,6 +518,8 @@ static int check_header_mismatch(FLACParseContext *fpc, for (i = 0; i < FLAC_MAX_SEQUENTIAL_HEADERS && curr != child; i++) curr = curr->next; + av_assert0(i < FLAC_MAX_SEQUENTIAL_HEADERS); + if (header->link_penalty[i] < FLAC_HEADER_CRC_FAIL_PENALTY || header->link_penalty[i] == FLAC_HEADER_NOT_PENALIZED_YET) { FLACHeaderMarker *start, *end;
Fixes: CID1454676 Out-of-bounds read Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/flac_parser.c | 2 ++ 1 file changed, 2 insertions(+)