| Message ID | 20230812203116.31057-1-pal@sandflow.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/jpeg2000htdec: check if block decoding will exceed internal precision | 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 |
lör 2023-08-12 klockan 13:31 -0700 skrev pal@sandflow.com: > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > Intended to replace > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230802000135.26482-3-michael@niedermayer.cc/ > with a more accurate block decoding magnitude bound. > > --- > libavcodec/jpeg2000htdec.c | 34 +++++++++++++++++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c > index 2c4cea5dd9..a7329206de 100644 > --- a/libavcodec/jpeg2000htdec.c > +++ b/libavcodec/jpeg2000htdec.c > @@ -572,6 +572,14 @@ static int > jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, > > const uint8_t *vlc_buf = Dcup + Pcup; > > + /* > + * Bound on the recision needed to process the codeblock. The > number of precision > + * decoded bit planes is equal to at most cblk->zbp + 2 since > S_blk = P if > + * there are no placeholder passes or HT Sets and P = cblk->zbp. > See Rec. > + * ITU-T T.814, 7.6. > + */ > + int maxbp = cblk->zbp + 2; > + > /* convert to raster-scan */ > const uint16_t is_border_x = width % 2; > const uint16_t is_border_y = height % 2; > @@ -590,6 +598,12 @@ static int > jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, > goto free; > } > > + /* do we have enough precision, assuming a 32-bit decoding path > */ > + if (maxbp >= 32) { > + return AVERROR_INVALIDDATA; > + goto free; > + } Why not move this check to just after maxbp is declared? That way a bunch of allocations are avoided The rest of the patch looks fine, though I haven't studied the HT spec deeply. The +2 on maxbp is familiar. /Tomas
On Mon, Aug 14, 2023 at 1:14 AM Tomas Härdin <git@haerdin.se> wrote: > > lör 2023-08-12 klockan 13:31 -0700 skrev pal@sandflow.com: > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > Intended to replace > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230802000135.26482-3-michael@niedermayer.cc/ > > with a more accurate block decoding magnitude bound. > > > > --- > > libavcodec/jpeg2000htdec.c | 34 +++++++++++++++++++++++++++++++++- > > 1 file changed, 33 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c > > index 2c4cea5dd9..a7329206de 100644 > > --- a/libavcodec/jpeg2000htdec.c > > +++ b/libavcodec/jpeg2000htdec.c > > @@ -572,6 +572,14 @@ static int > > jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, > > > > const uint8_t *vlc_buf = Dcup + Pcup; > > > > + /* > > + * Bound on the recision needed to process the codeblock. The > > number of > > precision > > > + * decoded bit planes is equal to at most cblk->zbp + 2 since > > S_blk = P if > > + * there are no placeholder passes or HT Sets and P = cblk->zbp. > > See Rec. > > + * ITU-T T.814, 7.6. > > + */ > > + int maxbp = cblk->zbp + 2; > > + > > /* convert to raster-scan */ > > const uint16_t is_border_x = width % 2; > > const uint16_t is_border_y = height % 2; > > @@ -590,6 +598,12 @@ static int > > jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, > > goto free; > > } > > > > + /* do we have enough precision, assuming a 32-bit decoding path > > */ > > + if (maxbp >= 32) { > > + return AVERROR_INVALIDDATA; > > + goto free; > > + } > > Why not move this check to just after maxbp is declared? That way a > bunch of allocations are avoided I had it that way but then the compiler complained about mixing declarations and code. > > The rest of the patch looks fine, though I haven't studied the HT spec > deeply. The +2 on maxbp is familiar. > > /Tomas > _______________________________________________ > 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 Sat, Aug 12, 2023 at 01:31:16PM -0700, pal@sandflow.com wrote: > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > Intended to replace https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230802000135.26482-3-michael@niedermayer.cc/ > with a more accurate block decoding magnitude bound. > > --- > libavcodec/jpeg2000htdec.c | 34 +++++++++++++++++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) will apply it seems this was forgotten somehow thx [...]
diff --git a/libavcodec/jpeg2000htdec.c b/libavcodec/jpeg2000htdec.c index 2c4cea5dd9..a7329206de 100644 --- a/libavcodec/jpeg2000htdec.c +++ b/libavcodec/jpeg2000htdec.c @@ -572,6 +572,14 @@ static int jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, const uint8_t *vlc_buf = Dcup + Pcup; + /* + * Bound on the recision needed to process the codeblock. The number of + * decoded bit planes is equal to at most cblk->zbp + 2 since S_blk = P if + * there are no placeholder passes or HT Sets and P = cblk->zbp. See Rec. + * ITU-T T.814, 7.6. + */ + int maxbp = cblk->zbp + 2; + /* convert to raster-scan */ const uint16_t is_border_x = width % 2; const uint16_t is_border_y = height % 2; @@ -590,6 +598,12 @@ static int jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, goto free; } + /* do we have enough precision, assuming a 32-bit decoding path */ + if (maxbp >= 32) { + return AVERROR_INVALIDDATA; + goto free; + } + sigma = sigma_n; mu = mu_n; @@ -676,6 +690,10 @@ static int jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, } U[J2K_Q1] = kappa[J2K_Q1] + u[J2K_Q1]; U[J2K_Q2] = kappa[J2K_Q2] + u[J2K_Q2]; + if (U[J2K_Q1] > maxbp || U[J2K_Q2] > maxbp) { + ret = AVERROR_INVALIDDATA; + goto free; + } for (int i = 0; i < 4; i++) { m[J2K_Q1][i] = sigma_n[4 * q1 + i] * U[J2K_Q1] - ((emb_pat_k[J2K_Q1] >> i) & 1); @@ -713,6 +731,10 @@ static int jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, } U[J2K_Q1] = kappa[J2K_Q1] + u[J2K_Q1]; + if (U[J2K_Q1] > maxbp) { + ret = AVERROR_INVALIDDATA; + goto free; + } for (int i = 0; i < 4; i++) m[J2K_Q1][i] = sigma_n[4 * q1 + i] * U[J2K_Q1] - ((emb_pat_k[J2K_Q1] >> i) & 1); @@ -842,6 +864,10 @@ static int jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, U[J2K_Q1] = kappa[J2K_Q1] + u[J2K_Q1]; U[J2K_Q2] = kappa[J2K_Q2] + u[J2K_Q2]; + if (U[J2K_Q1] > maxbp || U[J2K_Q2] > maxbp) { + ret = AVERROR_INVALIDDATA; + goto free; + } for (int i = 0; i < 4; i++) { m[J2K_Q1][i] = sigma_n[4 * q1 + i] * U[J2K_Q1] - ((emb_pat_k[J2K_Q1] >> i) & 1); @@ -910,6 +936,10 @@ static int jpeg2000_decode_ht_cleanup_segment(const Jpeg2000DecoderContext *s, kappa[J2K_Q1] = FFMAX(1, gamma[J2K_Q1] * (max_e[J2K_Q1] - 1)); U[J2K_Q1] = kappa[J2K_Q1] + u[J2K_Q1]; + if (U[J2K_Q1] > maxbp) { + ret = AVERROR_INVALIDDATA; + goto free; + } for (int i = 0; i < 4; i++) m[J2K_Q1][i] = sigma_n[4 * q1 + i] * U[J2K_Q1] - ((emb_pat_k[J2K_Q1] >> i) & 1); @@ -1238,8 +1268,10 @@ ff_jpeg2000_decode_htj2k(const Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c } if ((ret = jpeg2000_decode_ht_cleanup_segment(s, cblk, t1, &mel_state, &mel, &vlc, &mag_sgn, Dcup, Lcup, Pcup, pLSB, width, - height, sample_buf, block_states)) < 0) + height, sample_buf, block_states)) < 0) { + av_log(s->avctx, AV_LOG_ERROR, "Bad HT cleanup segment\n"); goto free; + } if (cblk->npasses > 1) jpeg2000_decode_sigprop_segment(cblk, width, height, Dref, Lref,
From: Pierre-Anthony Lemieux <pal@palemieux.com> Intended to replace https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230802000135.26482-3-michael@niedermayer.cc/ with a more accurate block decoding magnitude bound. --- libavcodec/jpeg2000htdec.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)