acdb: Fix memory-safety, locking and error-path bugs

[shijlin-1224]

Fix a heap buffer overflow in the diag transport, a double free during transport startup failure handling, and mutex locking bugs that could deadlock the TCP command and diagnostic servers. Also validate failure paths in AMDB command handlers and release locks
correctly when the file manager encounters existing database files.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed Commits: d0cd3c0

• d0cd3c0: acdb: Fix memory-safety, locking and error-path bugs

Fix a heap buffer overflow in the diag transport, a
double free during transport startup failure handling,
and mutex locking bugs that could deadlock the TCP
command and diagnostic servers. Also validate failure
paths in AMDB command handlers and release locks
correctly when the file manager encounters existing
database files.

Signed-off-by: Shijie Lin shijlin@qti.qualcomm.com

Pull Request Overview

This PR addresses multiple memory management and mutex handling issues across several files in the ACDB (Audio Calibration Database) codebase. The changes focus on fixing resource leaks, correcting mutex operations, and improving error handling paths.

Files Changed Summary

File	Lines Changed	Issues Found	Highest Severity
acdb/ats/transports/diag/linux/audtp/src/audtp.c	+5	1	High
acdb/ats/transports/tcpip/linux/src/ats_server.cpp	+1, -1	1	Medium
acdb/ats/transports/tcpip_server/src/tcpip_cmd_server.cpp	+7	2	High
acdb/ats/transports/tcpip_server/src/tcpip_dls_server.cpp	+1, -1	1	Medium
acdb/src/acdb.c	+12, -6	1	Low
acdb/src/acdb_file_mgr.c	+1	1	Medium

Critical Issues Identified

1. Buffer Overflow Vulnerability (High): Missing bounds check in copy_frame_to_buffer function could lead to buffer overflow
2. Resource Leaks (High): Missing mutex unlock operations in error paths in tcpip_cmd_server.cpp
3. Incorrect Mutex Operations (Medium): Using ar_osal_mutex_lock instead of ar_osal_mutex_unlock in multiple locations
4. Memory Leak (Medium): Missing socket cleanup in error path in ats_server.cpp

Positive Changes

• Added proper bounds checking before buffer operations
• Fixed mutex unlock operations in error handling paths
• Added missing resource cleanup (heap memory, mutex locks)

[SECURITY] High Severity - Buffer Overflow Prevention Added

The PR adds a critical bounds check in the copy_frame_to_buffer function to prevent potential buffer overflow. The check verifies that frame_ptr->header.frame_offset is within the bounds of buf_cntxt_ptr->buffer_length before performing memory copy operations.

Issue: Without this check, if a malformed frame with an offset beyond the buffer length is received, it could lead to writing beyond allocated memory, causing memory corruption or crashes.

Impact: This is a security vulnerability that could be exploited to cause denial of service or potentially execute arbitrary code.

Fixed Code Snippet:

if (frame_ptr->header.frame_offset >= buf_cntxt_ptr->buffer_length)
{
    return FALSE;
}

/** Calculate destination location where to copy the frame*/
dest_loc_ptr = buf_cntxt_ptr->buffer_ptr + frame_ptr->header.frame_offset;

Severity: High - This prevents a potential buffer overflow vulnerability.

[FUNCTIONALITY] High Severity - Missing Mutex Unlock in Error Paths

In tcpip_cmd_server.cpp, the transmit_routine function acquires a mutex lock at line 461 but fails to release it in three error return paths (lines 468-469, 475-476, and 483-484). This causes a deadlock where the mutex remains locked if memory allocation fails, preventing any future connections from being processed.

Issue: When ar_heap_malloc fails for message_buffer.buffer, recieve_buffer.buffer, or outbuf, the function returns without calling ar_osal_mutex_unlock(connection_lock). This leaves the mutex in a locked state permanently.

Impact: After the first allocation failure, all subsequent connection attempts will hang indefinitely waiting for the mutex, effectively causing a denial of service.

Fixed Code Snippet:

