Re: [FFmpeg-devel] [PATCH 7/7] avcodec/cbs_jpeg: Try to move the read entity to one side in a test

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Thu, May 02, 2024 at 08:58:17AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: CID1439654 Untrusted pointer read
> > 
> > Sponsored-by: Sovereign Tech Fund
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/cbs_jpeg.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c
> > index b1b58dcd65e..406147c082c 100644
> > --- a/libavcodec/cbs_jpeg.c
> > +++ b/libavcodec/cbs_jpeg.c
> > @@ -146,13 +146,13 @@ static int cbs_jpeg_split_fragment(CodedBitstreamContext *ctx,
> >              }
> >          } else {
> >              i = start;
> > -            if (i + 2 > frag->data_size) {
> > +            if (i > frag->data_size - 2) {
> >                  av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: "
> >                         "truncated at %02x marker.\n", marker);
> >                  return AVERROR_INVALIDDATA;
> >              }
> >              length = AV_RB16(frag->data + i);
> > -            if (i + length > frag->data_size) {
> > +            if (length > frag->data_size - i) {
> >                  av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid JPEG image: "
> >                         "truncated at %02x marker segment.\n", marker);
> >                  return AVERROR_INVALIDDATA;
> 
> You should always mention when you are not fixing bugs in our code, but
> rather intend to apply workaround for coverity crazyness (i.e. the
> requirement that reading values in non-native endianness needs to be
> sanitized).

writing code like
if (i + length > frag->data_size)

is IMHO not proper and that has nothing to do with coverity
the reason why this is not proper is that i + length could in
principle overflow. It depends on teh whole loop and calling code
if that is possible or not.

to check length, length should be alone on one side of the check

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.

[-- 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".