Skip to content

Commit

Permalink
Huff decs: Fix/suppress more innocuous UBSan errs
Browse files Browse the repository at this point in the history
- 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.

- d3a3a73 and
  7bc9fca 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.
  • Loading branch information
dcommander committed Apr 16, 2021
1 parent 8fa7036 commit d147be8
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 11 deletions.
23 changes: 17 additions & 6 deletions jdhuff.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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) */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -778,7 +788,8 @@ decode_mcu(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;
}
Expand Down
14 changes: 9 additions & 5 deletions jdphuff.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;

Expand Down

0 comments on commit d147be8

Please sign in to comment.