message_buffer.buffer = (char_t*)ar_heap_malloc(message_buffer.buffer_size, &heap_inf);
if (NULL == message_buffer.buffer) {
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

recieve_buffer.buffer = (char_t*)ar_heap_malloc(recieve_buffer.buffer_size, &heap_inf);
if (NULL == recieve_buffer.buffer) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

outbuf = (char_t*)ar_heap_malloc(maxsize, &heap_inf);
if (NULL == outbuf) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_heap_free(recieve_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

Severity: High - Causes deadlock and denial of service.

[FUNCTIONALITY] Medium Severity - Incorrect Mutex Operation (Lock Instead of Unlock)

In both tcpip_cmd_server.cpp and tcpip_dls_server.cpp, the set_connected_lock function incorrectly calls ar_osal_mutex_lock instead of ar_osal_mutex_unlock after setting the is_connected value. This causes the mutex to remain locked after the function returns.

Issue: The function acquires the mutex at the beginning, modifies is_connected, and then attempts to release it. However, line 304 (tcpip_cmd_server.cpp) and line 116 (tcpip_dls_server.cpp) call ar_osal_mutex_lock again instead of ar_osal_mutex_unlock, resulting in a double-lock situation.

Impact: The mutex remains locked after the function completes, causing any subsequent attempt to acquire the same mutex to deadlock. This prevents proper connection state management.

Fixed Code Snippet:

int32_t TcpipCmdServer::set_connected_lock(uint8_t value)
{
    int32_t status = AR_EOK;

    status = ar_osal_mutex_lock(connection_lock);
    if (AR_FAILED(status))
    {
        return status;
    }

    is_connected = value;

    status = ar_osal_mutex_unlock(connection_lock);  // Fixed: was ar_osal_mutex_lock
    if (AR_FAILED(status))
    {
        return status;
    }

    return status;
}

Severity: Medium - Causes mutex deadlock in connection state management.

[FUNCTIONALITY] Medium Severity - Missing Resource Cleanup in Error Path

In ats_server.cpp, when thread creation fails at line 432-434, the function frees gateway_socket_ptr but doesn't close the accepted socket. This leaves the socket open and consumes system resources.

Issue: After accepting a connection (line 382), if ar_osal_thread_create fails, the code frees the gateway_socket_ptr memory but doesn't call atsclosesocket(*gateway_socket_ptr) to close the actual socket connection.

Impact: Each failed thread creation leaks a socket file descriptor, eventually exhausting available file descriptors and preventing new connections.

Fixed Code Snippet:

status = ar_osal_thread_create(&thd_ats_transmit, &thd_attr,
    routine, gateway_socket_ptr);
if (AR_FAILED(status))
{
    ATS_ERR("Error[%d]: Failed to create transmission thread #%d.\n", status, i);
    atsclosesocket(*gateway_socket_ptr);  // Close the socket before freeing
    ACDB_FREE(gateway_socket_ptr);
}

Severity: Medium - Causes resource leak that can lead to denial of service over time.

[FUNCTIONALITY] Medium Severity - Missing Mutex Unlock in Error Path

In acdb_file_mgr.c, the AcdbFileManAddDatabase function acquires a mutex lock at line 633 but fails to release it before returning an error at line 651 when a duplicate database file is detected.

Issue: When a duplicate database file is found, the function logs an error and returns AR_EALREADY without calling ACDB_MUTEX_UNLOCK(acdb_file_man_context.file_man_lock). This leaves the mutex locked.

Impact: After detecting a duplicate file, the mutex remains locked, causing any subsequent calls to AcdbFileManAddDatabase or other functions using the same mutex to deadlock.

Fixed Code Snippet:

if (NULL == ar_strstr(&fm_db_info->database_file.path[0], &db_file->path[0]))
    continue;
else
{
    ACDB_ERR("Error[%d]: The database file %s already exists", AR_EALREADY,
        db_file->path);
    ACDB_MUTEX_UNLOCK(acdb_file_man_context.file_man_lock);
    return AR_EALREADY;
}

Severity: Medium - Causes mutex deadlock when duplicate files are detected.

[FUNCTIONALITY] High Severity - Missing Resource Cleanup on Thread Exit

In tcpip_cmd_server.cpp, the transmit_routine function allocates outbuf using ar_heap_malloc but doesn't free it before returning at line 553. This causes a memory leak every time a transmission thread completes.

Issue: The function allocates memory for outbuf at line 479 but only frees message_buffer.buffer and recieve_buffer.buffer in the cleanup section (lines 547-548). The outbuf allocation is never freed.

Impact: Each client connection that completes normally leaks the memory allocated for outbuf (up to maxsize bytes, which is ATS_BUFFER_LENGTH). Over time, this causes significant memory consumption.

Fixed Code Snippet:

//Cleanup
TCPIP_CMD_SVR_DBG("%s: closing thread", thd_name.c_str());

ar_heap_free(message_buffer.buffer, &heap_inf);
ar_heap_free(recieve_buffer.buffer, &heap_inf);
ar_heap_free(outbuf, &heap_inf);  // Added: Free outbuf

is_connected = false;
ar_osal_mutex_unlock(connection_lock);
return 0;

Severity: High - Causes memory leak on every client connection.

---

⚠️ This review was generated using AI assistance. Please verify all suggestions before applying.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

[qualcomm-ai-code-review-assistant]

[SECURITY] High Severity - Buffer Overflow Prevention Added

The PR adds a critical bounds check in the copy_frame_to_buffer function to prevent potential buffer overflow. The check verifies that frame_ptr->header.frame_offset is within the bounds of buf_cntxt_ptr->buffer_length before performing memory copy operations.

Issue: Without this check, if a malformed frame with an offset beyond the buffer length is received, it could lead to writing beyond allocated memory, causing memory corruption or crashes.

Impact: This is a security vulnerability that could be exploited to cause denial of service or potentially execute arbitrary code.

Fixed Code Snippet

if (frame_ptr->header.frame_offset >= buf_cntxt_ptr->buffer_length)
{
    return FALSE;
}

/** Calculate destination location where to copy the frame*/
dest_loc_ptr = buf_cntxt_ptr->buffer_ptr + frame_ptr->header.frame_offset;

Severity: High - This prevents a potential buffer overflow vulnerability.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] High Severity - Missing Mutex Unlock in Error Paths

In tcpip_cmd_server.cpp, the transmit_routine function acquires a mutex lock at line 461 but fails to release it in three error return paths (lines 468-469, 475-476, and 483-484). This causes a deadlock where the mutex remains locked if memory allocation fails, preventing any future connections from being processed.

Issue: When ar_heap_malloc fails for message_buffer.buffer, recieve_buffer.buffer, or outbuf, the function returns without calling ar_osal_mutex_unlock(connection_lock). This leaves the mutex in a locked state permanently.

Impact: After the first allocation failure, all subsequent connection attempts will hang indefinitely waiting for the mutex, effectively causing a denial of service.

Fixed Code Snippet

message_buffer.buffer = (char_t*)ar_heap_malloc(message_buffer.buffer_size, &heap_inf);
if (NULL == message_buffer.buffer) {
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

recieve_buffer.buffer = (char_t*)ar_heap_malloc(recieve_buffer.buffer_size, &heap_inf);
if (NULL == recieve_buffer.buffer) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

outbuf = (char_t*)ar_heap_malloc(maxsize, &heap_inf);
if (NULL == outbuf) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_heap_free(recieve_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

Severity: High - Causes deadlock and denial of service.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] High Severity - Missing Mutex Unlock in Error Paths

In tcpip_cmd_server.cpp, the transmit_routine function acquires a mutex lock at line 461 but fails to release it in three error return paths (lines 468-469, 475-476, and 483-484). This causes a deadlock where the mutex remains locked if memory allocation fails, preventing any future connections from being processed.

Issue: When ar_heap_malloc fails for message_buffer.buffer, recieve_buffer.buffer, or outbuf, the function returns without calling ar_osal_mutex_unlock(connection_lock). This leaves the mutex in a locked state permanently.

Impact: After the first allocation failure, all subsequent connection attempts will hang indefinitely waiting for the mutex, effectively causing a denial of service.

Fixed Code Snippet

message_buffer.buffer = (char_t*)ar_heap_malloc(message_buffer.buffer_size, &heap_inf);
if (NULL == message_buffer.buffer) {
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

recieve_buffer.buffer = (char_t*)ar_heap_malloc(recieve_buffer.buffer_size, &heap_inf);
if (NULL == recieve_buffer.buffer) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

outbuf = (char_t*)ar_heap_malloc(maxsize, &heap_inf);
if (NULL == outbuf) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_heap_free(recieve_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

Severity: High - Causes deadlock and denial of service.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] High Severity - Missing Mutex Unlock in Error Paths

In tcpip_cmd_server.cpp, the transmit_routine function acquires a mutex lock at line 461 but fails to release it in three error return paths (lines 468-469, 475-476, and 483-484). This causes a deadlock where the mutex remains locked if memory allocation fails, preventing any future connections from being processed.

Issue: When ar_heap_malloc fails for message_buffer.buffer, recieve_buffer.buffer, or outbuf, the function returns without calling ar_osal_mutex_unlock(connection_lock). This leaves the mutex in a locked state permanently.

Impact: After the first allocation failure, all subsequent connection attempts will hang indefinitely waiting for the mutex, effectively causing a denial of service.

Fixed Code Snippet

message_buffer.buffer = (char_t*)ar_heap_malloc(message_buffer.buffer_size, &heap_inf);
if (NULL == message_buffer.buffer) {
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

recieve_buffer.buffer = (char_t*)ar_heap_malloc(recieve_buffer.buffer_size, &heap_inf);
if (NULL == recieve_buffer.buffer) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

outbuf = (char_t*)ar_heap_malloc(maxsize, &heap_inf);
if (NULL == outbuf) {
    ar_heap_free(message_buffer.buffer, &heap_inf);
    ar_heap_free(recieve_buffer.buffer, &heap_inf);
    ar_osal_mutex_unlock(connection_lock);
    return 0;
}

Severity: High - Causes deadlock and denial of service.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] Medium Severity - Incorrect Mutex Operation (Lock Instead of Unlock)

In both tcpip_cmd_server.cpp and tcpip_dls_server.cpp, the set_connected_lock function incorrectly calls ar_osal_mutex_lock instead of ar_osal_mutex_unlock after setting the is_connected value. This causes the mutex to remain locked after the function returns.

Issue: The function acquires the mutex at the beginning, modifies is_connected, and then attempts to release it. However, line 304 (tcpip_cmd_server.cpp) and line 116 (tcpip_dls_server.cpp) call ar_osal_mutex_lock again instead of ar_osal_mutex_unlock, resulting in a double-lock situation.

Impact: The mutex remains locked after the function completes, causing any subsequent attempt to acquire the same mutex to deadlock. This prevents proper connection state management.

Fixed Code Snippet

int32_t TcpipCmdServer::set_connected_lock(uint8_t value)
{
    int32_t status = AR_EOK;

    status = ar_osal_mutex_lock(connection_lock);
    if (AR_FAILED(status))
    {
        return status;
    }

    is_connected = value;

    status = ar_osal_mutex_unlock(connection_lock);  // Fixed: was ar_osal_mutex_lock
    if (AR_FAILED(status))
    {
        return status;
    }

    return status;
}

Severity: Medium - Causes mutex deadlock in connection state management.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] Medium Severity - Incorrect Mutex Operation (Lock Instead of Unlock)

In both tcpip_cmd_server.cpp and tcpip_dls_server.cpp, the set_connected_lock function incorrectly calls ar_osal_mutex_lock instead of ar_osal_mutex_unlock after setting the is_connected value. This causes the mutex to remain locked after the function returns.

Issue: The function acquires the mutex at the beginning, modifies is_connected, and then attempts to release it. However, line 304 (tcpip_cmd_server.cpp) and line 116 (tcpip_dls_server.cpp) call ar_osal_mutex_lock again instead of ar_osal_mutex_unlock, resulting in a double-lock situation.

Impact: The mutex remains locked after the function completes, causing any subsequent attempt to acquire the same mutex to deadlock. This prevents proper connection state management.

Fixed Code Snippet

int32_t TcpipCmdServer::set_connected_lock(uint8_t value)
{
    int32_t status = AR_EOK;

    status = ar_osal_mutex_lock(connection_lock);
    if (AR_FAILED(status))
    {
        return status;
    }

    is_connected = value;

    status = ar_osal_mutex_unlock(connection_lock);  // Fixed: was ar_osal_mutex_lock
    if (AR_FAILED(status))
    {
        return status;
    }

    return status;
}

Severity: Medium - Causes mutex deadlock in connection state management.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] Medium Severity - Missing Resource Cleanup in Error Path

In ats_server.cpp, when thread creation fails at line 432-434, the function frees gateway_socket_ptr but doesn't close the accepted socket. This leaves the socket open and consumes system resources.

Issue: After accepting a connection (line 382), if ar_osal_thread_create fails, the code frees the gateway_socket_ptr memory but doesn't call atsclosesocket(*gateway_socket_ptr) to close the actual socket connection.

Impact: Each failed thread creation leaks a socket file descriptor, eventually exhausting available file descriptors and preventing new connections.

Fixed Code Snippet

status = ar_osal_thread_create(&thd_ats_transmit, &thd_attr,
    routine, gateway_socket_ptr);
if (AR_FAILED(status))
{
    ATS_ERR("Error[%d]: Failed to create transmission thread #%d.\n", status, i);
    atsclosesocket(*gateway_socket_ptr);  // Close the socket before freeing
    ACDB_FREE(gateway_socket_ptr);
}

Severity: Medium - Causes resource leak that can lead to denial of service over time.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] Medium Severity - Missing Mutex Unlock in Error Path

