openstack
/
liberasurecode
Code Issues Proposed changes

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 <tim@swiftstack.com>

1: https://review.openstack.org/#/c/431812

Change-Id: I96f124e4e536bbd7544208acc084de1cda5c19b2
This commit is contained in:
Kota Tsuyuzaki 2017-02-15 01:08:25 -08:00
parent 10e096fe09
commit c9136a62b6
4 changed files with 110 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
Makefile.am
View File

@ -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

2
src/builtin/rs_vand/liberasurecode_rs_vand.c
View File

@ -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);

7
src/erasurecode.c
View File

@ -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(

110
test/liberasurecode_test.c
View File

@ -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);
}