diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2850bfc..f319c04 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,3 +69,23 @@ jobs: run: cmake --build _build --config ${{ matrix.build_type }} - name: Test run: cmake -E chdir _build ctest --build-config ${{ matrix.build_type }} + + # Run the unit suite under AddressSanitizer + UndefinedBehaviorSanitizer so + # memory-safety and integer-overflow regressions (the class the fuzzers find) + # are caught deterministically per-PR, not only in the weekly fuzzing run. + sanitizers: + name: sanitizers (linux, Debug, ASan+UBSan) + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v1 + with: + submodules: recursive + - name: Install dependencies + run: sudo apt-get update && sudo apt-get install -y zlib1g-dev + - name: Configure + run: cmake -E make_directory _build && cmake -E chdir _build cmake .. -DCMAKE_BUILD_TYPE=Debug -DHDR_LOG_REQUIRED=ON -DCMAKE_C_FLAGS="-fsanitize=address,undefined -fno-sanitize-recover=all -g" + - name: Build + run: cmake --build _build + - name: Test + run: cmake -E chdir _build ctest --output-on-failure diff --git a/.gitignore b/.gitignore index c7ae01e..f4c576a 100644 --- a/.gitignore +++ b/.gitignore @@ -54,3 +54,4 @@ debug cmake-build-debug /build-afl .vscode +_build diff --git a/src/hdr_histogram.c b/src/hdr_histogram.c index 1ec32e6..c4b4290 100644 --- a/src/hdr_histogram.c +++ b/src/hdr_histogram.c @@ -392,9 +392,14 @@ int hdr_calculate_bucket_config( int32_t sub_bucket_count_magnitude; int64_t largest_value_with_single_unit_resolution; + /* define cfg on every reject path so a two-step-init caller that mishandles + the EINVAL return never reads uninitialized fields */ + memset(cfg, 0, sizeof(*cfg)); + if (lowest_discernible_value < 1 || significant_figures < 1 || 5 < significant_figures || - lowest_discernible_value * 2 > highest_trackable_value) + /* division form: lowest*2 near INT64_MAX overflows int64 (UB) */ + lowest_discernible_value > highest_trackable_value / 2) { return EINVAL; } @@ -416,13 +421,15 @@ int hdr_calculate_bucket_config( cfg->unit_magnitude = (int32_t) unit_magnitude; cfg->sub_bucket_count = (int32_t) pow(2, (cfg->sub_bucket_half_count_magnitude + 1)); cfg->sub_bucket_half_count = cfg->sub_bucket_count / 2; - cfg->sub_bucket_mask = ((int64_t) cfg->sub_bucket_count - 1) << cfg->unit_magnitude; + /* reject before shifting: sub_bucket_mask shift past bit 61 is signed-shift UB */ if (cfg->unit_magnitude + cfg->sub_bucket_half_count_magnitude > 61) { return EINVAL; } + cfg->sub_bucket_mask = ((int64_t) cfg->sub_bucket_count - 1) << cfg->unit_magnitude; + cfg->bucket_count = buckets_needed_to_cover_value(highest_trackable_value, cfg->sub_bucket_count, (int32_t)cfg->unit_magnitude); cfg->counts_len = (cfg->bucket_count + 1) * (cfg->sub_bucket_count / 2); diff --git a/src/hdr_histogram_log.c b/src/hdr_histogram_log.c index 39fb96b..6823977 100644 --- a/src/hdr_histogram_log.c +++ b/src/hdr_histogram_log.c @@ -451,7 +451,7 @@ static int hdr_decode_compressed_v0( else { hdr_add(*histogram, h); - hdr_free(h); + hdr_close(h); /* hdr_close: counts is a separate alloc; hdr_free leaks it */ } return result; @@ -556,7 +556,7 @@ static int hdr_decode_compressed_v1( else { hdr_add(*histogram, h); - hdr_free(h); + hdr_close(h); } return result; @@ -658,7 +658,7 @@ static int hdr_decode_compressed_v2( else { hdr_add(*histogram, h); - hdr_free(h); + hdr_close(h); } return result; diff --git a/test/hdr_histogram_atomic_concurrency_test.c b/test/hdr_histogram_atomic_concurrency_test.c index 8735cd3..c869f6c 100644 --- a/test/hdr_histogram_atomic_concurrency_test.c +++ b/test/hdr_histogram_atomic_concurrency_test.c @@ -50,6 +50,7 @@ static char* test_recording_concurrently(void) struct hdr_iter expected_iter; struct hdr_iter actual_iter; pthread_t threads[2]; + char* result; int i; mu_assert("init", 0 == hdr_init(1, 10000000, 2, &expected_histogram)); @@ -82,7 +83,13 @@ static char* test_recording_concurrently(void) hdr_iter_init(&expected_iter, expected_histogram); hdr_iter_init(&actual_iter, actual_histogram); - return compare_histograms(expected_histogram, actual_histogram); + result = compare_histograms(expected_histogram, actual_histogram); + + free(values); /* plain array */ + hdr_close(expected_histogram); + hdr_close(actual_histogram); + + return result; } static struct mu_result all_tests(void) diff --git a/test/hdr_histogram_atomic_test.c b/test/hdr_histogram_atomic_test.c index 7a407bd..27c6c6f 100644 --- a/test/hdr_histogram_atomic_test.c +++ b/test/hdr_histogram_atomic_test.c @@ -45,28 +45,28 @@ static void load_histograms(void) int i; if (raw_histogram) { - free(raw_histogram); + hdr_close(raw_histogram); /* hdr_close: counts is a separate alloc; free() leaks it */ } hdr_init(1, highest_trackable_value, significant_figures, &raw_histogram); if (cor_histogram) { - free(cor_histogram); + hdr_close(cor_histogram); } hdr_init(1, highest_trackable_value, significant_figures, &cor_histogram); if (scaled_raw_histogram) { - free(scaled_raw_histogram); + hdr_close(scaled_raw_histogram); } hdr_init(1000, highest_trackable_value * 512, significant_figures, &scaled_raw_histogram); if (scaled_cor_histogram) { - free(scaled_cor_histogram); + hdr_close(scaled_cor_histogram); } hdr_init(1000, highest_trackable_value * 512, significant_figures, &scaled_cor_histogram); @@ -96,7 +96,7 @@ static char* test_create(void) mu_assert("Failed to allocate hdr_histogram", h != NULL); mu_assert("Incorrect array length", compare_int64(h->counts_len, 23552)); - free(h); + hdr_close(h); return 0; } @@ -127,6 +127,8 @@ static char* test_create_with_large_values(void) "99.0% Percentile", hdr_values_are_equivalent(h, 100000000, hdr_value_at_percentile(h, 99.0))); + hdr_close(h); + return 0; } @@ -438,6 +440,8 @@ static char* test_out_of_range_values(void) mu_assert("Should successfully record value", hdr_record_value_atomic(h, 1000)); mu_assert("Should not record value", !hdr_record_value_atomic(h, 1001)); + hdr_close(h); + return 0; } @@ -474,6 +478,8 @@ static char* test_linear_iter_buckets_correctly(void) mu_assert("Number of steps", compare_int64(4, step_count)); mu_assert("Total count", compare_int64(6, total_count)); + hdr_close(h); + return 0; } @@ -508,17 +514,20 @@ static char* test_interval_recording(void) result = compare_histograms(expected_histogram, recorder_histogram); if (result) { - return result; + goto cleanup; } recorder_corrected_histogram = hdr_interval_recorder_sample(&recorder_corrected); result = compare_histograms(expected_corrected_histogram, recorder_corrected_histogram); - if (result) - { - return result; - } - return 0; +cleanup: + /* destroy closes recorder active+inactive (incl. sampled histograms) */ + hdr_close(expected_histogram); + hdr_close(expected_corrected_histogram); + hdr_interval_recorder_destroy(&recorder); + hdr_interval_recorder_destroy(&recorder_corrected); + + return result; } static struct mu_result all_tests(void) @@ -547,6 +556,12 @@ static int hdr_histogram_run_tests(void) { struct mu_result result = all_tests(); + /* free static fixtures (hdr_close is NULL-safe) */ + hdr_close(raw_histogram); + hdr_close(cor_histogram); + hdr_close(scaled_raw_histogram); + hdr_close(scaled_cor_histogram); + if (result.message != 0) { printf("hdr_histogram_test.%s(): %s\n", result.test, result.message); diff --git a/test/hdr_histogram_log_test.c b/test/hdr_histogram_log_test.c index d21a901..c1c6f2c 100644 --- a/test/hdr_histogram_log_test.c +++ b/test/hdr_histogram_log_test.c @@ -163,8 +163,8 @@ static void load_histograms(void) { int i; - free(raw_histogram); - free(cor_histogram); + hdr_close(raw_histogram); /* hdr_close: counts is a separate alloc; free() leaks it */ + hdr_close(cor_histogram); hdr_alloc(INT64_C(3600) * 1000 * 1000, 3, &raw_histogram); hdr_alloc(INT64_C(3600) * 1000 * 1000, 3, &cor_histogram); @@ -223,7 +223,8 @@ static char* test_encode_and_decode_compressed(void) "Comparison did not match", compare_histogram(expected, actual)); - free(actual); + hdr_close(actual); + free(buffer); /* encode buffer */ return 0; } @@ -252,7 +253,8 @@ static char* test_encode_and_decode_compressed2(void) "Comparison did not match", compare_histogram(expected, actual)); - free(actual); + hdr_close(actual); + free(buffer); return 0; } @@ -276,6 +278,8 @@ static char* test_bounds_check_on_decode(void) mu_assert("Should have be invalid", compare_int64(EINVAL, rc)); mu_assert("Should not have built histogram", NULL == actual); + free(buffer); /* encode buffer; decode failed so actual is NULL */ + return 0; } @@ -303,6 +307,10 @@ static char* test_encode_and_decode_base64(void) mu_assert("Should be same", memcmp(buffer, decoded, len) == 0); + free(buffer); + free(encoded); + free(decoded); + return 0; } @@ -316,7 +324,7 @@ static char* test_encode_and_decode_empty(void) size_t encoded_len; size_t decoded_len; - free(raw_histogram); + hdr_close(raw_histogram); mu_assert("allocation should be valid", 0 == hdr_init(1, 1000000, 1, &raw_histogram)); @@ -333,6 +341,10 @@ static char* test_encode_and_decode_empty(void) mu_assert("Should be same", memcmp(buffer, decoded, len) == 0); + free(buffer); + free(encoded); + free(decoded); + return 0; } @@ -366,8 +378,9 @@ static char* test_encode_and_decode_compressed_large(void) "Comparison did not match", compare_histogram(expected, actual)); - free(expected); - free(actual); + hdr_close(expected); + hdr_close(actual); + free(buffer); return 0; } @@ -452,8 +465,11 @@ static bool assert_base64_decode(const char* base64_encoded, const char* expecte uint8_t* output = calloc(sizeof(uint8_t), output_len); int result = hdr_base64_decode(base64_encoded, encoded_len, output, output_len); + bool ok = result == 0 && compare_string(expected, (char*)output, output_len); + + free(output); - return result == 0 && compare_string(expected, (char*)output, output_len); + return ok; } static char* base64_decode_decodes_strings_without_padding(void) @@ -582,6 +598,9 @@ static char* writes_and_reads_log(void) fclose(log_file); remove(file_name); + hdr_close(read_cor_histogram); + hdr_close(read_raw_histogram); + return 0; } @@ -645,7 +664,7 @@ static char* log_reader_aggregates_into_single_histogram(void) fclose(log_file); remove(file_name); - free(histogram); + hdr_close(histogram); return 0; } @@ -689,9 +708,9 @@ static char* test_encode_decode_empty(void) 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)); - free(histogram); - free(hdr_new); - free(data); + hdr_close(histogram); + hdr_close(hdr_new); + free(data); /* encoded string */ return 0; } @@ -713,6 +732,10 @@ static char* test_string_encode_decode(void) 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)); + hdr_close(histogram); + hdr_close(hdr_new); + free(data); + return 0; } @@ -737,6 +760,10 @@ static char* test_string_encode_decode_2(void) 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)); + hdr_close(histogram); + hdr_close(hdr_new); + free(data); + return 0; } @@ -773,7 +800,7 @@ static char* decode_v1_log(void) dropped = hdr_add(accum, h); mu_assert("Dropped events", compare_int64(dropped, 0)); - free(h); + hdr_close(h); h = NULL; } @@ -784,6 +811,8 @@ static char* decode_v1_log(void) mu_assert("Seconds wrong", compare_int64(1438867590, reader.start_timestamp.tv_sec)); mu_assert("Nanoseconds wrong", compare_int64(285000000, reader.start_timestamp.tv_nsec)); + hdr_close(accum); + return 0; } @@ -820,7 +849,7 @@ static char* decode_v2_log(void) dropped = hdr_add(accum, h); mu_assert("Dropped events", compare_int64(dropped, 0)); - free(h); + hdr_close(h); h = NULL; } @@ -831,6 +860,8 @@ static char* decode_v2_log(void) mu_assert("Seconds wrong", compare_int64(1441812279, reader.start_timestamp.tv_sec)); mu_assert("Nanoseconds wrong", compare_int64(474000000, reader.start_timestamp.tv_nsec)); + hdr_close(accum); + return 0; } @@ -870,7 +901,7 @@ static char* decode_v3_log(void) dropped = hdr_add(accum, h); mu_assert("Dropped events", compare_int64(dropped, 0)); - free(h); + hdr_close(h); h = NULL; } @@ -881,6 +912,8 @@ static char* decode_v3_log(void) mu_assert("Seconds wrong", compare_int64(1441812279, reader.start_timestamp.tv_sec)); mu_assert("Nanoseconds wrong", compare_int64(474000000, reader.start_timestamp.tv_nsec)); + hdr_close(accum); + return 0; } @@ -945,7 +978,7 @@ static char* decode_v0_log(void) dropped = hdr_add(accum, h); mu_assert("Dropped events", compare_int64(dropped, 0)); - free(h); + hdr_close(h); h = NULL; } @@ -956,6 +989,8 @@ static char* decode_v0_log(void) mu_assert("Seconds wrong", compare_int64(1438869961, reader.start_timestamp.tv_sec)); mu_assert("Nanoseconds wrong", compare_int64(225000000, reader.start_timestamp.tv_nsec)); + hdr_close(accum); + return 0; } @@ -1018,8 +1053,8 @@ static struct mu_result all_tests(void) mu_run_test(test_encode_and_decode_empty); - free(raw_histogram); - free(cor_histogram); + hdr_close(raw_histogram); /* free static fixtures */ + hdr_close(cor_histogram); mu_ok; } diff --git a/test/hdr_histogram_test.c b/test/hdr_histogram_test.c index ca9c855..864e6e0 100644 --- a/test/hdr_histogram_test.c +++ b/test/hdr_histogram_test.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -45,28 +46,28 @@ static void load_histograms(void) int i; if (raw_histogram) { - free(raw_histogram); + hdr_close(raw_histogram); /* hdr_close: counts is a separate alloc; free() leaks it */ } hdr_init(1, highest_trackable_value, significant_figures, &raw_histogram); if (cor_histogram) { - free(cor_histogram); + hdr_close(cor_histogram); } hdr_init(1, highest_trackable_value, significant_figures, &cor_histogram); if (scaled_raw_histogram) { - free(scaled_raw_histogram); + hdr_close(scaled_raw_histogram); } hdr_init(1000, highest_trackable_value * 512, significant_figures, &scaled_raw_histogram); if (scaled_cor_histogram) { - free(scaled_cor_histogram); + hdr_close(scaled_cor_histogram); } hdr_init(1000, highest_trackable_value * 512, significant_figures, &scaled_cor_histogram); @@ -96,7 +97,7 @@ static char* test_create(void) mu_assert("Failed to allocate hdr_histogram", h != NULL); mu_assert("Incorrect array length", compare_int64(h->counts_len, 23552)); - free(h); + hdr_close(h); return 0; } @@ -127,6 +128,8 @@ static char* test_create_with_large_values(void) "99.0% Percentile", hdr_values_are_equivalent(h, 100000000, hdr_value_at_percentile(h, 99.0))); + hdr_close(h); + return 0; } @@ -155,6 +158,41 @@ static char* test_invalid_init(void) return 0; } +static char* test_bucket_config_shift_overflow(void) +{ + struct hdr_histogram* h = NULL; + + /* Regression test for a signed-left-shift overflow (UBSan) in + hdr_calculate_bucket_config. With lowest_discernible_value = 2^56 the + unit_magnitude is 56; a significant_figures of 2 yields + sub_bucket_half_count_magnitude = 7, so the sub_bucket_mask shift would + set bit 56 + 7 = 63 of an int64_t (undefined behaviour). This config + must be rejected with EINVAL rather than crashing. Originally found by + fuzzing via a crafted decoded log with lowest ~= 2^56. */ + int r = hdr_init(INT64_C(1) << 56, INT64_C(1) << 58, 2, &h); + mu_assert("Overflowing bucket config must return EINVAL", r == EINVAL); + mu_assert("Histogram must be NULL on rejected config", h == NULL); + + return 0; +} + +static char* test_bucket_config_reject_defines_cfg(void) +{ + /* The >61 guard rejects before sub_bucket_mask/bucket_count/counts_len are + computed; cfg must still be fully defined so a two-step-init caller that + mishandles the EINVAL return does not read uninitialized fields. */ + struct hdr_histogram_bucket_config cfg; + memset(&cfg, 0xAB, sizeof(cfg)); + + mu_assert("Overflowing bucket config must return EINVAL", + hdr_calculate_bucket_config(INT64_C(1) << 56, INT64_C(1) << 58, 2, &cfg) == EINVAL); + mu_assert("sub_bucket_mask must be defined on reject", cfg.sub_bucket_mask == 0); + mu_assert("bucket_count must be defined on reject", cfg.bucket_count == 0); + mu_assert("counts_len must be defined on reject", cfg.counts_len == 0); + + return 0; +} + static char* test_total_count(void) { load_histograms(); @@ -453,6 +491,8 @@ static char* test_out_of_range_values(void) mu_assert("Should successfully record value", hdr_record_value(h, 1000)); mu_assert("Should not record value", !hdr_record_value(h, 1001)); + hdr_close(h); + return 0; } @@ -489,6 +529,8 @@ static char* test_linear_iter_buckets_correctly(void) mu_assert("Number of steps", compare_int64(4, step_count)); mu_assert("Total count", compare_int64(6, total_count)); + hdr_close(h); + return 0; } @@ -523,17 +565,20 @@ static char* test_interval_recording(void) result = compare_histograms(expected_histogram, recorder_histogram); if (result) { - return result; + goto cleanup; } recorder_corrected_histogram = hdr_interval_recorder_sample(&recorder_corrected); result = compare_histograms(expected_corrected_histogram, recorder_corrected_histogram); - if (result) - { - return result; - } - return 0; +cleanup: + /* destroy closes recorder active+inactive (incl. sampled histograms) */ + hdr_close(expected_histogram); + hdr_close(expected_corrected_histogram); + hdr_interval_recorder_destroy(&recorder); + hdr_interval_recorder_destroy(&recorder_corrected); + + return result; } static char* reset_histogram_on_sample_and_recycle(void) @@ -558,6 +603,9 @@ static char* reset_histogram_on_sample_and_recycle(void) mu_assert("Should have been reset", compare_int64(0, sample1->total_count)); + hdr_close(sample1); /* leftover not held by recorder */ + hdr_interval_recorder_destroy(&recorder); + return 0; } @@ -565,6 +613,8 @@ static struct mu_result all_tests(void) { mu_run_test(test_create); mu_run_test(test_invalid_init); + mu_run_test(test_bucket_config_shift_overflow); + mu_run_test(test_bucket_config_reject_defines_cfg); mu_run_test(test_create_with_large_values); mu_run_test(test_invalid_significant_figures); mu_run_test(test_total_count); @@ -589,6 +639,12 @@ static int hdr_histogram_run_tests(void) { struct mu_result result = all_tests(); + /* free static fixtures (hdr_close is NULL-safe) */ + hdr_close(raw_histogram); + hdr_close(cor_histogram); + hdr_close(scaled_raw_histogram); + hdr_close(scaled_cor_histogram); + if (result.message != 0) { printf("hdr_histogram_test.%s(): %s\n", result.test, result.message); diff --git a/test/regression-bucket-config-shift-overflow.hlog b/test/regression-bucket-config-shift-overflow.hlog new file mode 100644 index 0000000..6851a8f --- /dev/null +++ b/test/regression-bucket-config-shift-overflow.hlog @@ -0,0 +1,6 @@ +#[Logged with jHiccup version 2.0.3-SNAPSHOT] +#[Histogram log format version 1.01] +#[StartTime: 1403476110.183 (jNJQMVDobQwlGZCo0fB8ASMo0EwGs+jYDS+R8HwBAC+mAXh1zcKRsEoGAWjYBSMgqEAAHWdBeo= +35.046,5.000,0.115,HISTiQAAAEV42pNpmdzBwMDAxLgMQvFQBlSegQNC8W2BCp+Bis+H0hpQmgtKM0NpRoZRMApGwSgYBaGfiQAAAEZ42pNpmdzBwMDAxAAGfgoMDMxuBjsWQLgMQilQBiOUZoNQAnJQ4bNQ8UgoLQylWaA01NxRMApGwSgYBaNgFAwlAABjqwXo +30.046,5.000,0.131,HISGwSgYBaNgFFAAAOooBt0= +