In acdb_file_mgr.c, the AcdbFileManAddDatabase function acquires a mutex lock at line 633 but fails to release it before returning an error at line 651 when a duplicate database file is detected.

Issue: When a duplicate database file is found, the function logs an error and returns AR_EALREADY without calling ACDB_MUTEX_UNLOCK(acdb_file_man_context.file_man_lock). This leaves the mutex locked.

Impact: After detecting a duplicate file, the mutex remains locked, causing any subsequent calls to AcdbFileManAddDatabase or other functions using the same mutex to deadlock.

Fixed Code Snippet

if (NULL == ar_strstr(&fm_db_info->database_file.path[0], &db_file->path[0]))
    continue;
else
{
    ACDB_ERR("Error[%d]: The database file %s already exists", AR_EALREADY,
        db_file->path);
    ACDB_MUTEX_UNLOCK(acdb_file_man_context.file_man_lock);
    return AR_EALREADY;
}

Severity: Medium - Causes mutex deadlock when duplicate files are detected.

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] High Severity - Missing Resource Cleanup on Thread Exit

In tcpip_cmd_server.cpp, the transmit_routine function allocates outbuf using ar_heap_malloc but doesn't free it before returning at line 553. This causes a memory leak every time a transmission thread completes.

Issue: The function allocates memory for outbuf at line 479 but only frees message_buffer.buffer and recieve_buffer.buffer in the cleanup section (lines 547-548). The outbuf allocation is never freed.

Impact: Each client connection that completes normally leaks the memory allocated for outbuf (up to maxsize bytes, which is ATS_BUFFER_LENGTH). Over time, this causes significant memory consumption.

Fixed Code Snippet

//Cleanup
TCPIP_CMD_SVR_DBG("%s: closing thread", thd_name.c_str());

ar_heap_free(message_buffer.buffer, &heap_inf);
ar_heap_free(recieve_buffer.buffer, &heap_inf);
ar_heap_free(outbuf, &heap_inf);  // Added: Free outbuf

is_connected = false;
ar_osal_mutex_unlock(connection_lock);
return 0;

Severity: High - Causes memory leak on every client connection.