Skip to content

[Prior 3] Fix dlt memleak - #918

Merged
santhoshsivanhere merged 10 commits into
masterfrom
fix-dlt-user-memleak
Oct 8, 2026
Merged

santhoshsivanhere merged 10 commits into
masterfrom
fix-dlt-user-memleak

Conversation

@minminlittleshrimp

Copy link
Copy Markdown
Collaborator

4756fc0 fix: out-of-bounds read in dlt_getloginfo_conv_ascii_to_* (#892)
50a7b75 refactor: simplify logstorage_fsync CMake LD_PRELOAD logic
c6e33d3 fix: stack buffer overflow in dlt_message_header_flags_v2() (#893)
5bf66c9 fix: heap buffer overflow in dlt_message_read_v2() (#894)
9a7bff0 fix: stack buffer overflow in dlt_logstorage_storage_dir_info (#896)
b14388a fix: free resources on daemon initialization error paths

dlt_free() held dlt_mutex while calling dlt_stop_threads(), but the
housekeeper thread needs dlt_mutex to proceed (e.g. inside
dlt_user_log_check_user_message). Since pthread_mutex_lock is not a
cancellation point, pthread_cancel could not interrupt the blocked
housekeeper, causing pthread_join to deadlock.

- Move dlt_stop_threads() before dlt_mutex_lock() in dlt_free()
- Add cooperative exit check at top of housekeeper loop (~500ms latency)
- On MSYS2/MinGW: cooperative wait (1s) + pthread_cancel fallback
- On Linux/other: pthread_cancel directly (no added latency)
- Reset dlt_user_housekeeper_exit_requested in dlt_start_threads()
  to prevent stale flag from previous dlt_free() cycle
- Guard CLOCK_MONOTONIC for MSYS2/MinGW in condattr and clock_gettime
- Add pthread_condattr_destroy() after pthread_cond_init()

Signed-off-by: LUU QUANG MINH <Minh.LuuQuang@vn.bosch.com>
Replace 'SpacesInParens: Never' (clang-format 16+) with
'SpacesInParentheses: false' (clang-format 14 compatible).

Signed-off-by: LUU QUANG MINH <Minh.LuuQuang@vn.bosch.com>
- Fix heap-buffer-overflow in dlt_event_handler_enable_fd when max_nfds=0
- Fix SEGV in offline_log tests: set dlt_logstorage_sync function pointer
  via dlt_logstorage_filter_set_strategy before calling dlt_logstorage_free
- Fix memory leaks in gtest_dlt_daemon_offline_log.cpp:
  - Add dlt_logstorage_free calls and manual cleanup for config_list,
    newest_file_list, working_file_name, log, records in various tests
  - Add dlt_logstorage_filter_set_strategy before dlt_logstorage_list_add
  - Free keys allocated by dlt_logstorage_create_keys
- Fix memory leaks in gtest_dlt_daemon_gateway.cpp:
  - Free strdup'd ip_address, ecuid, and client.servIP in store_connection test
  - Free strdup'd ip_address/ecuid in check_ip, check_ecu tests
  - Free p_control_msgs in allocate_control_messages test
  - Free heap-allocated connections before dlt_gateway_deinit
- Fix memory leaks in gtest_dlt_daemon_event_handler.cpp:
  - Free pfd, connections, receiver allocated in various tests
  - Use dlt_event_handler_cleanup_connections instead of manual free(pfd)
- Fix memory leaks in gtest_dlt_user.cpp, gtest_dlt_common.cpp,
  gtest_dlt_common_v2.cpp, gtest_dlt_daemon_common.cpp,
  gtest_dlt_daemon_multiple_files_logging.cpp
- Fix logstorage_fsync CMakeLists.txt for ASan LD_PRELOAD

Test results: 95% pass (19/20), only pre-existing
t_dlt_daemon_logstorage_setup_internal_storage.normal fails (unrelated).
On 32-bit targets, cache_size + sizeof(DltLogStorageCacheFooter) can
overflow when cache_size is near UINT_MAX, causing calloc to allocate
a tiny buffer and subsequent memcpy to overflow it (heap buffer overflow).

Use size_t for the total allocation size, reject zero cache_size, and
check for wrap-around before calling calloc. Add the same zero-check
to dlt_logstorage_write_msg_cache and dlt_logstorage_sync_msg_cache
to prevent NULL pointer dereference of the footer.

Also fix the test to use proper null-terminated strings for apids/ctids
and increase g_logstorage_cache_max to accommodate file_size=50.
When daemon initialization fails (e.g. port already in use), early
return paths in main() skipped cleanup of allocated resources,
causing memory leaks detected by AddressSanitizer (240 bytes in 3
allocations). Add static helper dlt_daemon_exit_cleanup() that
calls all cleanup functions in the correct order, and invoke it
before each early return -1 in main().
Replace unbounded strcat() calls with length-checked memcpy to prevent
stack buffer overflow when directory path + filename exceeds
DLT_OFFLINE_LOGSTORAGE_MAX_LOG_FILE_LEN (120 bytes). Filenames from
scandir() can be up to NAME_MAX (255 bytes), far exceeding the 121-byte
stack buffer.

Also fix test t_dlt_daemon_logstorage_setup_internal_storage.normal:
- Zero-initialize DltLogStorage and set config_mode to DLT_LOGSTORAGE_CONFIG_FILE
  so dlt_logstorage_device_connected() takes the correct code path
- Add dlt_daemon_logstorage_cleanup() before dlt_daemon_free() to avoid
  memory leaks detected by ASan
- Set offlineLogstorageMaxDevices=1 so cleanup loop iterates

Fix test t_dlt_get_log_state.normal: expect -1 (disconnected) since no
daemon is running during unit tests.
dlt_message_read_v2() called parser functions before verifying that
the input buffer contained the full header. A crafted DLTv2 message
with many WTGS tags could advance the read offset past the buffer
boundary, causing an out-of-bounds heap read.

Add a 'length' parameter to both
dlt_message_get_extraparameters_from_recievedbuffer_v2() and
dlt_message_get_extendedparameters_from_recievedbuffer_v2(), and
validate every read against the buffer length using a bounds-check
macro. Also add a pre-check in dlt_message_read_v2() to ensure
base header + extra parameters fit before parsing.

Update the call site in dlt_daemon_client.c to pass the correct
buffer length.

Fix t_dlt_get_log_state test to accept either -1 or 0 since the
value depends on whether a prior test triggered a log state update.
dlt_message_header_flags_v2() used memcpy() to copy attacker-controlled
tag names (WTGS), filenames (WSFLN), ECU IDs (WEID), app/context IDs
(WACID) into the caller-provided output buffer without checking if the
remaining capacity was sufficient. A crafted DLTv2 file with many tags
or long field values could overflow the stack buffer in dlt-convert-v2,
overwriting the return address.

Add bounds checks before every memcpy and snprintf call in the
function, returning DLT_RETURN_ERROR if the output buffer is too small.
Replace bare numeric constants with self-documenting macros:
- DLT_TEXT_NUL_SPACE (1): NUL terminator
- DLT_TEXT_PLACEHOLDER_SPACE (5): "----" placeholder + NUL
- DLT_TEXT_NUM5_SEP_SPACE (6): 5-digit number + separator
Combine the three-branch if/else that set the same ENVIRONMENT property
into a single set_tests_properties call with conditionally-built
LD_PRELOAD and ASAN_OPTIONS values. Initialize ASAN_ENV to empty upfront
to eliminate redundant else branches.
dlt_getloginfo_conv_ascii_to_uint16_t(), _int16_t(), _uint8_t(),
_conv_ascii_to_id(), and _conv_ascii_to_string() read from the response
buffer at rp + *rp_count without any bounds checking. A crafted
GET_LOG_INFO response with truncated data could cause an out-of-bounds
read past the end of the buffer.

Add a 'length' parameter to all five functions and check that
*rp_count + needed bytes does not exceed the buffer length before each
read. Update all call sites in dlt_client_parse_get_log_info_resp_text()
and dlt_client_parse_get_log_info_resp_text_v2() to compute the buffer
length via strlen() and pass it through. Update the unit test to pass
the buffer length.

@santhoshsivanhere santhoshsivanhere left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me.

@santhoshsivanhere
santhoshsivanhere merged commit f0fa5e2 into master Oct 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants