Issues uncovered by Coverity Scan #17

New Issue

claunia · 2026-01-29T20:28:58Z

claunia commented

2026-01-29 20:28:58 +00:00

Originally created by @nemequ on GitHub (Aug 28, 2015).

This is with revision d811b186c5. I can't really reproduce the full report here, but here is a summary of each item it found.

enc/histogram.h:37: Non-static class member bit_cost_ is not initialized in this constructor nor in any functions that it calls.
enc/command.h:106: Non-static class members insert_len_, copy_len_, cmd_prefix_, dist_prefix_, cmd_extra_, and dist_extra_ are not initialized in this constructor nor in any functions that it calls.
enc/brotli_bit_stream.cc:343: index is passed to a parameter that cannot be negative. IndexOf can return -1
enc/literal_cost.cc:102: Execution cannot reach the expression 0 inside this statement: last_c = ((i + window_half …
- at_least: At condition i + window_half - 2 < 0, the value of i must be at least 0.
- const: At condition i + window_half - 2 < 0, the value of window_half must be equal to 495.
- dead_error_condition: The condition i + window_half - 2 < 0 cannot be true.
enc/prefix.h:75: In expression distance_code >> bucket, shifting by a negative amount has undefined behavior. The shift amount, bucket, is -2.
- return_constant: Function call brotli::Log2Floor(distance_code) returns -1.
- assignment: Assigning: bucket = brotli::Log2Floor(distance_code) - 1. The value of bucket is now -2.
enc/entropy_encode.cc:45: Non-static class members total_count_, index_left_, and index_right_or_value_ are not initialized in this constructor nor in any functions that it calls.
enc/backward_references.cc:157: The compiler-generated constructor for this class does not initialize min_cost_cmd_
enc/encode.cc:207: Non-static class member literal_cost_mask_ is not initialized in this constructor nor in any functions that it calls.
dec/huffman.c:142: Using uninitialized value sorted[symbol++]
dec/decode.c:871: Using uninitialized value s.loop_counter when calling BrotliDecompressStreaming
enc/metablock.cc:496: Using tainted variable context as an index to pointer static_context_map
enc/static_dict.cc:391: data[0] == '\xc2' is always false regardless of the values of its operands. This occurs as the logical operand of if.

If you need more details about an item let me know. I only provided the full information on the one item because it is a bit difficult to follow without the it, but I think the others should be pretty easy to figure out.

Originally created by @nemequ on GitHub (Aug 28, 2015). This is with revision d811b186c5037b434d56ddb831ceccdf5a954687. I can't really reproduce the full report here, but here is a summary of each item it found. - `enc/histogram.h:37`: Non-static class member `bit_cost_` is not initialized in this constructor nor in any functions that it calls. - `enc/command.h:106`: Non-static class members `insert_len_`, `copy_len_`, `cmd_prefix_`, `dist_prefix_`, `cmd_extra_`, and `dist_extra_` are not initialized in this constructor nor in any functions that it calls. - `enc/brotli_bit_stream.cc:343`: `index` is passed to a parameter that cannot be negative. _IndexOf can return -1_ - `enc/literal_cost.cc:102`: Execution cannot reach the expression 0 inside this statement: `last_c = ((i + window_half …` - at_least: At condition `i + window_half - 2 < 0`, the value of `i` must be at least 0. - const: At condition `i + window_half - 2 < 0`, the value of `window_half` must be equal to 495. - dead_error_condition: The condition `i + window_half - 2 < 0` cannot be true. - `enc/prefix.h:75`: In expression `distance_code >> bucket`, shifting by a negative amount has undefined behavior. The shift amount, `bucket`, is -2. - return_constant: Function call `brotli::Log2Floor(distance_code)` returns -1. - assignment: Assigning: `bucket = brotli::Log2Floor(distance_code) - 1`. The value of `bucket` is now -2. - `enc/entropy_encode.cc:45`: Non-static class members `total_count_`, `index_left_`, and `index_right_or_value_` are not initialized in this constructor nor in any functions that it calls. - `enc/backward_references.cc:157`: The compiler-generated constructor for this class does not initialize `min_cost_cmd_` - `enc/encode.cc:207`: Non-static class member `literal_cost_mask_` is not initialized in this constructor nor in any functions that it calls. - `dec/huffman.c:142`: Using uninitialized value `sorted[symbol++]` ![dec/huffman.c:142](https://cloud.githubusercontent.com/assets/1151/9540204/a5da6576-4d10-11e5-9b0d-70f8dc1f1438.png) - `dec/decode.c:871`: Using uninitialized value `s.loop_counter` when calling `BrotliDecompressStreaming` - `enc/metablock.cc:496`: Using tainted variable context as an index to pointer `static_context_map` `enc/static_dict.cc:391`: `data[0] == '\xc2'` is always false regardless of the values of its operands. This occurs as the logical operand of if. If you need more details about an item let me know. I only provided the full information on the one item because it is a bit difficult to follow without the it, but I think the others should be pretty easy to figure out.

claunia closed this issue

2026-01-29 20:28:59 +00:00

claunia commented

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

@nemequ commented on GitHub (Sep 6, 2015):

FWIW I would be willing to add someone to Squash's project on Coverity Scan so you can see the results in their interface, though I would suggest just creating a project for brotli. It has actually uncovered quite a few real issues in other compression libraries (as well as Squash itself) with surprisingly few false positives—definitely worth it, IMHO.

@nemequ commented on GitHub (Sep 6, 2015): FWIW I would be willing to add someone to Squash's project on [Coverity Scan](https://scan.coverity.com/) so you can see the results in their interface, though I would suggest just creating a project for brotli. It has actually uncovered quite a few real issues in other compression libraries (as well as Squash itself) with surprisingly few false positives—definitely worth it, IMHO.

claunia commented

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

@eustas commented on GitHub (Oct 6, 2015):

All Coverity Scan reports have been addressed =)

See https://scan.coverity.com/projects/google-brotli

Thank you for reporting about these issues.

@eustas commented on GitHub (Oct 6, 2015): All Coverity Scan reports have been addressed =) See https://scan.coverity.com/projects/google-brotli Thank you for reporting about these issues.

