From c9136a62b6e7cc8701cd7206ef0367520a20b8b9 Mon Sep 17 00:00:00 2001 From: Kota Tsuyuzaki Date: Wed, 15 Feb 2017 01:08:25 -0800 Subject: [PATCH] Fix valgrind-check and memory leak Can you believe that we ware testing the memory leak with valgrind to just *bash scripts* instead of actual binaries on liberasurecode_test and libec_slap? That is why we cannot find such an easy memory leak[1] at the gate. Now this patch enable to run the valgrind against to the binaries. With this fix, we found various memory leak at liberasurecode_test as follows and this patch also fixes them: - If we create fake fragments, we're responsible for freeing all of the frags as well as the array holding the pointers to the frags. - If we allocate any space, we're responsible for freeing it. - If we create an EC descriptor, we're responsible for destroying it. - If we create a fragment or skip array, we're responsible for freeing it. - If that happens inside a loop, we're responsible for doing it *inside that same loop*. In addition to the test fix, this patch fixes following memory leaks at the code which is affected to other users (pyeclib, OpenStack Swift) * Refuse to decode fragments that aren't even long enough to include fragment headers. * Fix a small memory leak in the builtin rs_vand implementation. Closes-Bug: #1665242 Co-Authored-By: Tim Burke 1: https://review.openstack.org/#/c/431812 Change-Id: I96f124e4e536bbd7544208acc084de1cda5c19b2 --- Makefile.am | 19 +++- src/builtin/rs_vand/liberasurecode_rs_vand.c | 2 +- src/erasurecode.c | 7 ++ test/liberasurecode_test.c | 110 +++++++++++++++---- 4 files changed, 110 insertions(+), 28 deletions(-) diff --git a/Makefile.am b/Makefile.am index 5f6fb9b..f52e046 100644 --- a/Makefile.am +++ b/Makefile.am @@ -62,13 +62,22 @@ test: check VALGRIND_EXEC_COMMAND = $(LIBTOOL_COMMAND) valgrind --tool=memcheck \ --error-exitcode=1 --leak-check=yes --track-fds=yes \ - --malloc-fill=A5 --free-fill=DE --fullpath-after=. + --malloc-fill=A5 --free-fill=DE --fullpath-after=. --trace-children=yes valgrind-test: check - @$(VALGRIND_EXEC_COMMAND) ./test/alg_sig_test - @$(VALGRIND_EXEC_COMMAND) ./test/liberasurecode_test - @$(VALGRIND_EXEC_COMMAND) ./test/test_xor_hd_code - @$(VALGRIND_EXEC_COMMAND) ./test/libec_slap + $(eval SONAMES := $(shell find $(abs_top_builddir) -name '*.so')) + $(eval SODIRS := $(dir $(SONAMES))) + $(eval LD_LIBRARY_PATH := LD_LIBRARY_PATH="$(LD_LIBRARY_PATH):$(subst / ,/:,$(SODIRS))") + $(eval DYLD_LIBRARY_PATH := DYLD_LIBRARY_PATH="$(DYLD_LIBRARY_PATH):$(subst / ,/:,$(SODIRS))") + $(eval DYLD_FALLBACK_LIBRARY_PATH := DYLD_FALLBACK_LIBRARY_PATH=$(DYLD_FALLBACK_LIBRARY_PATH):"$(subst / ,/:,$(SODIRS))") + @$(LD_LIBRARY_PATH) $(DYLD_LIBRARY_PATH) $(DYLD_FALLBACK_LIBRARY_PATH) $(VALGRIND_EXEC_COMMAND) \ + ./test/alg_sig_test + @$(LD_LIBRARY_PATH) $(DYLD_LIBRARY_PATH) $(DYLD_FALLBACK_LIBRARY_PATH) $(VALGRIND_EXEC_COMMAND) \ + ./test/.libs/liberasurecode_test + @$(LD_LIBRARY_PATH) $(DYLD_LIBRARY_PATH) $(DYLD_FALLBACK_LIBRARY_PATH) $(VALGRIND_EXEC_COMMAND) \ + ./test/test_xor_hd_code + @$(LD_LIBRARY_PATH) $(DYLD_LIBRARY_PATH) $(DYLD_FALLBACK_LIBRARY_PATH) $(VALGRIND_EXEC_COMMAND) \ + ./test/.libs/libec_slap CLEANFILES = cscope.in.out cscope.out cscope.po.out diff --git a/src/builtin/rs_vand/liberasurecode_rs_vand.c b/src/builtin/rs_vand/liberasurecode_rs_vand.c index de88d34..94b1620 100644 --- a/src/builtin/rs_vand/liberasurecode_rs_vand.c +++ b/src/builtin/rs_vand/liberasurecode_rs_vand.c @@ -543,7 +543,7 @@ int liberasurecode_rs_vand_reconstruct(int *generator_matrix, char **data, char } region_dot_product(first_k_available, parity[destination_idx - k], parity_row, k, blocksize); } - + free(parity_row); free(decoding_matrix); free(inverse_decoding_matrix); free(first_k_available); diff --git a/src/erasurecode.c b/src/erasurecode.c index a63bc25..6206b47 100644 --- a/src/erasurecode.c +++ b/src/erasurecode.c @@ -587,6 +587,13 @@ int liberasurecode_decode(int desc, goto out; } + if (fragment_len < sizeof(fragment_header_t)) { + log_error("Fragments not long enough to include headers! " + "Need %zu, but got %lu.", sizeof(fragment_header_t), + (unsigned long)fragment_len); + ret = -EBADHEADER; + goto out; + } for (i = 0; i < num_fragments; ++i) { /* Verify metadata checksum */ if (is_invalid_fragment_header( diff --git a/test/liberasurecode_test.c b/test/liberasurecode_test.c index 8f89cd8..a80e324 100644 --- a/test/liberasurecode_test.c +++ b/test/liberasurecode_test.c @@ -373,6 +373,9 @@ int *create_skips_array(struct ec_args *args, int skip) static int create_fake_frags_no_meta(char ***array, int num_frags, const char *data, int data_len) { + // N.B. The difference from creat_frags_arry is to creat new + // memory allocation and set a copy of data/parity there. The + // allocated memory should be maintained by the caller. int _num_frags = 0; int i = 0; char **ptr = NULL; @@ -401,6 +404,10 @@ static int create_frags_array(char ***array, struct ec_args *args, int *skips) { + // N.B. this function sets pointer reference to the ***array + // from **data and **parity so DO NOT free each value of + // the array independently because the data and parity will + // be expected to be cleanup via liberasurecode_encode_cleanup int num_frags = 0; int i = 0; char **ptr = NULL; @@ -431,6 +438,12 @@ out: return num_frags; } +static void cleanup_avail_frags(char **avail_frags, + int num_frags) +{ + while(num_frags > 0) free(avail_frags[--num_frags]); + free(avail_frags); +} static int encode_failure_stub(void *desc, char **data, char **parity, int blocksize) { @@ -582,6 +595,7 @@ static void test_encode_invalid_args() assert(rc < 0); instance->common.ops->encode = orig_encode_func; + liberasurecode_instance_destroy(desc); free(orig_data); } @@ -613,6 +627,7 @@ static void test_encode_cleanup_invalid_args() rc = liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); assert(rc == 0); + liberasurecode_instance_destroy(desc); free(orig_data); } @@ -629,7 +644,10 @@ static void test_decode_invalid_args() int *skips = create_skips_array(&null_args, -1); char *decoded_data = NULL; uint64_t decoded_data_len = 0; - const char *fake_data = " "; + // fake_data len should be bigger than fragment_header_t for + // the verifications + int fake_data_len = 1024; + char *fake_data = create_buffer(fake_data_len, 'y'); desc = liberasurecode_instance_create(EC_BACKEND_NULL, &null_args); if (-EBACKENDNOTAVAIL == desc) { @@ -641,30 +659,40 @@ static void test_decode_invalid_args() // test with invalid fragments (no metadata headers) num_avail_frags = create_fake_frags_no_meta(&avail_frags, (null_args.k + null_args.m), - fake_data, strlen(fake_data)); + fake_data, fake_data_len); assert(num_avail_frags > 0); + free(fake_data); rc = liberasurecode_decode(desc, avail_frags, num_avail_frags, - strlen(fake_data), 1, + fake_data_len, 1, &decoded_data, &decoded_data_len); // no metadata headers w/ force_metadata_checks results in EBADHEADER assert(rc == -EBADHEADER); rc = liberasurecode_decode(desc, avail_frags, num_avail_frags, - strlen(fake_data), 0, + fake_data_len, 0, &decoded_data, &decoded_data_len); // no metadata headers w/o force_metadata_checks also results in EBADHEADER assert(rc == -EBADHEADER); + // encoded_fragment_len is too small + rc = liberasurecode_decode(desc, avail_frags, num_avail_frags, + 1, 1, &decoded_data, &decoded_data_len); + assert(rc == -EBADHEADER); + + cleanup_avail_frags(avail_frags, num_avail_frags); + // test with num_fragments < (k) num_avail_frags = create_fake_frags_no_meta(&avail_frags, (null_args.k - 1), " ", 1); assert(num_avail_frags > 0); rc = liberasurecode_decode(desc, avail_frags, num_avail_frags, - strlen(fake_data), 1, + fake_data_len, 1, &decoded_data, &decoded_data_len); assert(rc == -EINSUFFFRAGS); + cleanup_avail_frags(avail_frags, num_avail_frags); + rc = liberasurecode_encode(desc, orig_data, orig_data_size, &encoded_data, &encoded_parity, &encoded_fragment_len); assert(rc == 0); @@ -692,11 +720,15 @@ static void test_decode_invalid_args() encoded_fragment_len, 1, &decoded_data, NULL); assert(rc < 0); + free(skips); liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); + // N.B. create_frags_array sets pointer reference of either encoded_data + // or encoded_parity and they are cleaned up via + // liberasurecode_encode_cleanup free(avail_frags); free(orig_data); - } static void test_decode_cleanup_invalid_args() @@ -717,7 +749,7 @@ static void test_decode_cleanup_invalid_args() rc = liberasurecode_decode_cleanup(desc, NULL); assert(rc == 0); - + liberasurecode_instance_destroy(desc); free(orig_data); } @@ -753,6 +785,7 @@ static void test_reconstruct_fragment_invalid_args() assert(rc < 0); free(out_frag); + free(avail_frags); // Test for EINSUFFFRAGS // we have to call encode to get fragments which have valid header. @@ -766,9 +799,10 @@ static void test_reconstruct_fragment_invalid_args() rc = liberasurecode_reconstruct_fragment(desc, encoded_data, 1, encoded_fragment_len, 1, out_frag); assert(rc == -EINSUFFFRAGS); + free(orig_data); free(out_frag); - free(avail_frags); liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); } static void test_fragments_needed_invalid_args() @@ -797,6 +831,7 @@ static void test_fragments_needed_invalid_args() rc = liberasurecode_fragments_needed(desc, &frags_to_recon, &frags_to_exclude, NULL); assert(rc < 0); + liberasurecode_instance_destroy(desc); } static void test_get_fragment_metadata_invalid_args() { @@ -847,7 +882,8 @@ static void test_verify_stripe_metadata_invalid_args() { rc = liberasurecode_verify_stripe_metadata(desc, frags, 0); assert(rc == -EINVALIDPARAMS); - + liberasurecode_instance_destroy(desc); + free(frags); } static void test_get_fragment_partition() @@ -867,6 +903,8 @@ static void test_get_fragment_partition() desc = liberasurecode_instance_create(EC_BACKEND_NULL, &null_args); if (-EBACKENDNOTAVAIL == desc) { fprintf (stderr, "Backend library not available!\n"); + free(orig_data); + free(skips); return; } assert(desc > 0); @@ -876,7 +914,19 @@ static void test_get_fragment_partition() missing = alloc_and_set_buffer(sizeof(char*) * null_args.k, -1); - for(i = 0; i < null_args.m; i++) skips[i] = 1; + for(i = 0; i < null_args.m; i++) { + skips[i] = 1; + /* get_fragment_partition is going to NULL all the entries in + * encoded_data and encoded_parity before populating them again + * from avail_frags. Since we're explicitly *not* including these + * data frags in avail_frags, free them now. + */ + if (i < null_args.k) { + free(encoded_data[i]); + } else { + free(encoded_parity[i - null_args.k]); + } + } num_avail_frags = create_frags_array(&avail_frags, encoded_data, encoded_parity, &null_args, skips); @@ -885,11 +935,19 @@ static void test_get_fragment_partition() assert(0 == rc); for(i = 0; i < null_args.m; i++) assert(missing[i] == i); - assert(missing[++i] == -1); + // Loop already pushed us one past + assert(missing[i] == -1); + free(avail_frags); free(missing); - for(i = 0; i < null_args.m + 1; i++) skips[i] = 1; + skips[i] = 1; + if (i < null_args.k) { + free(encoded_data[i]); + } else { + free(encoded_parity[i - null_args.k]); + } + num_avail_frags = create_frags_array(&avail_frags, encoded_data, encoded_parity, &null_args, skips); @@ -905,6 +963,7 @@ static void test_get_fragment_partition() free(missing); free(skips); liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); free(avail_frags); free(orig_data); } @@ -1000,10 +1059,7 @@ static void encode_decode_test_impl(const ec_backend_id_t be_id, rc = liberasurecode_decode_cleanup(desc, decoded_data); assert(rc == 0); - if (desc) { - assert(0 == liberasurecode_instance_destroy(desc)); - } - + assert(0 == liberasurecode_instance_destroy(desc)); free(orig_data); free(avail_frags); } @@ -1075,11 +1131,12 @@ static void reconstruct_test_impl(const ec_backend_id_t be_id, rc = liberasurecode_reconstruct_fragment(desc, avail_frags, num_avail_frags, encoded_fragment_len, i, out); assert(rc == 0); assert(memcmp(out, cmp, encoded_fragment_len) == 0); + free(avail_frags); } free(orig_data); free(out); - free(avail_frags); liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); } static void test_fragments_needed_impl(const ec_backend_id_t be_id, @@ -1203,6 +1260,7 @@ static void test_fragments_needed_impl(const ec_backend_id_t be_id, assert(is_valid_fragment == 1); i++; } + liberasurecode_instance_destroy(desc); free(fragments_to_reconstruct); free(fragments_to_exclude); free(fragments_needed); @@ -1275,6 +1333,7 @@ static void test_get_fragment_metadata(const ec_backend_id_t be_id, struct ec_ar assert(be_version == be->common.ec_backend_version); } liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); free(orig_data); } @@ -1387,6 +1446,8 @@ static void test_decode_reconstruct_specific_error_case( EC_BACKEND_ISA_L_RS_VAND, &specific_1010_args); if (-EBACKENDNOTAVAIL == desc) { fprintf (stderr, "Backend library not available!\n"); + free(orig_data); + free(skips); return; } assert(desc > 0); @@ -1436,6 +1497,7 @@ static void test_decode_reconstruct_specific_error_case( assert(rc == 0); free(out_frag); + free(avail_frags); // cleanup all rc = liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); @@ -1444,12 +1506,8 @@ static void test_decode_reconstruct_specific_error_case( rc = liberasurecode_decode_cleanup(desc, decoded_data); assert(rc == 0); - if (desc) { - assert(0 == liberasurecode_instance_destroy(desc)); - } - + assert(0 == liberasurecode_instance_destroy(desc)); free(orig_data); - free(avail_frags); free(skips); } @@ -1510,6 +1568,8 @@ static void test_verify_stripe_metadata(const ec_backend_id_t be_id, if (-EBACKENDNOTAVAIL == desc) { fprintf (stderr, "Backend library not available!\n"); + free(orig_data); + free(skip); return; } assert(desc > 0); @@ -1527,8 +1587,10 @@ static void test_verify_stripe_metadata(const ec_backend_id_t be_id, assert(0 == rc); liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); free(orig_data); free(skip); + free(avail_frags); } static void verify_fragment_metadata_mismatch_impl(const ec_backend_id_t be_id, struct ec_args *args, @@ -1551,6 +1613,8 @@ static void verify_fragment_metadata_mismatch_impl(const ec_backend_id_t be_id, if (-EBACKENDNOTAVAIL == desc) { fprintf (stderr, "Backend library not available!\n"); + free(orig_data); + free(skip); return; } assert(desc > 0); @@ -1615,6 +1679,8 @@ static void verify_fragment_metadata_mismatch_impl(const ec_backend_id_t be_id, } } liberasurecode_encode_cleanup(desc, encoded_data, encoded_parity); + liberasurecode_instance_destroy(desc); + free(avail_frags); free(orig_data); free(skip); }