These are chat archives for HdrHistogram/HdrHistogram
@mikeb01 : had a look at your latest commit and I think it still needs some adjustment in the decoding path.
the code first inflates the header (line 613)
then extracts the payload length from it:
int32_t counts_limit = be32toh(encoding_flyweight.payload_len);
then it allocates the space for inflating the varint string with 9 bytes extra padding:
if ((counts_array = calloc(1, (size_t) counts_limit + 9)) == NULL)
then inflates the the varint string up to counts_array bytes:
strm.next_out = counts_array;
strm.avail_out = (uInt) counts_limit;
if (inflate(&strm, Z_FINISH) != Z_STREAM_END)
then finally decodes the varint string
int r = _apply_to_counts_zz(h, counts_array, counts_limit);
First comment is that we don't really need 9 extra zero bytes to stop an overrun because a single trailing null byte is enough to stop the decoding on a truncated varint stream.
Second with this solution of using a padding, the function would not return a decoding error on that artificial stop and instead return a truncated value (which is clearly not desired)
So I'm afraid the function needs to take the exact decoded length and check for premature varint decoding, ie the last byte in the stream must either be the 9th byte or a varint or have its more bit set to zero.
Last but not least, a malicious encoded histogram can crash the decoder. In the _apply_to_counts_zz() function, there is no check in the loop that counts_index does not run out of bounds:
h->counts[counts_index] = value;
It is very easy to send a valid varint stream that can overrun any array - and it does not even have to big a large one.
e.g. (INT32_MIN - 1) followed by (1) is enough to corrupt the memory for any array that is smaller than INT32_MAX entries.