Fixed leak in dewrapper.c, added code to help find code in the future. (#207)

* Fixed leak in dewrapper.c. Switched setting to a build variable that can be set in CMAKE.
Also updated leak detection to run a binary unload.
Cleanup up most of the errors I found.
Opened issues #210 and @212 to track leaks that still need to be addressed.

* Adding leak detection logic to k4a_unittest_init() so more tests get it automatically

* Addressing PR feedback

CMakeLists.txt

@@ -67,6 +67,10 @@ set(K4A_INCLUDE_DIR ${CMAKE_CURRENT_LIST_DIR}/include)
 # Source for the common version resource file
 set(K4A_VERSION_RC ${CMAKE_CURRENT_LIST_DIR}/version.rc.in)
 
+if ("${K4A_ENABLE_LEAK_DETECTION_CMAKE}" STREQUAL "1")
+    add_definitions(-DK4A_ENABLE_LEAK_DETECTION)
+endif()
+
 add_subdirectory(examples)
 add_subdirectory(src)
 add_subdirectory(tests)

src/dewrapper/dewrapper.c

@@ -116,6 +116,7 @@ static void free_shared_depth_image(void *buffer, void *context)
     if (count == 0)
     {
         shared_context->memory_free_cb(shared_context->buffer, shared_context->memory_free_cb_context);
+        free(context);
     }
 }
 

src/dynlib/dynlib_linux.c

@@ -92,6 +92,11 @@ k4a_result_t dynlib_create(const char *name, uint32_t major_ver, uint32_t minor_
         free(versioned_name);
     }
 
+    if (K4A_FAILED(result))
+    {
+        dynlib_t_destroy(*dynlib_handle);
+        *dynlib_handle = NULL;
+    }
     return result;
 }
 

src/dynlib/dynlib_windows.c

@@ -85,6 +85,12 @@ k4a_result_t dynlib_create(const char *name, uint32_t major_ver, uint32_t minor_
         free(versioned_name);
     }
 
+    if (K4A_FAILED(result))
+    {
+        dynlib_t_destroy(*dynlib_handle);
+        *dynlib_handle = NULL;
+    }
+
     return result;
 }
 

src/logging/logging.cpp

@@ -130,44 +130,48 @@ k4a_result_t logger_create(logger_config_t *config, logger_t *logger_handle)
     }
     context->logger = g_logger;
 
-    //[2018-08-27 10:44:23.218] [level] [threadID] <message>
-    // https://github.com/gabime/spdlog/wiki/3.-Custom-formatting
-    spdlog::set_pattern("[%Y-%m-%d %H:%M:%S.%e] [%^%l%$] [t=%t] %v");
-
-    // Set the default logging level
-    spdlog::set_level(spdlog::level::err);
-
-    // override the default logging level
-    if (logging_level && logging_level[0] != '\0')
+    if (g_logger)
     {
-        if (logging_level[0] == 't' || logging_level[0] == 'T')
-        {
-            // capture a severity of trace or higher
-            spdlog::set_level(spdlog::level::trace);
-        }
-        else if (logging_level[0] == 'i' || logging_level[0] == 'I')
-        {
-            // capture a severity of info or higher
-            spdlog::set_level(spdlog::level::info);
-        }
-        else if (logging_level[0] == 'w' || logging_level[0] == 'W')
-        {
-            // capture a severity of warning or higher
-            spdlog::set_level(spdlog::level::warn);
-        }
-        else if (logging_level[0] == 'e' || logging_level[0] == 'E')
-        {
-            // capture a severity of error or higher
-            spdlog::set_level(spdlog::level::err);
-        }
-        else if (logging_level[0] == 'c' || logging_level[0] == 'C')
-        {
-            // capture a severity of error or higher
-            spdlog::set_level(spdlog::level::critical);
-        }
-    }
 
