These are chat archives for HdrHistogram/HdrHistogram

10th
Oct 2015
Michael Barker
@mikeb01
Oct 10 2015 00:24
Thanks, fixed.
Alec
@ahothan
Oct 10 2015 02:29

one more to fix ;-)
this one is pretty nasty one, it crashed our wrk2 process randomly. Luckily the crash could be reproduced very easily.
To reproduce, simply try to encode an empty histogram:
--- a/test/hdr_histogram_log_test.c
+++ b/test/hdr_histogram_log_test.c

+static char* test_string_encode_empty()
+{

  • struct hdr_histogram histogram, hdr_new = NULL;
  • hdr_alloc(INT64_C(3600) 1000 1000, 3, &histogram);
    +
  • char *data;
    +
  • mu_assert("Failed to encode histogram data", hdr_log_encode(histogram, &data) == 0);
  • mu_assert("Failed to decode histogram data", hdr_log_decode(&hdr_new, data, strlen(data)) == 0);
  • mu_assert("Histograms should be the same", compare_histogram(histogram, hdr_new));
  • mu_assert("Mean different after encode/decode", compare_double(hdr_mean(histogram), hdr_mean(hdr_new), 0.001));
  • return 0;
    +}

if you run it will crash almost every time.
It turns out this bug is corrupting memory at every encode but does not always manifest as a crash. Found the problem thanks to valgrind:

--- a/src/hdr_histogram_log.c
+++ b/src/hdr_histogram_log.c
@@ -232,7 +232,7 @@ int hdr_encode_compressed(
int32_t len_to_max = counts_index_for(h, h->max_value) + 1;
int32_t counts_limit = len_to_max < h->counts_len ? len_to_max : h->counts_len;

  • if ((encoded = (_encoding_flyweight_v1) calloc(MAX_BYTES_LEB128 (size_t) counts_limit, sizeof(uint8_t))) == NULL)
  • if ((encoded = (_encoding_flyweight_v1) calloc(sizeof(_encoding_flyweight_v1) + MAX_BYTES_LEB128 (size_t) counts_limit, sizeof(uint8_t))) == NULL)
    {
    FAIL_AND_CLEANUP(cleanup, result, ENOMEM);
    }

The valgrind run was clean with the fixed version.
Thanks

actually it manifests only if the actual length of the encoded varint string turns out to be more than the allocated size - the header size, which happens all the time for an empty histogram.
Michael Barker
@mikeb01
Oct 10 2015 03:06
@ahothan Thank you for the info, would it be possible to raise an issue on Github and attach the patch there. I'm struggling to interpret the patch within gitter's weird output.
Alec
@ahothan
Oct 10 2015 03:56
Just saw that the gitter editor supports md so need to use triple back quote. The issue has been raised. I have not executed the other test programs with valgrind but if you need the leak tracebacks let me know.
Michael Barker
@mikeb01
Oct 10 2015 05:56
Cheers, fixed.