| Message ID | 20200324193615.18487-1-gautamramk@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/jpeg2000dec: error check when processing tlm marker | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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".
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 [...]
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".
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 --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++) {
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(-)