-    g_logger->flush_on(spdlog::level::warn);
+        //[2018-08-27 10:44:23.218] [level] [threadID] <message>
+        // https://github.com/gabime/spdlog/wiki/3.-Custom-formatting
+        spdlog::set_pattern("[%Y-%m-%d %H:%M:%S.%e] [%^%l%$] [t=%t] %v");
+
+        // Set the default logging level
+        spdlog::set_level(spdlog::level::err);
+
+        // override the default logging level
+        if (logging_level && logging_level[0] != '\0')
+        {
+            if (logging_level[0] == 't' || logging_level[0] == 'T')
+            {
+                // capture a severity of trace or higher
+                spdlog::set_level(spdlog::level::trace);
+            }
+            else if (logging_level[0] == 'i' || logging_level[0] == 'I')
+            {
+                // capture a severity of info or higher
+                spdlog::set_level(spdlog::level::info);
+            }
+            else if (logging_level[0] == 'w' || logging_level[0] == 'W')
+            {
+                // capture a severity of warning or higher
+                spdlog::set_level(spdlog::level::warn);
+            }
+            else if (logging_level[0] == 'e' || logging_level[0] == 'E')
+            {
+                // capture a severity of error or higher
+                spdlog::set_level(spdlog::level::err);
+            }
+            else if (logging_level[0] == 'c' || logging_level[0] == 'C')
+            {
+                // capture a severity of error or higher
+                spdlog::set_level(spdlog::level::critical);
+            }
+        }
+
+        g_logger->flush_on(spdlog::level::warn);
+    }
     return K4A_RESULT_SUCCEEDED;
 }
 
@@ -178,8 +182,12 @@ void logger_destroy(logger_t logger_handle)
     // destroy the logger
     if (DEC_REF_VAR(g_logger_count) == 0)
     {
+        bool drop_logger = g_logger != NULL;
         g_logger = NULL;
-        spdlog::drop(K4A_LOGGER);
+        if (drop_logger)
+        {
+            spdlog::drop(K4A_LOGGER);
+        }
         g_logger_is_file_based = false;
     }
     context->logger = NULL;

src/record/sdk/CMakeLists.txt

@@ -5,6 +5,7 @@
 add_library(k4arecord SHARED
             playback.cpp
             record.cpp
+            dll_main.c
             ${CMAKE_CURRENT_BINARY_DIR}/version.rc
             )
 

src/record/sdk/dll_main.c

@@ -0,0 +1,54 @@
+#if _WIN32
+#include <windows.h>
+#include <process.h>
+#include <crtdbg.h>
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#if _WIN32
+#if K4A_ENABLE_LEAK_DETECTION
+
+// Enable this block for memory leak messaging to be send to STDOUT and debugger. This is useful for running a
+// script to run all tests and quickly review all output. Also enable EXE's for verification with application
+// verifier to to get call stacks of memory allocation. With memory leak addresses in hand use WinDbg command '!heap
+// -p -a <Address>' to get the stack.
+//
+// NOTES:
+//   Compile in debug mode.
+//   Compile with K4A_ENABLE_LEAK_DETECTION set
+
+BOOL WINAPI DllMain(_In_ HINSTANCE inst_dll, _In_ DWORD reason, _In_ LPVOID reserved)
+{
+    (void)inst_dll;
+    (void)reserved;
+
+    if (reason == DLL_PROCESS_ATTACH)
+    {
+        /* Send memory leak detection error to stdout and debugger*/
+        _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDOUT);
+        _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDOUT);
+        _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
+
+        /* Do memory check at binary unload after statics are freed */
+        _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
+    }
+    else if (reason == DLL_PROCESS_DETACH)
+    {
+        // Due to the flag _CRTDBG_LEAK_CHECK_DF, _CrtDumpMemoryLeaks() is called on unload
+    }
+    return TRUE;
+}
+#endif // K4A_ENABLE_LEAK_DETECTION
+#else  //!_WIN32
+typedef int make_c_compiler_happy_t;
+#endif //_WIN32
+
+#ifdef __cplusplus
+}
+#endif

src/sdk/CMakeLists.txt

@@ -4,8 +4,11 @@
 # Create K4A library
 add_library(k4a SHARED
             k4a.c
+            dll_main.c
             ${CMAKE_CURRENT_BINARY_DIR}/version.rc)
 
+
+
 # Generate k4a_export.h
 # This is a platform specific header defining the macros for the export functions
 # of the shared library. This header is referenced by k4a.h and needs to be installed

src/sdk/dll_main.c

