| Message ID | 20240320025923.3794-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/jpeg2000htdec: Check M_b / magp before using it in a shift | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
ons 2024-03-20 klockan 03:59 +0100 skrev Michael Niedermayer: > Fixes: shift exponent -1 is negative > Fixes: 65378/clusterfuzz-testcase-minimized- > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/jpeg2000htdec.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c > index 6b9898d3ff..0b94bb5da2 100644 > --- a/libavcodec/jpeg2000htdec.c > +++ b/libavcodec/jpeg2000htdec.c > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > int32_t M_b = magp; > > + if (magp >= 31) > + return AVERROR_INVALIDDATA; This isn't where the error is, assuming it even is an error. It's either expn or nguardbits that are wrong, and they should be detected and reported as such in jpeg2000dec.c. Checking this in every call to ff_jpeg2000_decode_htj2k() is wasteful. nguardbits can be 0..7 and expn can be 0..31. Table A.11 indicates that Ssize can be up to 38 bits, so M_b >= 31 is in fact perfectly valid. A more appropriate error might be AVERROR_PATCHWELCOME. While we're on the topic, it seems get_qcx() could be more strict when it comes to the size of JPEG2000_QCD. I see some TODOs around this stuff as well, init_band_stepsize() /Tomas
On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > ons 2024-03-20 klockan 03:59 +0100 skrev Michael Niedermayer: > > Fixes: shift exponent -1 is negative > > Fixes: 65378/clusterfuzz-testcase-minimized- > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/jpeg2000htdec.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c > > index 6b9898d3ff..0b94bb5da2 100644 > > --- a/libavcodec/jpeg2000htdec.c > > +++ b/libavcodec/jpeg2000htdec.c > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > int32_t M_b = magp; > > > > + if (magp >= 31) > > + return AVERROR_INVALIDDATA; > > This isn't where the error is, assuming it even is an error. It's > either expn or nguardbits that are wrong, and they should be detected > and reported as such in jpeg2000dec.c. Checking this in every call to > ff_jpeg2000_decode_htj2k() is wasteful. > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 indicates that > Ssize can be up to 38 bits, so M_b >= 31 is in fact perfectly valid. > A > more appropriate error might be AVERROR_PATCHWELCOME. indeed, i will change it to AVERROR_PATCHWELCOME thx [...]
ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer: > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael Niedermayer: > > > Fixes: shift exponent -1 is negative > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > b/libavcodec/jpeg2000htdec.c > > > index 6b9898d3ff..0b94bb5da2 100644 > > > --- a/libavcodec/jpeg2000htdec.c > > > +++ b/libavcodec/jpeg2000htdec.c > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > int32_t M_b = magp; > > > > > > + if (magp >= 31) > > > + return AVERROR_INVALIDDATA; > > > > This isn't where the error is, assuming it even is an error. It's > > either expn or nguardbits that are wrong, and they should be > > detected > > and reported as such in jpeg2000dec.c. Checking this in every call > > to > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 indicates > > that > > Ssize can be up to 38 bits, so M_b >= 31 is in fact perfectly > > valid. > > > A > > more appropriate error might be AVERROR_PATCHWELCOME. > > indeed, i will change it to AVERROR_PATCHWELCOME Please also move it further up so as to not waste cycles checking it every time /Tomas
ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin: > ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer: > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael Niedermayer: > > > > Fixes: shift exponent -1 is negative > > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > > > Found-by: continuous fuzzing process > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > > b/libavcodec/jpeg2000htdec.c > > > > index 6b9898d3ff..0b94bb5da2 100644 > > > > --- a/libavcodec/jpeg2000htdec.c > > > > +++ b/libavcodec/jpeg2000htdec.c > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > > > int32_t M_b = magp; > > > > > > > > + if (magp >= 31) > > > > + return AVERROR_INVALIDDATA; > > > > > > This isn't where the error is, assuming it even is an error. It's > > > either expn or nguardbits that are wrong, and they should be > > > detected > > > and reported as such in jpeg2000dec.c. Checking this in every > > > call > > > to > > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 > > > indicates > > > that > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact perfectly > > > valid. > > > > > A > > > more appropriate error might be AVERROR_PATCHWELCOME. > > > > indeed, i will change it to AVERROR_PATCHWELCOME > > Please also move it further up so as to not waste cycles checking it > every time To be more precise, get_qcx() looks like the proper place for it /Tomas
On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote: > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin: > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer: > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael Niedermayer: > > > > > Fixes: shift exponent -1 is negative > > > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > > > > > Found-by: continuous fuzzing process > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > > > b/libavcodec/jpeg2000htdec.c > > > > > index 6b9898d3ff..0b94bb5da2 100644 > > > > > --- a/libavcodec/jpeg2000htdec.c > > > > > +++ b/libavcodec/jpeg2000htdec.c > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > > > > > int32_t M_b = magp; > > > > > > > > > > + if (magp >= 31) > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > This isn't where the error is, assuming it even is an error. It's > > > > either expn or nguardbits that are wrong, and they should be > > > > detected > > > > and reported as such in jpeg2000dec.c. Checking this in every > > > > call > > > > to > > > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 > > > > indicates > > > > that > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact perfectly > > > > valid. > > > > > > > A > > > > more appropriate error might be AVERROR_PATCHWELCOME. > > > > > > indeed, i will change it to AVERROR_PATCHWELCOME > > > > Please also move it further up so as to not waste cycles checking it > > every time > > To be more precise, get_qcx() looks like the proper place for it will apply with teh check moved there thx [...]
On Mon, Mar 25, 2024 at 08:13:13PM +0100, Michael Niedermayer wrote: > On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote: > > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin: > > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer: > > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael Niedermayer: > > > > > > Fixes: shift exponent -1 is negative > > > > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > --- > > > > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > > > > b/libavcodec/jpeg2000htdec.c > > > > > > index 6b9898d3ff..0b94bb5da2 100644 > > > > > > --- a/libavcodec/jpeg2000htdec.c > > > > > > +++ b/libavcodec/jpeg2000htdec.c > > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > > > > > > > int32_t M_b = magp; > > > > > > > > > > > > + if (magp >= 31) > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > This isn't where the error is, assuming it even is an error. It's > > > > > either expn or nguardbits that are wrong, and they should be > > > > > detected > > > > > and reported as such in jpeg2000dec.c. Checking this in every > > > > > call > > > > > to > > > > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 > > > > > indicates > > > > > that > > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact perfectly > > > > > valid. > > > > > > > > > A > > > > > more appropriate error might be AVERROR_PATCHWELCOME. > > > > > > > > indeed, i will change it to AVERROR_PATCHWELCOME > > > > > > Please also move it further up so as to not waste cycles checking it > > > every time > > > > To be more precise, get_qcx() looks like the proper place for it > > will apply with teh check moved there the values that are causing undefined behavior for htj2k are used in normal j2k knowing which type of j2k we have seems decided by COC/COD/COX so i dont think we can check in QCX, because a later COX could make it both invalid or valid and we cannot check in COX as a later QCX can similarly change it thx [...]
mån 2024-03-25 klockan 21:04 +0100 skrev Michael Niedermayer: > On Mon, Mar 25, 2024 at 08:13:13PM +0100, Michael Niedermayer wrote: > > On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote: > > > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin: > > > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer: > > > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > > > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael > > > > > > Niedermayer: > > > > > > > Fixes: shift exponent -1 is negative > > > > > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > <michael@niedermayer.cc> > > > > > > > --- > > > > > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > > > > > b/libavcodec/jpeg2000htdec.c > > > > > > > index 6b9898d3ff..0b94bb5da2 100644 > > > > > > > --- a/libavcodec/jpeg2000htdec.c > > > > > > > +++ b/libavcodec/jpeg2000htdec.c > > > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > > > > > > > > > int32_t M_b = magp; > > > > > > > > > > > > > > + if (magp >= 31) > > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > > > This isn't where the error is, assuming it even is an > > > > > > error. It's > > > > > > either expn or nguardbits that are wrong, and they should > > > > > > be > > > > > > detected > > > > > > and reported as such in jpeg2000dec.c. Checking this in > > > > > > every > > > > > > call > > > > > > to > > > > > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > > > > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 > > > > > > indicates > > > > > > that > > > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact > > > > > > perfectly > > > > > > valid. > > > > > > > > > > > A > > > > > > more appropriate error might be AVERROR_PATCHWELCOME. > > > > > > > > > > indeed, i will change it to AVERROR_PATCHWELCOME > > > > > > > > Please also move it further up so as to not waste cycles > > > > checking it > > > > every time > > > > > > To be more precise, get_qcx() looks like the proper place for it > > > > will apply with teh check moved there > > the values that are causing undefined behavior for htj2k are used in > normal > j2k knowing which type of j2k we have seems decided by COC/COD/COX > > so i dont think we can check in QCX, because a later COX could > make it both invalid or valid > and we cannot check in COX as a later QCX can similarly change it That all calls get_qcx(). If you git grep for nguardbits you'll see it's only ever set there when decoding, and similarly with expn. Coding style and quantization style are not the same thing. /Tomas
On Wed, Mar 27, 2024 at 11:13:48AM +0100, Tomas Härdin wrote: > mån 2024-03-25 klockan 21:04 +0100 skrev Michael Niedermayer: > > On Mon, Mar 25, 2024 at 08:13:13PM +0100, Michael Niedermayer wrote: > > > On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote: > > > > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin: > > > > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer: > > > > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote: > > > > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael > > > > > > > Niedermayer: > > > > > > > > Fixes: shift exponent -1 is negative > > > > > > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > > <michael@niedermayer.cc> > > > > > > > > --- > > > > > > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > > > > > > b/libavcodec/jpeg2000htdec.c > > > > > > > > index 6b9898d3ff..0b94bb5da2 100644 > > > > > > > > --- a/libavcodec/jpeg2000htdec.c > > > > > > > > +++ b/libavcodec/jpeg2000htdec.c > > > > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > > > > > > > > > > > int32_t M_b = magp; > > > > > > > > > > > > > > > > + if (magp >= 31) > > > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > > > > > This isn't where the error is, assuming it even is an > > > > > > > error. It's > > > > > > > either expn or nguardbits that are wrong, and they should > > > > > > > be > > > > > > > detected > > > > > > > and reported as such in jpeg2000dec.c. Checking this in > > > > > > > every > > > > > > > call > > > > > > > to > > > > > > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > > > > > > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11 > > > > > > > indicates > > > > > > > that > > > > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact > > > > > > > perfectly > > > > > > > valid. > > > > > > > > > > > > > A > > > > > > > more appropriate error might be AVERROR_PATCHWELCOME. > > > > > > > > > > > > indeed, i will change it to AVERROR_PATCHWELCOME > > > > > > > > > > Please also move it further up so as to not waste cycles > > > > > checking it > > > > > every time > > > > > > > > To be more precise, get_qcx() looks like the proper place for it > > > > > > will apply with teh check moved there > > > > the values that are causing undefined behavior for htj2k are used in > > normal > > j2k knowing which type of j2k we have seems decided by COC/COD/COX > > > > so i dont think we can check in QCX, because a later COX could > > make it both invalid or valid > > and we cannot check in COX as a later QCX can similarly change it > > That all calls get_qcx(). yes > If you git grep for nguardbits you'll see > it's only ever set there when decoding, and similarly with expn. yes > Coding > style and quantization style are not the same thing. yes but still, you can try to add a check the values for both nguardbits and expn which lead to undefined shifts in ff_jpeg2000_decode_htj2k() are used in normal jpeg2000 and break these samples because the same get_qcx() is used both for "normal" jpeg2000 and htj2k so the check needs to know if its htj2k inside ff_jpeg2000_decode_htj2k() thats obvious, in get_qcx() its not one can use the coding style to tell them apart but thats not trivial with the various ways to slice and dice things and it might not be set when get_qcx() is run if iam missing something please tell me how to know in get_qcx() that teh data is only used for ht2jk (Its very possible iam missing something given i did not read the hundreads of pages of spec, and mostly assume that its as insane as possible) thx [...]
tor 2024-03-28 klockan 03:48 +0100 skrev Michael Niedermayer: > On Wed, Mar 27, 2024 at 11:13:48AM +0100, Tomas Härdin wrote: > > mån 2024-03-25 klockan 21:04 +0100 skrev Michael Niedermayer: > > > On Mon, Mar 25, 2024 at 08:13:13PM +0100, Michael Niedermayer > > > wrote: > > > > On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote: > > > > > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin: > > > > > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael > > > > > > Niedermayer: > > > > > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin > > > > > > > wrote: > > > > > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael > > > > > > > > Niedermayer: > > > > > > > > > Fixes: shift exponent -1 is negative > > > > > > > > > Fixes: 65378/clusterfuzz-testcase-minimized- > > > > > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 > > > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > > > > Signed-off-by: Michael Niedermayer > > > > > > > > > <michael@niedermayer.cc> > > > > > > > > > --- > > > > > > > > > libavcodec/jpeg2000htdec.c | 3 +++ > > > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c > > > > > > > > > b/libavcodec/jpeg2000htdec.c > > > > > > > > > index 6b9898d3ff..0b94bb5da2 100644 > > > > > > > > > --- a/libavcodec/jpeg2000htdec.c > > > > > > > > > +++ b/libavcodec/jpeg2000htdec.c > > > > > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const > > > > > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c > > > > > > > > > > > > > > > > > > int32_t M_b = magp; > > > > > > > > > > > > > > > > > > + if (magp >= 31) > > > > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > > > > > > > This isn't where the error is, assuming it even is an > > > > > > > > error. It's > > > > > > > > either expn or nguardbits that are wrong, and they > > > > > > > > should > > > > > > > > be > > > > > > > > detected > > > > > > > > and reported as such in jpeg2000dec.c. Checking this in > > > > > > > > every > > > > > > > > call > > > > > > > > to > > > > > > > > ff_jpeg2000_decode_htj2k() is wasteful. > > > > > > > > > > > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table > > > > > > > > A.11 > > > > > > > > indicates > > > > > > > > that > > > > > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact > > > > > > > > perfectly > > > > > > > > valid. > > > > > > > > > > > > > > > A > > > > > > > > more appropriate error might be AVERROR_PATCHWELCOME. > > > > > > > > > > > > > > indeed, i will change it to AVERROR_PATCHWELCOME > > > > > > > > > > > > Please also move it further up so as to not waste cycles > > > > > > checking it > > > > > > every time > > > > > > > > > > To be more precise, get_qcx() looks like the proper place for > > > > > it > > > > > > > > will apply with teh check moved there > > > > > > the values that are causing undefined behavior for htj2k are used > > > in > > > normal > > > j2k knowing which type of j2k we have seems decided by > > > COC/COD/COX > > > > > > so i dont think we can check in QCX, because a later COX could > > > make it both invalid or valid > > > and we cannot check in COX as a later QCX can similarly change it > > > > That all calls get_qcx(). > > yes > > > > If you git grep for nguardbits you'll see > > it's only ever set there when decoding, and similarly with expn. > > yes > > > > Coding > > style and quantization style are not the same thing. > > yes > > > but still, you can try to add a check the values for both nguardbits > and expn which lead to undefined shifts in ff_jpeg2000_decode_htj2k() > are used in normal jpeg2000 and break these samples Hum hum.. You seem to be speaking of cblk->nonzerobits which is used in decode_cblk() for the Part 1 decoder. It only accepts bpno in 0..29. bpno in turn depends on roi_shift which is never negative. bpno = expn + nguardbits + roi_shift - 1 - zbp But zbp can be up to 100 if I understand correctly, so expn + guardbits can be up to 130 and still be legal Part 1. So yeah, putting the check in get_qcx() isn't right. But it could live in tile_codeblocks() in the bandno loop. As a bonus we only need to compute magp once per subband (the compiler probably already notices this). Interestingly roi_shift is sent to ff_jpeg2000_decode_htj2k() but never used. T.814 seems to indicate ROI is still used for HTJ2K so maybe we should warn if it's non-zero? /Tomas
diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c index 6b9898d3ff..0b94bb5da2 100644 --- a/libavcodec/jpeg2000htdec.c +++ b/libavcodec/jpeg2000htdec.c @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c int32_t M_b = magp; + if (magp >= 31) + return AVERROR_INVALIDDATA; + /* codeblock size as constrained by Rec. ITU-T T.800, Table A.18 */ av_assert0(width <= 1024U && height <= 1024U); av_assert0(width * height <= 4096);
Fixes: shift exponent -1 is negative Fixes: 65378/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/jpeg2000htdec.c | 3 +++ 1 file changed, 3 insertions(+)