[FFmpeg-devel] [PATCH 1/3] avcodec/cbs_jpeg: Assert that cbs_jpeg_assemble_fragment() stays within the array

[FFmpeg-devel] [PATCH 1/3] avcodec/cbs_jpeg: Assert that cbs_jpeg_assemble_fragment() stays within the array

[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);
             }
         }
     }
-- 
2.43.2

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

[FFmpeg-devel] [PATCH 2/3] avformat/sdp: Check before appending ", "

[Michael Niedermayer]

Found by reviewing code related to CID1500301 String not null terminated

Sponsored-by: Sovereign Tech Fund
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/sdp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/sdp.c b/libavformat/sdp.c
index 7e11a759185..ccfaa8aff58 100644
--- a/libavformat/sdp.c
+++ b/libavformat/sdp.c
@@ -203,6 +203,8 @@ static int extradata2psets(AVFormatContext *s, const AVCodecParameters *par,
             continue;
         }
         if (p != (psets + strlen(pset_string))) {
+            if (p - psets >= MAX_PSET_SIZE)
+                goto fail_in_loop;
             *p = ',';
             p++;
         }
@@ -213,6 +215,7 @@ static int extradata2psets(AVFormatContext *s, const AVCodecParameters *par,
         if (!av_base64_encode(p, MAX_PSET_SIZE - (p - psets), r, r1 - r)) {
             av_log(s, AV_LOG_ERROR, "Cannot Base64-encode %"PTRDIFF_SPECIFIER" %"PTRDIFF_SPECIFIER"!\n",
                    MAX_PSET_SIZE - (p - psets), r1 - r);
+fail_in_loop:
             av_free(psets);
             av_free(tmpbuf);
 
-- 
2.43.2

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

Re: [FFmpeg-devel] [PATCH 2/3] avformat/sdp: Check before appending ", "

[Michael Niedermayer]

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

On Wed, May 08, 2024 at 04:39:22AM +0200, Michael Niedermayer wrote:
> Found by reviewing code related to CID1500301 String not null terminated
> 
> Sponsored-by: Sovereign Tech Fund
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/sdp.c | 3 +++
>  1 file changed, 3 insertions(+)

will apply

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

[FFmpeg-devel] [PATCH 3/3] avformat/img2dec: assert no pipe on ts_from_file

[Michael Niedermayer]

Help coverity with CID500302 Uninitialized scalar variable

Sponsored-by: Sovereign Tech Fund
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/img2dec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
index a40675d4341..6220003fb4d 100644
--- a/libavformat/img2dec.c
+++ b/libavformat/img2dec.c
@@ -25,6 +25,7 @@
 #define _DEFAULT_SOURCE
 #define _BSD_SOURCE
 #include <sys/stat.h>
+#include "libavutil/avassert.h"
 #include "libavutil/avstring.h"
 #include "libavutil/log.h"
 #include "libavutil/mem.h"
@@ -504,6 +505,7 @@ int ff_img_read_packet(AVFormatContext *s1, AVPacket *pkt)
     pkt->flags       |= AV_PKT_FLAG_KEY;
     if (s->ts_from_file) {
         struct stat img_stat;
+        av_assert0(!s->is_pipe);
         if (stat(filename, &img_stat)) {
             res = AVERROR(EIO);
             goto fail;
-- 
2.43.2

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

Re: [FFmpeg-devel] [PATCH 3/3] avformat/img2dec: assert no pipe on ts_from_file

[Andreas Rheinhardt]

Michael Niedermayer:
> Help coverity with CID500302 Uninitialized scalar variable
> 
> Sponsored-by: Sovereign Tech Fund
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/img2dec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index a40675d4341..6220003fb4d 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -25,6 +25,7 @@
>  #define _DEFAULT_SOURCE
>  #define _BSD_SOURCE
>  #include <sys/stat.h>
> +#include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/log.h"
>  #include "libavutil/mem.h"
> @@ -504,6 +505,7 @@ int ff_img_read_packet(AVFormatContext *s1, AVPacket *pkt)
>      pkt->flags       |= AV_PKT_FLAG_KEY;
>      if (s->ts_from_file) {
>          struct stat img_stat;
> +        av_assert0(!s->is_pipe);
>          if (stat(filename, &img_stat)) {
>              res = AVERROR(EIO);
>              goto fail;

1. You seem to refer to 1500302 which I can't even select any more
because it has been dismissed.
2. Why are you using av_assert0 to help coverity?
3. In cases like these it would be important to actually note why this
assert can't be triggered. With a comment like "The ts_from_file option
is not supported by piped input demuxers".
4. I just sent an alternative for this:
https://ffmpeg.org/pipermail/ffmpeg-devel/2024-May/327035.html. I don't
know which one should be preferred or whether the code should be left as-is.

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

Re: [FFmpeg-devel] [PATCH 3/3] avformat/img2dec: assert no pipe on ts_from_file

[Michael Niedermayer]

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

On Wed, May 08, 2024 at 12:16:44PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Help coverity with CID500302 Uninitialized scalar variable
> > 
> > Sponsored-by: Sovereign Tech Fund
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/img2dec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > index a40675d4341..6220003fb4d 100644
> > --- a/libavformat/img2dec.c
> > +++ b/libavformat/img2dec.c
> > @@ -25,6 +25,7 @@
> >  #define _DEFAULT_SOURCE
> >  #define _BSD_SOURCE
> >  #include <sys/stat.h>
> > +#include "libavutil/avassert.h"
> >  #include "libavutil/avstring.h"
> >  #include "libavutil/log.h"
> >  #include "libavutil/mem.h"
> > @@ -504,6 +505,7 @@ int ff_img_read_packet(AVFormatContext *s1, AVPacket *pkt)
> >      pkt->flags       |= AV_PKT_FLAG_KEY;
> >      if (s->ts_from_file) {
> >          struct stat img_stat;
> > +        av_assert0(!s->is_pipe);
> >          if (stat(filename, &img_stat)) {
> >              res = AVERROR(EIO);
> >              goto fail;
> 
> 1. You seem to refer to 1500302

yes, fixed locally


> which I can't even select any more
> because it has been dismissed.

no, it can be selected, it works here on the 3rd try
coverity sometimes ignores selecting issues
sometimes it displayes a long list of unrelated issues
sometimes it works


> 2. Why are you using av_assert0 to help coverity?

I dont understand the question


> 3. In cases like these it would be important to actually note why this
> assert can't be triggered. With a comment like "The ts_from_file option
> is not supported by piped input demuxers".

its kind of clear from the assert but i added the comment,


> 4. I just sent an alternative for this:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2024-May/327035.html. I don't
> know which one should be preferred or whether the code should be left as-is.

I think if somehow ts_from_file and is_pipe are set. Thats a bug in the code
its only the combination of flags and AVOptions that prevents this.
I think an assert like in my patch makes the code more robust compared to
simply not crashing on an inconsistant state

thx

[...]

--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

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

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_jpeg: Assert that cbs_jpeg_assemble_fragment() stays within the array

[Andreas Rheinhardt]

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avcodec/cbs_jpeg: Assert that cbs_jpeg_assemble_fragment() stays within the array

[Michael Niedermayer]

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

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Take away the freedom of one citizen and you will be jailed, take away
the freedom of all citizens and you will be congratulated by your peers
in Parliament.

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