claunia commented

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

@nemequ commented on GitHub (Jul 12, 2016):

I think it's time for another run; Coverity is reporting 9 high-impact issues and 4 medium-impact issues in Brotli.

@nemequ commented on GitHub (Jul 12, 2016): I think it's time for another run; Coverity is [reporting](https://scan.coverity.com/projects/squash) 9 high-impact issues and 4 medium-impact issues in Brotli.

claunia commented

2026-01-29 20:29:02 +00:00

@eustas commented on GitHub (Jul 12, 2016):

Uninitialized variables and null-dereference are all false-positives. Usage of tainted values is natural -> almost false positive. Going to toss coda a little bit to convince Coverity Scan.

@eustas commented on GitHub (Jul 12, 2016): Uninitialized variables and null-dereference are all false-positives. Usage of tainted values is natural -> almost false positive. Going to toss coda a little bit to convince Coverity Scan.

claunia commented

2026-01-29 20:29:03 +00:00

@nemequ commented on GitHub (Jul 12, 2016):

FWIW, cppcheck picks up a couple minor issues, too:

[brotli/enc/block_splitter_inc.h:280] -> [brotli/enc/block_splitter_inc.h:281]: (style) Variable 'pairs' is reassigned a value before the old one has been used.
[brotli/enc/write_bits.h:80]: (warning) %d in format string (no. 1) requires 'int' but the argument type is 'size_t {aka unsigned long}'.

To fix the second one, you could cast to int, or if you want to really use size_t you can do something like this (stolen from https://github.com/nemequ/munit/blob/master/munit.h#L56):

#if !defined(_WIN32)
  #define BROTLI_SIZE_SPECIFIER "zu"
#elif defined(_M_X64) || defined(__amd64__)
  #define BROTLI_SIZE_SPECIFIER "I64u"
#else
  #define BROTLI_SIZE_SPECIFIER "u"
#endif

Windows make everything harder.

@nemequ commented on GitHub (Jul 12, 2016): FWIW, cppcheck picks up a couple minor issues, too: ``` [brotli/enc/block_splitter_inc.h:280] -> [brotli/enc/block_splitter_inc.h:281]: (style) Variable 'pairs' is reassigned a value before the old one has been used. [brotli/enc/write_bits.h:80]: (warning) %d in format string (no. 1) requires 'int' but the argument type is 'size_t {aka unsigned long}'. ``` To fix the second one, you could cast to int, or if you want to really use size_t you can do something like this (stolen from https://github.com/nemequ/munit/blob/master/munit.h#L56): ``` c #if !defined(_WIN32) #define BROTLI_SIZE_SPECIFIER "zu" #elif defined(_M_X64) || defined(__amd64__) #define BROTLI_SIZE_SPECIFIER "I64u" #else #define BROTLI_SIZE_SPECIFIER "u" #endif ``` Windows make everything harder.

claunia commented

2026-01-29 20:29:06 +00:00

@nemequ commented on GitHub (Jul 13, 2016):

@eustas, I see the only issue remaining is a null-pointer dereference; IIRC that can be "fixed" by adding an assert(foo != NULL). If it's a function parameter, another possibility might be to add a nonnnull attribute… Looks like it's available in GCC 3.3+.

@nemequ commented on GitHub (Jul 13, 2016): @eustas, I see the only issue remaining is a null-pointer dereference; IIRC that can be "fixed" by adding an `assert(foo != NULL)`. If it's a function parameter, another possibility might be to add a [nonnnull attribute](https://gcc.gnu.org/onlinedocs/gcc-6.1.0/gcc/Common-Function-Attributes.html#index-g_t_0040code_007bnonnull_007d-function-attribute-3238)… Looks like it's [available in GCC 3.3+](https://nemequ.github.io/hedley/api-reference.html#HEDLEY_NON_NULL).

claunia referenced this issue

2026-01-29 20:46:20 +00:00

[PR #17] Add Python bindings #596

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/brotli#17