Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address LeakSanitizer errors in unit tests #300

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

nirosys
Copy link
Contributor

@nirosys nirosys commented Jul 6, 2022

Issue #, if available: None

Description of changes:
Linux builds with more recent versions of GCC or clang are generating leak sanitizer errors. These errors include the stack traces where an allocation occurred, the number of times those allocations were not freed, and the total resulting size of the leaked data:

Direct leak of 917504 byte(s) in 14 object(s) allocated from:
    #0 0x4e6b2d in malloc (/home/runner/work/ion-c/ion-c/build/debug/test/all_tests+0x4e6b2d)
    #1 0x7fe40a6955ae in _ion_alloc_block /home/runner/work/ion-c/ion-c/ionc/ion_allocation.c:182:41
    #2 0x7fe40a695512 in _ion_alloc_owner /home/runner/work/ion-c/ion-c/ionc/ion_allocation.c:44:17
    #3 0x7fe40a6f446b in _ion_reader_make_new_reader /home/runner/work/ion-c/ion-c/ionc/ion_reader.c:291:29
    #4 0x7fe40a6f3e8e in _ion_reader_open_buffer_helper /home/runner/work/ion-c/ion-c/ionc/ion_reader.c:48:5
    #5 0x7fe40a6f3b47 in ion_reader_open_buffer /home/runner/work/ion-c/ion-c/ionc/ion_reader.c:32:5
    #6 0x7fe40a87f354 in ion_extractor_path_create_from_ion /home/runner/work/ion-c/ion-c/ionc/ion_extractor.c:218:5
    #7 0x8c4466 in IonExtractorSucceedsWhen_NumPathsAtMaximum_Test::TestBody() /home/runner/work/ion-c/ion-c/test/test_ion_extractor.cpp:650:9

Running unit tests on Ubuntu with clang 13 results in 480 stack traces with leaks. This PR addresses the leaks that were a result of leaked data from tests themselves, which accounts for 196 of the total 480 identified stack traces.

I've grouped all of the tests together into one PR since most of the changes are simply adding frees or otherwise 1 line releasing of resources allocated in the test. I'll add commentary where the changes are not as clear.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -406,7 +421,7 @@ void test_ion_binary_reader_threshold_for_int64_as_big_int(BYTE *data, size_t le
hREADER reader;
ION_TYPE type;
int64_t value;
ION_INT big_int_expected;
ION_INT *big_int_expected;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved big_int_expected to be heap allocated, to make releasing with ion_int_free possible. Otherwise freeing the internal data for an ION_INT requires implementation knowledge, which can change.

#define ION_DECIMAL_BINARY_READER_EXPECT_OVERFLOW(func, decimal_digits) \
ION_DECIMAL_TEXT_TO_BINARY(decimal_digits); \
ION_ASSERT_OK(ion_reader_close(reader)); \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ION_DECIMAL_EXPECT_OVERFLOW re-uses reader, allocating a new struct which leaks the original reader that was initialized in ION_DECIMAL_TEXT_TO_BINARY.

@nirosys nirosys force-pushed the rgiliam/leaksanitizer_tests branch from 01bcbc2 to 67fa600 Compare July 7, 2022 00:06

ion_test_write_all_values_from_binary(result, result_len, &result, &result_len, FALSE);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the way ion_test_write_all_values_from_binary is written, the destination &result is expected to be a pointer to a pointer that can receive the address of the buffer that the function will allocate. So re-using result here results in the previous result buffer being leaked, and result containing a new address. This occurs again below. I added more pointers & lengths to track and free the individual buffers.

@@ -32,7 +32,7 @@ class WriterTest : public ::testing::Test {
FILE *out;
};

iERR ion_test_open_file_writer(hWRITER *writer, FILE *out, BOOL is_binary) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ion_test_open_file_writer creates a stream that is not freed when the writer is closed. The caller is not given the stream so knowledge of the stream, and how to free it using the writer is needed. I updated the function to return the stream as an argument so that callers will be aware and can easily free it.

@@ -184,14 +193,15 @@ TEST(IonTextSymbol, ReaderReadsAnnotationSymbolZero) {
ION_STRING annotation_strs[1];
SIZE num_annotations;
ION_SYMBOL symbol;
char *tmp = NULL;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffer allocated by ion_string_strdup needs to be freed. The tmp variable will receive the address to the buffer, so it can be freed once the comparison is done, and re-used for each instance.

@nirosys nirosys marked this pull request as ready for review July 7, 2022 00:20
tgregg
tgregg previously approved these changes Jul 7, 2022
Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this cleanup!

test/test_ion_symbol.cpp Outdated Show resolved Hide resolved
test/test_ion_symbol.cpp Outdated Show resolved Hide resolved
@nirosys nirosys merged commit 9654552 into amazon-ion:master Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants