| Message ID | 20240508023923.28209-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/cbs_jpeg: Assert that cbs_jpeg_assemble_fragment() stays within the array | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Michael Niedermayer: > Inspired by: CID1473561 Untrusted pointer write > > Sponsored-by: Sovereign Tech Fund > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cbs_jpeg.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c > index b1b58dcd65e..628841c5f37 100644 > --- a/libavcodec/cbs_jpeg.c > +++ b/libavcodec/cbs_jpeg.c > @@ -401,6 +401,7 @@ static int cbs_jpeg_assemble_fragment(CodedBitstreamContext *ctx, > } else { > data[dp++] = unit->data[sp]; > } > + av_assert0(dp <= size - 2); > } > } > } You want to add an av_assert0 to a hot loop (or rather: to what would be a hot loop in case this code were executed) just because Coverity thinks that reading data with a different endianness taints this data? (That the supposedly tainted variable has actually already been checked via an assert makes this even more crazy.) - Andreas
On Wed, May 08, 2024 at 11:46:45AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Inspired by: CID1473561 Untrusted pointer write > > > > Sponsored-by: Sovereign Tech Fund > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/cbs_jpeg.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c > > index b1b58dcd65e..628841c5f37 100644 > > --- a/libavcodec/cbs_jpeg.c > > +++ b/libavcodec/cbs_jpeg.c > > @@ -401,6 +401,7 @@ static int cbs_jpeg_assemble_fragment(CodedBitstreamContext *ctx, > > } else { > > data[dp++] = unit->data[sp]; > > } > > + av_assert0(dp <= size - 2); > > } > > } > > } > > You want to add an av_assert0 to a hot loop (or rather: to what would be > a hot loop in case this code were executed) just because Coverity thinks > that reading data with a different endianness taints this data? (That > the supposedly tainted variable has actually already been checked via an > assert makes this even more crazy.) patch droped But the code is fragile, there are 2 loops that must match exactly if what the second writes doesnt match what the first counts it writes out of array This just needs someone finding a bug in the loop and fix it without updating the 2nd loop thx [...]
diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b1b58dcd65e..628841c5f37 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -401,6 +401,7 @@ static int cbs_jpeg_assemble_fragment(CodedBitstreamContext *ctx, } else { data[dp++] = unit->data[sp]; } + av_assert0(dp <= size - 2); } } }
Inspired by: CID1473561 Untrusted pointer write Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_jpeg.c | 1 + 1 file changed, 1 insertion(+)