From 9715fc6750816af1869d3d8140cd38bc8433446d Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 23 Sep 2021 15:27:07 +0200 Subject: [PATCH] opj_t1_ht_decode_cblk(): avoid out-of-bounds read on ds0_ht_02_b11.j2k Avoids the following issue: $ valgrind bin/opj_decompress -i ~/OpenHTJ2K/conformance_data/ds0_ht_02_b11.j2k -o out.ppm -threads 0 ==4037690== Invalid read of size 1 ==4037690== at 0x48589FA: opj_t1_ht_decode_cblk (fbc_dec.c:1262) ==4037690== by 0x48B28E5: opj_t1_clbl_decode_processor (t1.c:1690) ==4037690== by 0x4854A33: opj_thread_pool_submit_job (thread.c:835) ==4037690== by 0x48B37C3: opj_t1_decode_cblks (t1.c:1943) ==4037690== by 0x48BD668: opj_tcd_t1_decode (tcd.c:2000) ==4037690== by 0x48BCADF: opj_tcd_decode_tile (tcd.c:1654) ==4037690== by 0x487D348: opj_j2k_decode_tile (j2k.c:9759) ==4037690== by 0x4881CDA: opj_j2k_decode_tiles (j2k.c:11566) ==4037690== by 0x487B333: opj_j2k_exec (j2k.c:8903) ==4037690== by 0x4882AD1: opj_j2k_decode (j2k.c:11912) ==4037690== by 0x488EF5C: opj_decode (openjpeg.c:494) ==4037690== by 0x1103AC: main (opj_decompress.c:1547) ==4037690== Address 0x52884ef is 1 bytes before a block of size 2 alloc'd ==4037690== at 0x483B723: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==4037690== by 0x483E017: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==4037690== by 0x48C0676: opj_realloc (opj_malloc.c:244) ==4037690== by 0x48584E6: opj_t1_ht_decode_cblk (fbc_dec.c:1123) ==4037690== by 0x48B28E5: opj_t1_clbl_decode_processor (t1.c:1690) ==4037690== by 0x4854A33: opj_thread_pool_submit_job (thread.c:835) ==4037690== by 0x48B37C3: opj_t1_decode_cblks (t1.c:1943) ==4037690== by 0x48BD668: opj_tcd_t1_decode (tcd.c:2000) ==4037690== by 0x48BCADF: opj_tcd_decode_tile (tcd.c:1654) ==4037690== by 0x487D348: opj_j2k_decode_tile (j2k.c:9759) ==4037690== by 0x4881CDA: opj_j2k_decode_tiles (j2k.c:11566) ==4037690== by 0x487B333: opj_j2k_exec (j2k.c:8903) I've also simplified a bit the allocation of the concatenated code block buffer, to remove the OPJ_COMMON_CBLK_DATA_EXTRA that I believe is a trick only needed for regular code block decoding, not HT. --- src/lib/openjp2/fbc_dec.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/lib/openjp2/fbc_dec.c b/src/lib/openjp2/fbc_dec.c index 71e3bb868..d84c3af67 100644 --- a/src/lib/openjp2/fbc_dec.c +++ b/src/lib/openjp2/fbc_dec.c @@ -1111,6 +1111,7 @@ OPJ_BOOL opj_t1_ht_decode_cblk(opj_t1_t *t1, OPJ_UINT32* sp; OPJ_INT32 x, y; // loop indices OPJ_BOOL stripe_causal = (cblksty & J2K_CCP_CBLKSTY_VSC) != 0; + OPJ_UINT32 cblk_len = 0; (void)(orient); // stops unused parameter message (void)(check_pterm); // stops unused parameter message @@ -1143,29 +1144,27 @@ OPJ_BOOL opj_t1_ht_decode_cblk(opj_t1_t *t1, /* numbps = Mb + 1 - zero_bplanes, Mb = Kmax, zero_bplanes = missing_msbs */ zero_bplanes = (cblk->Mb + 1) - cblk->numbps; - /* Even if we have a single chunk, in multi-threaded decoding */ - /* the insertion of our synthetic marker might potentially override */ - /* valid codestream of other codeblocks decoded in parallel. */ - if (cblk->numchunks > 1 || t1->mustuse_cblkdatabuffer) { + /* Compute whole codeblock length from chunk lengths */ + cblk_len = 0; + { OPJ_UINT32 i; - OPJ_UINT32 cblk_len; - - /* Compute whole codeblock length from chunk lengths */ - cblk_len = 0; for (i = 0; i < cblk->numchunks; i++) { cblk_len += cblk->chunks[i].len; } + } + + if (cblk->numchunks > 1 || t1->mustuse_cblkdatabuffer) { + OPJ_UINT32 i; /* Allocate temporary memory if needed */ - if (cblk_len + OPJ_COMMON_CBLK_DATA_EXTRA > t1->cblkdatabuffersize) { + if (cblk_len > t1->cblkdatabuffersize) { cblkdata = (OPJ_BYTE*)opj_realloc( - t1->cblkdatabuffer, cblk_len + OPJ_COMMON_CBLK_DATA_EXTRA); + t1->cblkdatabuffer, cblk_len); if (cblkdata == NULL) { return OPJ_FALSE; } t1->cblkdatabuffer = cblkdata; - memset(t1->cblkdatabuffer + cblk_len, 0, OPJ_COMMON_CBLK_DATA_EXTRA); - t1->cblkdatabuffersize = cblk_len + OPJ_COMMON_CBLK_DATA_EXTRA; + t1->cblkdatabuffersize = cblk_len; } /* Concatenate all chunks */ @@ -1327,6 +1326,19 @@ OPJ_BOOL opj_t1_ht_decode_cblk(opj_t1_t *t1, // read scup and fix the bytes there lcup = (int)lengths1; // length of CUP //scup is the length of MEL + VLC + if (lcup < 2 || (OPJ_UINT32)lcup - 2 >= cblk_len) + { + if (p_manager_mutex) { + opj_mutex_lock(p_manager_mutex); + } + opj_event_msg(p_manager, EVT_ERROR, "Malformed HT codeblock. " + "Invalid lcup value\n"); + + if (p_manager_mutex) { + opj_mutex_unlock(p_manager_mutex); + } + return OPJ_FALSE; + } scup = (((int)coded_data[lcup - 1]) << 4) + (coded_data[lcup - 2] & 0xF); if (scup < 2 || scup > lcup || scup > 4079) { //something is wrong /* The standard stipulates 2 <= Scup <= min(Lcup, 4079) */