diff mbox series

[FFmpeg-devel] avcodec/jpeg2000dec: error check when processing tlm marker

Message ID 20200324193615.18487-1-gautamramk@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/jpeg2000dec: error check when processing tlm marker
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork warning New warnings during build
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Gautam Ramakrishnan March 24, 2020, 7:36 p.m. UTC
From: Gautam Ramakrishnan <gautamramk@gmail.com>

Validate the value of ST field in the TLM marker of JPEG2000.
Throw an error when ST takes value of 0x11.
---
 libavcodec/jpeg2000dec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Paul B Mahol March 24, 2020, 9:16 p.m. UTC | #1
On 3/24/20, gautamramk@gmail.com <gautamramk@gmail.com> wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
>
> Validate the value of ST field in the TLM marker of JPEG2000.
> Throw an error when ST takes value of 0x11.
> ---
>  libavcodec/jpeg2000dec.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 019dc81f56..74d70b686f 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -803,7 +803,11 @@ static uint8_t get_tlm(Jpeg2000DecoderContext *s, int
> n)
>
>      // too complex ? ST = ((Stlm >> 4) & 0x01) + ((Stlm >> 4) & 0x02);
>      ST = (Stlm >> 4) & 0x03;
> -    // TODO: Manage case of ST = 0b11 --> raise error
> +    if (ST == 0x11) {

0x11 is hex representation of 17 decimal number

above is &0x03 which is 3 in decimal, this ST can never be 0x11.
So patch in pointless.
IF you check for 3 instead does it break any existing files?

> +        av_log(s, AV_LOG_ERROR, "TLM marker contains invalid ST value.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
>      SP       = (Stlm >> 6) & 0x01;
>      tile_tlm = (n - 4) / ((SP + 1) * 2 + ST);
>      for (i = 0; i < tile_tlm; i++) {
> --
> 2.17.1
>
> _______________________________________________
> 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".
Michael Niedermayer March 25, 2020, 12:36 a.m. UTC | #2
On Wed, Mar 25, 2020 at 01:06:15AM +0530, gautamramk@gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk@gmail.com>
> 
> Validate the value of ST field in the TLM marker of JPEG2000.
> Throw an error when ST takes value of 0x11.
> ---
>  libavcodec/jpeg2000dec.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 019dc81f56..74d70b686f 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -803,7 +803,11 @@ static uint8_t get_tlm(Jpeg2000DecoderContext *s, int n)
>  
>      // too complex ? ST = ((Stlm >> 4) & 0x01) + ((Stlm >> 4) & 0x02);
>      ST = (Stlm >> 4) & 0x03;
> -    // TODO: Manage case of ST = 0b11 --> raise error
> +    if (ST == 0x11) {
> +        av_log(s, AV_LOG_ERROR, "TLM marker contains invalid ST value.\n");

> +        return AVERROR_INVALIDDATA;

in addition to what paul said,

the return type of get_tlm() is unsigned 8bit the error code doesnt fit in 8bit


[...]
Gautam Ramakrishnan March 25, 2020, 2:45 a.m. UTC | #3
That was an extremely careless error. Apologies for that. Shall fix
that immediately. However, is changing the return type of get_tlm()
the right way?

On Wed, Mar 25, 2020 at 6:06 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Wed, Mar 25, 2020 at 01:06:15AM +0530, gautamramk@gmail.com wrote:
> > From: Gautam Ramakrishnan <gautamramk@gmail.com>
> >
> > Validate the value of ST field in the TLM marker of JPEG2000.
> > Throw an error when ST takes value of 0x11.
> > ---
> >  libavcodec/jpeg2000dec.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> > index 019dc81f56..74d70b686f 100644
> > --- a/libavcodec/jpeg2000dec.c
> > +++ b/libavcodec/jpeg2000dec.c
> > @@ -803,7 +803,11 @@ static uint8_t get_tlm(Jpeg2000DecoderContext *s, int n)
> >
> >      // too complex ? ST = ((Stlm >> 4) & 0x01) + ((Stlm >> 4) & 0x02);
> >      ST = (Stlm >> 4) & 0x03;
> > -    // TODO: Manage case of ST = 0b11 --> raise error
> > +    if (ST == 0x11) {
> > +        av_log(s, AV_LOG_ERROR, "TLM marker contains invalid ST value.\n");
>
> > +        return AVERROR_INVALIDDATA;
>
> in addition to what paul said,
>
> the return type of get_tlm() is unsigned 8bit the error code doesnt fit in 8bit
>
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The day soldiers stop bringing you their problems is the day you have stopped
> leading them. They have either lost confidence that you can help or concluded
> you do not care. Either case is a failure of leadership. - Colin Powell
> _______________________________________________
> 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".
Michael Niedermayer March 25, 2020, 5:46 p.m. UTC | #4
On Wed, Mar 25, 2020 at 08:15:11AM +0530, Gautam Ramakrishnan wrote:
> That was an extremely careless error. Apologies for that. Shall fix
> that immediately. However, is changing the return type of get_tlm()
> the right way?

its probably a good idea to change the return type to int


thanks

[...]
diff mbox series

Patch

diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 019dc81f56..74d70b686f 100644
--- a/libavcodec/jpeg2000dec.c
+++ b/libavcodec/jpeg2000dec.c
@@ -803,7 +803,11 @@  static uint8_t get_tlm(Jpeg2000DecoderContext *s, int n)
 
     // too complex ? ST = ((Stlm >> 4) & 0x01) + ((Stlm >> 4) & 0x02);
     ST = (Stlm >> 4) & 0x03;
-    // TODO: Manage case of ST = 0b11 --> raise error
+    if (ST == 0x11) {
+        av_log(s, AV_LOG_ERROR, "TLM marker contains invalid ST value.\n");
+        return AVERROR_INVALIDDATA;
+    }
+
     SP       = (Stlm >> 6) & 0x01;
     tile_tlm = (n - 4) / ((SP + 1) * 2 + ST);
     for (i = 0; i < tile_tlm; i++) {