@@ -0,0 +1,58 @@
+#if _WIN32
+#include <windows.h>
+#include <process.h>
+#include <crtdbg.h>
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#if _WIN32
+#if K4A_ENABLE_LEAK_DETECTION
+
+// Enable this block for memory leak messaging to be send to STDOUT and debugger. This is useful for running a
+// script to run all tests and quickly review all output. Also enable EXE's for verification with application
+// verifier to to get call stacks of memory allocation. With memory leak addresses in hand use WinDbg command '!heap
+// -p -a <Address>' to get the stack.
+//
+// NOTES:
+//   Compile in debug mode.
+//   Compile with K4A_ENABLE_LEAK_DETECTION set
+
+BOOL WINAPI DllMain(_In_ HINSTANCE inst_dll, _In_ DWORD reason, _In_ LPVOID reserved)
+{
+    (void)inst_dll;
+    (void)reserved;
+
+    if (reason == DLL_PROCESS_ATTACH)
+    {
+        /* Send memory leak detection error to stdout and debugger*/
+        _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+        _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDOUT);
+        _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDOUT);
+        _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
+
+        /* Do memory check at binary unload after statics are freed */
+        _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
+    }
+    else if (reason == DLL_PROCESS_DETACH)
+    {
+        // Due to the flag _CRTDBG_LEAK_CHECK_DF, _CrtDumpMemoryLeaks() is called on unload
+    }
+    return TRUE;
+}
+
+#endif // K4A_ENABLE_LEAK_DETECTION
+#else  //!_WIN32
+
+/* This is needed so that this source file is not empty. */
+typedef int make_c_compiler_happy_t;
+
+#endif //_WIN32
+
+#ifdef __cplusplus
+}
+#endif

tests/FirmwareTests/firmware_helper.cpp

@@ -821,5 +821,6 @@ int main(int argc, char **argv)
     int result = RUN_ALL_TESTS();
 
     tear_down_common_test();
+    k4a_unittest_deinit();
     return result;
 }

tests/IMUTests/UnitTest/imu_ut.cpp

@@ -256,6 +256,7 @@ TEST_F(imu_ut, create)
     imu_destroy(imu_handle1);
     imu_destroy(imu_handle2);
     tickcounter_destroy(tick);
+    calibration_destroy(calibration_handle);
 }
 
 TEST_F(imu_ut, start)
@@ -278,6 +279,7 @@ TEST_F(imu_ut, start)
     // Destroy the instance
     imu_destroy(imu_handle);
     tickcounter_destroy(tick);
+    calibration_destroy(calibration_handle);
 }
 
 TEST_F(imu_ut, get_sample)
@@ -344,6 +346,8 @@ TEST_F(imu_ut, get_sample)
     ASSERT_EQ(allocator_test_for_leaks(), 0);
     // Destroy the instance
     imu_destroy(imu_handle);
+    tickcounter_destroy(tick);
+    calibration_destroy(calibration_handle);
 }
 
 int main(int argc, char **argv)

tests/RecordTests/UnitTest/playback_perf.cpp

@@ -176,5 +176,7 @@ int main(int argc, char **argv)
     }
     g_test_file_name = std::string(argv[1]);
 
-    return RUN_ALL_TESTS();
+    int results = RUN_ALL_TESTS();
+    k4a_unittest_deinit();
+    return results;
 }

tests/RecordTests/UnitTest/playback_ut.cpp

@@ -787,5 +787,7 @@ int main(int argc, char **argv)
 
     ::testing::AddGlobalTestEnvironment(new SampleRecordings());
     ::testing::InitGoogleTest(&argc, argv);
-    return RUN_ALL_TESTS();
+    int results = RUN_ALL_TESTS();
+    k4a_unittest_deinit();
+    return results;
 }

tests/Transformation/transformation.cpp

@@ -463,6 +463,11 @@ TEST_F(transformation_ut, transformation_create_depth_only)
     image_dec_ref(transformed_depth_image);
     image_dec_ref(point_cloud_image);
     transformation_destroy(transformation_handle);
+    image_dec_ref(depth_image);
+    image_dec_ref(color_image);
+    image_dec_ref(transformed_color_image);
+    image_dec_ref(transformed_depth_image);
+    image_dec_ref(point_cloud_image);
 }
 
 TEST_F(transformation_ut, transformation_create_color_only)
@@ -548,6 +553,11 @@ TEST_F(transformation_ut, transformation_create_color_only)
     image_dec_ref(transformed_color_image);
     image_dec_ref(transformed_depth_image);
     transformation_destroy(transformation_handle);
