-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
@@ -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; |
There was a problem hiding this comment.
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)); \ |
There was a problem hiding this comment.
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
.
01bcbc2
to
67fa600
Compare
|
||
ion_test_write_all_values_from_binary(result, result_len, &result, &result_len, FALSE); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
67fa600
to
818b345
Compare
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:
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
free
s 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.