Coverity Scan errors BAD_SHIFT and FORWARD_NULL in c/dec/decode.c #360

New Issue

claunia · 2026-01-29T20:42:39Z

claunia commented

2026-01-29 20:42:39 +00:00

Originally created by @erAck on GitHub (Nov 23, 2020).

A Coverity Scan of 1.0.8 decoder sources resulted in the following errors:

Error: BAD_SHIFT:
c/dec/decode.c:1471: cond_at_least: Checking "s->distance_code <= 3" implies that "s->distance_code" is at least 4 on the false branch.
c/dec/decode.c:1479: assignment: Assigning: "base" = "s->distance_code - 10". The value of "base" is now at least -6.
c/dec/decode.c:1486: negative_shift: In expression "0x605142 >> 4 * base", shifting by a negative amount has undefined behavior.  The shift amount, "4 * base", is as little as -24.
# 1484|       }
# 1485|       /* Unpack one of six 4-bit values. */
# 1486|->     delta = ((0x605142 >> (4 * base)) & 0xF) - 3;
# 1487|       s->distance_code = s->dist_rb[(s->dist_rb_idx + index_delta) & 0x3] + delta;
# 1488|       if (s->distance_code <= 0) {

Error: FORWARD_NULL (CWE-476):
c/dec/decode.c:2219: write_zero_model: Passing "s" to "BrotliDecoderStateMetablockBegin", which sets "s->context_modes" to "NULL".
c/dec/decode.c:2445: var_deref_model: Passing "s" to "PrepareLiteralDecoding", which dereferences null "s->context_modes".
# 2443|   
# 2444|         case BROTLI_STATE_BEFORE_COMPRESSED_METABLOCK_BODY:
# 2445|->         PrepareLiteralDecoding(s);
# 2446|           s->dist_context_map_slice = s->dist_context_map;
# 2447|           s->htree_command = s->insert_copy_hgroup.htrees[0];

It is advisable to investigate and fix those.
Thanks.

Originally created by @erAck on GitHub (Nov 23, 2020). A Coverity Scan of 1.0.8 decoder sources resulted in the following errors: ``` Error: BAD_SHIFT: c/dec/decode.c:1471: cond_at_least: Checking "s->distance_code <= 3" implies that "s->distance_code" is at least 4 on the false branch. c/dec/decode.c:1479: assignment: Assigning: "base" = "s->distance_code - 10". The value of "base" is now at least -6. c/dec/decode.c:1486: negative_shift: In expression "0x605142 >> 4 * base", shifting by a negative amount has undefined behavior. The shift amount, "4 * base", is as little as -24. # 1484| } # 1485| /* Unpack one of six 4-bit values. */ # 1486|-> delta = ((0x605142 >> (4 * base)) & 0xF) - 3; # 1487| s->distance_code = s->dist_rb[(s->dist_rb_idx + index_delta) & 0x3] + delta; # 1488| if (s->distance_code <= 0) { Error: FORWARD_NULL (CWE-476): c/dec/decode.c:2219: write_zero_model: Passing "s" to "BrotliDecoderStateMetablockBegin", which sets "s->context_modes" to "NULL". c/dec/decode.c:2445: var_deref_model: Passing "s" to "PrepareLiteralDecoding", which dereferences null "s->context_modes". # 2443| # 2444| case BROTLI_STATE_BEFORE_COMPRESSED_METABLOCK_BODY: # 2445|-> PrepareLiteralDecoding(s); # 2446| s->dist_context_map_slice = s->dist_context_map; # 2447| s->htree_command = s->insert_copy_hgroup.htrees[0]; ``` It is advisable to investigate and fix those. Thanks.

claunia closed this issue

2026-01-29 20:42:39 +00:00

claunia commented

2026-01-29 20:42:40 +00:00

@eustas commented on GitHub (Jan 18, 2021):

False positive:

int offset = s->distance_code - 3;
  if (s->distance_code <= 3) {
    /* Compensate double distance-ring-buffer roll for dictionary items. */
    s->distance_context = 1 >> s->distance_code;
    s->distance_code = s->dist_rb[(s->dist_rb_idx - offset) & 3];
    s->dist_rb_idx -= s->distance_context;
  } else {
    int index_delta = 3;
    int delta;
    int base = s->distance_code - 10;
    if (s->distance_code < 10) {
      base = s->distance_code - 4;
    } else {
      index_delta = 2;
    }
    /* Unpack one of six 4-bit values. */
    delta = ((0x605142 >> (4 * base)) & 0xF) - 3;

It is obvious that 4 <= s->distance_code < 10 activates base = s->distance_code - 4 branch.

Less obvious false positive. BROTLI_STATE_BEFORE_COMPRESSED_METABLOCK_BODY phase is always preceded by BROTLI_STATE_METABLOCK_HEADER_2.

We, do regular Coverity Scan checks... and it produces a lot of false positives...

@eustas commented on GitHub (Jan 18, 2021): 1) False positive: ``` int offset = s->distance_code - 3; if (s->distance_code <= 3) { /* Compensate double distance-ring-buffer roll for dictionary items. */ s->distance_context = 1 >> s->distance_code; s->distance_code = s->dist_rb[(s->dist_rb_idx - offset) & 3]; s->dist_rb_idx -= s->distance_context; } else { int index_delta = 3; int delta; int base = s->distance_code - 10; if (s->distance_code < 10) { base = s->distance_code - 4; } else { index_delta = 2; } /* Unpack one of six 4-bit values. */ delta = ((0x605142 >> (4 * base)) & 0xF) - 3; ``` It is obvious that `4 <= s->distance_code < 10` activates `base = s->distance_code - 4` branch. 2) Less obvious false positive. `BROTLI_STATE_BEFORE_COMPRESSED_METABLOCK_BODY ` phase is always preceded by `BROTLI_STATE_METABLOCK_HEADER_2`. We, do regular Coverity Scan checks... and it produces a lot of false positives...

claunia referenced this issue

2026-01-29 20:51:49 +00:00

[PR #360] [MERGED] Step 1: extract common #954

claunia referenced this issue

2026-01-29 20:51:50 +00:00

[PR #360] Step 1: extract common #958

claunia referenced this issue

2026-01-29 20:51:55 +00:00

[PR #375] [MERGED] Update to v0.5 #975

Sign in to join this conversation.