+
+    image_dec_ref(color_image);
+    image_dec_ref(depth_image);
+    image_dec_ref(transformed_color_image);
+    image_dec_ref(transformed_depth_image);
 }
 
 int main(int argc, char **argv)

tests/UnitTests/utcommon/inc/utcommon.h

@@ -19,6 +19,7 @@ extern "C" {
 
 // Initialize default k4a specific unittest behavior
 void k4a_unittest_init();
+void k4a_unittest_deinit();
 
 #ifdef _WIN32
 void k4a_unittest_init_logging_with_processid();

tests/UnitTests/utcommon/utcommon.cpp

@@ -58,13 +58,34 @@ std::ostream &operator<<(std::ostream &s, const k4a_wait_result_t &val)
 
 extern "C" {
 
+static logger_t g_logger_handle = NULL;
+
+static void enable_leak_detection()
+{
+#if K4A_ENABLE_LEAK_DETECTION
+    // Enable this block for memory leak messaging to be send to STDOUT and debugger. This is useful for running a
+    // script to run all tests and quickly review all output. Also enable EXE's for verification with application
+    // verifier to to get call stacks of memory allocation. With memory leak addresses in hand use WinDbg command '!heap
+    // -p -a <Address>' to get the stack
+    _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+    _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+    _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+    _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDOUT);
+    _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDOUT);
+    _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
+    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
+#endif
+}
+
 void k4a_unittest_init()
 {
+    enable_leak_detection();
+
     logger_config_t logger_config;
-    logger_t logger_handle = NULL;
     logger_config_init_default(&logger_config);
     logger_config.env_var_log_to_a_file = "K4A_ENABLE_LOG_TO_A_FILE_TEST";
-    (void)logger_create(&logger_config, &logger_handle);
+    assert(g_logger_handle == NULL);
+    (void)logger_create(&logger_config, &g_logger_handle);
 
     // Mocked functions that return k4a_result_t should default to returning K4A_RESULT_FAILED
     // unless specifically expected
@@ -73,10 +94,20 @@ void k4a_unittest_init()
     DefaultValue<k4a_buffer_result_t>::Set(K4A_BUFFER_RESULT_FAILED);
 }
 
+void k4a_unittest_deinit()
+{
+    DefaultValue<k4a_result_t>::Clear();
+    DefaultValue<k4a_wait_result_t>::Clear();
+    DefaultValue<k4a_buffer_result_t>::Clear();
+    logger_destroy(g_logger_handle);
+}
+
 #ifdef _WIN32
 char g_log_file_name[1024];
 void k4a_unittest_init_logging_with_processid()
 {
+    enable_leak_detection();
+
     logger_config_t logger_config;
     logger_t logger_handle = NULL;
     logger_config_init_default(&logger_config);
@@ -106,34 +137,10 @@ int k4a_test_commmon_main(int argc, char **argv)
 
     ::testing::InitGoogleTest(&argc, argv);
 
-#if 0
-    // Enable this block for memory leak messaging to be send to STDOUT and debugger. This is useful for running a
-    // script to run all tests and quickly review all output. Also enable EXE's for verification with application
-    // verifier to to get call stacks of memory allocation. With memory leak addresses in hand use WinDbg command '!heap
-    // -p -a <Address>' to get the stack
-    //
-    // _CrtMemCheckpoint() called below after gtest initialize as it uses a log of static structures that get reported
-    // as leaks. This also avoids the leaks reported by SPD log used by the logger module.
-    _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
-    _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
-    _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
-    _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDOUT);
-    _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDOUT);
-    _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
-#endif
-
-#ifdef _WIN32
-    _CrtMemState s1;
-    _CrtMemCheckpoint(&s1);
-#ifndef DBG
-    (void)s1; // Not used outside of DEBUG builds
-#endif
-#endif
     int ret = RUN_ALL_TESTS();
 
-#ifdef _WIN32
-    _CrtMemDumpAllObjectsSince(&s1);
-#endif
+    k4a_unittest_deinit();
+
     return ret;
 }
 }

tests/throughput/throughput_perf.cpp

@@ -841,5 +841,7 @@ int main(int argc, char **argv)
 
         return 1; // Indicates an error or warning
     }
-    return RUN_ALL_TESTS();
+    int results = RUN_ALL_TESTS();
+    k4a_unittest_deinit();
+    return results;
 }