opj_t1_ht_decode_cblk(): avoid out-of-bounds read on ds0_ht_02_b11.j2k (

#2)

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

@@ -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,18 @@ 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) */