Parser memory leak

[alexandremgo]

Hey! It seems that in some cases, our Ragel parsers create a memory leak. To reproduce, if we add for example " test" to the string being parsed ("rtsp://host:123/path?query test"):

https://github.com/roc-project/roc/blob/bc364daf9f77c83706354c19c139b111414a1a0d/src/tests/roc_address/test_endpoint_uri.cpp#L89-L102

The following error appears:

src/tests/roc_address/test_endpoint_uri.cpp:99: error: Failure in TEST(endpoint_uri, fields)
    expected <query>
    but was  <query test>
    difference starts at position 5 at: <     query test     >
                                                   ^

src/modules/roc_core/heap_allocator.cpp:26: error: roc_panic()

ERROR: roc_core: heap allocator: detected leak(s): 2 objects was not freed

Backtrace:
#1: 0x44b57a roc::core::print_backtrace()+0x5a
#2: 0x44ac8a roc::core::crash(char const*)+0x3a
#3: 0x44ac4e roc::core::panic(char const*, char const*, int, char const*, ...)+0x25e
#4: 0x449855 roc::core::HeapAllocator::~HeapAllocator()+0x85
#5: 0x433951 roc::address::TEST_GROUP_CppUTestGroupendpoint_uri::~TEST_GROUP_CppUTestGroupendpoint_uri()+0x31
#6: 0x433515 roc::address::TEST_endpoint_uri_fields_Test::~TEST_endpoint_uri_fields_Test()+0x15
#7: 0x433539 roc::address::TEST_endpoint_uri_fields_Test::~TEST_endpoint_uri_fields_Test()+0x19
#8: 0x45b32e UtestShell::runOneTestInCurrentProcess(TestPlugin*, TestResult&)+0x7e
#9: 0x45b972 PlatformSpecificSetJmpImplementation+0x42
#10: 0x458ea7 UtestShell::runOneTest(TestPlugin*, TestResult&)+0x57
#11: 0x458457 TestRegistry::runAllTests(TestResult&)+0xa7
#12: 0x44d6a9 CommandLineTestRunner::runAllTests()+0xe9
#13: 0x44d799 CommandLineTestRunner::runAllTestsMain()+0x79
#14: 0x44d8fd CommandLineTestRunner::RunAllTests(int, char const**)+0xad
#15: 0x444893 main+0xd3
#16: 0x7f7292b5bb97 __libc_start_main+0xe7
#17: 0x42890a _start+0x2a
Aborted (core dumped)

[gavv]

Hi Alexandre, sorry for late replay, was very tired last days.

Actually this is not a real leak. It's just an unexpected side effect of the test failure. We have the same problem with a few other tests as well.

The problem is caused by CppUTest behavior. When STRCMP_EQUAL (or any other check) fails, it performs a long jump (as you can see from backtrace BTW) and thus skips all local destructors. In our case, EndpointURI destructor is not called. Then it, however, it invokes test group destructor. In our case, it includes HeapAllocator destructor. And in turn, HeapAllocator notices unfreed allocations and reports a leak.

One workaround is to make HeapAllocator global, like it's done in most other tests:

diff --git a/src/tests/roc_address/test_endpoint_uri.cpp b/src/tests/roc_address/test_endpoint_uri.cpp
index acf57bb6..9de95099 100644
--- a/src/tests/roc_address/test_endpoint_uri.cpp
+++ b/src/tests/roc_address/test_endpoint_uri.cpp
@@ -15,9 +15,13 @@
 namespace roc {
 namespace address {

-TEST_GROUP(endpoint_uri) {
-    core::HeapAllocator allocator;
-};
+namespace {
+
+core::HeapAllocator allocator;
+
+} // namespace
+
+TEST_GROUP(endpoint_uri) {};

 TEST(endpoint_uri, empty) {
     EndpointURI u(allocator);

In this case, HeapAllocator destructor would be called later, after main() returns. But it wont happen because our main() calls fast_exit() if a test fails. This is done intentionally to avoid calling global destructors because of this long jump problem.

A more correct fix would be to force CppUTest to use C++ exceptions instead of long jumps. I saw something about exceptions in their docs, but didn't try it. However, maybe it's not a good idea too because we're not using exceptions and our code is not exception safe, so it can cause other mysterious errors in future :)

[alexandremgo]

Making the HeapAllocator global worked indeed, thanks for the explanation!

[gavv]

Resolved by 0066875333d834507751d898a2bbfa28a169a9d2