Fix unsigned int overflow reported by UBSan (#761)

* Fix unsigned int overflow reported by UBSan

Please add -DOPJ_UBSAN_BUILD to CFLAGS when building with
-fsanitize=undefined,unsigned-integer-overflow

It seems clang/gcc do not allow to disable checking for block of code
other than function or file.

diff --git a/src/lib/openjp2/bio.c b/src/lib/openjp2/bio.c
index e4edb3724e44e3753ed7e184f1e39ead93c60eca..269769b45f5a768792d6b5eda3a86ec920b666af 100644 (file)
--- a/src/lib/openjp2/bio.c
+++ b/src/lib/openjp2/bio.c
@@ -151,19 +151,31 @@ void opj_bio_init_dec(opj_bio_t *bio, OPJ_BYTE *bp, OPJ_UINT32 len) {
        bio->ct = 0;
 }
 
+OPJ_NOSANITIZE("unsigned-integer-overflow")
 void opj_bio_write(opj_bio_t *bio, OPJ_UINT32 v, OPJ_UINT32 n) {
        OPJ_UINT32 i;
-       for (i = n - 1; i < n; i--) {
+       
+       assert((n > 0U) && (n <= 32U));
+       for (i = n - 1; i < n; i--) { /* overflow used for end-loop condition */
                opj_bio_putbit(bio, (v >> i) & 1);
        }
 }
 
+OPJ_NOSANITIZE("unsigned-integer-overflow")
 OPJ_UINT32 opj_bio_read(opj_bio_t *bio, OPJ_UINT32 n) {
        OPJ_UINT32 i;
-    OPJ_UINT32 v;
-       v = 0;
-       for (i = n - 1; i < n; i--) {
-               v += opj_bio_getbit(bio) << i;
+       OPJ_UINT32 v;
+       
+       assert((n > 0U) /* && (n <= 32U)*/);
+#ifdef OPJ_UBSAN_BUILD
+       /* This assert fails for some corrupted images which are gracefully rejected */
+       /* Add this assert only for ubsan build. */
+       /* This is the condition for overflow not to occur below which is needed because of OPJ_NOSANITIZE */
+       assert(n <= 32U);
+#endif
+       v = 0U;
+       for (i = n - 1; i < n; i--) { /* overflow used for end-loop condition */
+               v |= opj_bio_getbit(bio) << i; /* can't overflow, opj_bio_getbit returns 0 or 1 */
        }
        return v;
 }

diff --git a/src/lib/openjp2/j2k.c b/src/lib/openjp2/j2k.c
index be8e94423a4f9e1b46149fed09fba50dac6c3106..525f629a4cf7bedb5d1855b3a44252a913ebcf49 100644 (file)
--- a/src/lib/openjp2/j2k.c
+++ b/src/lib/openjp2/j2k.c
@@ -2063,17 +2063,17 @@ static OPJ_BOOL opj_j2k_read_siz(opj_j2k_t *p_j2k,
         /* testcase 4035.pdf.SIGSEGV.d8b.3375 */
         /* testcase issue427-null-image-size.jp2 */
         if ((l_image->x0 >= l_image->x1) || (l_image->y0 >= l_image->y1)) {
-                opj_event_msg(p_manager, EVT_ERROR, "Error with SIZ marker: negative or zero image size (%d x %d)\n", l_image->x1 - l_image->x0, l_image->y1 - l_image->y0);
+                opj_event_msg(p_manager, EVT_ERROR, "Error with SIZ marker: negative or zero image size (%" PRId64 " x %" PRId64 ")\n", (OPJ_INT64)l_image->x1 - l_image->x0, (OPJ_INT64)l_image->y1 - l_image->y0);
                 return OPJ_FALSE;
         }
         /* testcase 2539.pdf.SIGFPE.706.1712 (also 3622.pdf.SIGFPE.706.2916 and 4008.pdf.SIGFPE.706.3345 and maybe more) */
-        if (!(l_cp->tdx * l_cp->tdy)) {
+        if ((l_cp->tdx == 0U) || (l_cp->tdy == 0U)) {
                 opj_event_msg(p_manager, EVT_ERROR, "Error with SIZ marker: invalid tile size (tdx: %d, tdy: %d)\n", l_cp->tdx, l_cp->tdy);
                 return OPJ_FALSE;
         }
 
         /* testcase 1610.pdf.SIGSEGV.59c.681 */
-        if (((OPJ_UINT64)l_image->x1) * ((OPJ_UINT64)l_image->y1) != (l_image->x1 * l_image->y1)) {
+        if ((0xFFFFFFFFU / l_image->x1) < l_image->y1) {
                 opj_event_msg(p_manager, EVT_ERROR, "Prevent buffer overflow (x1: %d, y1: %d)\n", l_image->x1, l_image->y1);
                 return OPJ_FALSE;
         }

diff --git a/src/lib/openjp2/opj_includes.h b/src/lib/openjp2/opj_includes.h
index 5add09188e958419af326391d79508e6366f6ddc..58a5a9a9ac070eca5c6d4ca901b20718ad9fc0d7 100644 (file)
--- a/src/lib/openjp2/opj_includes.h
+++ b/src/lib/openjp2/opj_includes.h
@@ -112,6 +112,14 @@
        #endif
 #endif
 
+#ifdef __has_attribute
+       #if __has_attribute(no_sanitize)
+               #define OPJ_NOSANITIZE(kind) __attribute__((no_sanitize(kind)))
+       #endif
+#endif
+#ifndef OPJ_NOSANITIZE
+       #define OPJ_NOSANITIZE(kind)
+#endif
 
 
 /* MSVC before 2013 and Borland C do not have lrintf */

diff --git a/src/lib/openjp2/pi.c b/src/lib/openjp2/pi.c
index bfee10a2f47fd1e11162c02ed11109d3444809fc..cffad6684f0fc1f7521461c15c5135c19fb57bc6 100644 (file)
--- a/src/lib/openjp2/pi.c
+++ b/src/lib/openjp2/pi.c
@@ -747,10 +747,12 @@ static void opj_get_all_encoding_parameters(   const opj_image_t *p_image,
                }
 
                /* use custom size for precincts*/
-               l_level_no = l_tccp->numresolutions - 1;
+               l_level_no = l_tccp->numresolutions;
                for (resno = 0; resno < l_tccp->numresolutions; ++resno) {
                        OPJ_UINT32 l_dx, l_dy;
 
+                       --l_level_no;
+                       
                        /* precinct width and height*/
                        l_pdx = l_tccp->prcw[resno];
                        l_pdy = l_tccp->prch[resno];
@@ -782,7 +784,6 @@ static void opj_get_all_encoding_parameters(   const opj_image_t *p_image,
                                *p_max_prec = l_product;
                        }
 
-                       --l_level_no;
                }
                ++l_tccp;
                ++l_img_comp;