From d147be83e9a9f904918ba7f834b0fb28e09de9b5 Mon Sep 17 00:00:00 2001 From: DRC Date: Thu, 15 Apr 2021 23:31:51 -0500 Subject: [PATCH] Huff decs: Fix/suppress more innocuous UBSan errs - UBSan complained that entropy->restarts_to_go was underflowing an unsigned integer when it was decremented while cinfo->restart_interval == 0. That was, of course, completely innocuous behavior, since the result of the underflowing computation was never used. - d3a3a73f64041c6a6905faf6f9f9832e735fd880 and 7bc9fca4309563d66b0c5665a616285d0e9baeb4 silenced a UBSan signed integer overflow error, but unfortunately other malformed JPEG images have been discovered that cause unsigned integer overflow in the same computation. Since, to the best of our understanding, this behavior is innocuous, this commit reverts the commits listed above, suppresses the UBSan errors, and adds code comments to document the issue. --- jdhuff.c | 23 +++++++++++++++++------ jdphuff.c | 14 +++++++++----- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/jdhuff.c b/jdhuff.c index b5665d56..f786c105 100644 --- a/jdhuff.c +++ b/jdhuff.c @@ -540,6 +540,12 @@ process_restart(j_decompress_ptr cinfo) } +#if defined(__has_feature) +#if __has_feature(undefined_behavior_sanitizer) +__attribute__((no_sanitize("signed-integer-overflow"), + no_sanitize("unsigned-integer-overflow"))) +#endif +#endif LOCAL(boolean) decode_mcu_slow(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) { @@ -572,11 +578,15 @@ decode_mcu_slow(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) if (entropy->dc_needed[blkn]) { /* Convert DC difference to actual value, update last_dc_val */ int ci = cinfo->MCU_membership[blkn]; - /* This is really just - * s += state.last_dc_val[ci]; - * It is written this way in order to shut up UBSan. + /* Certain malformed JPEG images produce repeated DC coefficient + * differences of 2047 or -2047, which causes state.last_dc_val[ci] to + * grow until it overflows or underflows a 32-bit signed integer. This + * behavior is, to the best of our understanding, innocuous, and it is + * unclear how to work around it without potentially affecting + * performance. Thus, we (hopefully temporarily) suppress UBSan integer + * overflow errors for this function. */ - s = (int)((unsigned int)s + (unsigned int)state.last_dc_val[ci]); + s += state.last_dc_val[ci]; state.last_dc_val[ci] = s; if (block) { /* Output the DC coefficient (assumes jpeg_natural_order[0] = 0) */ @@ -671,7 +681,7 @@ decode_mcu_fast(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) if (entropy->dc_needed[blkn]) { int ci = cinfo->MCU_membership[blkn]; - s = (int)((unsigned int)s + (unsigned int)state.last_dc_val[ci]); + s += state.last_dc_val[ci]; state.last_dc_val[ci] = s; if (block) (*block)[0] = (JCOEF)s; @@ -778,7 +788,8 @@ use_slow: } /* Account for restart interval (no-op if not using restarts) */ - entropy->restarts_to_go--; + if (cinfo->restart_interval) + entropy->restarts_to_go--; return TRUE; } diff --git a/jdphuff.c b/jdphuff.c index 0e981f2f..c6d82ca1 100644 --- a/jdphuff.c +++ b/jdphuff.c @@ -4,7 +4,7 @@ * This file was part of the Independent JPEG Group's software: * Copyright (C) 1995-1997, Thomas G. Lane. * libjpeg-turbo Modifications: - * Copyright (C) 2015-2016, 2018-2020, D. R. Commander. + * Copyright (C) 2015-2016, 2018-2021, D. R. Commander. * For conditions of distribution and use, see the accompanying README.ijg * file. * @@ -348,7 +348,8 @@ decode_mcu_DC_first(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) } /* Account for restart interval (no-op if not using restarts) */ - entropy->restarts_to_go--; + if (cinfo->restart_interval) + entropy->restarts_to_go--; return TRUE; } @@ -432,7 +433,8 @@ decode_mcu_AC_first(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) } /* Account for restart interval (no-op if not using restarts) */ - entropy->restarts_to_go--; + if (cinfo->restart_interval) + entropy->restarts_to_go--; return TRUE; } @@ -483,7 +485,8 @@ decode_mcu_DC_refine(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) BITREAD_SAVE_STATE(cinfo, entropy->bitstate); /* Account for restart interval (no-op if not using restarts) */ - entropy->restarts_to_go--; + if (cinfo->restart_interval) + entropy->restarts_to_go--; return TRUE; } @@ -626,7 +629,8 @@ decode_mcu_AC_refine(j_decompress_ptr cinfo, JBLOCKROW *MCU_data) } /* Account for restart interval (no-op if not using restarts) */ - entropy->restarts_to_go--; + if (cinfo->restart_interval) + entropy->restarts_to_go--; return TRUE;