Skip to content

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

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.

Sign up for GitHub

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

Open
shijlin-1224 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions acdb/ats/transports/diag/linux/audtp/src/audtp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,11 @@ bool_t copy_frame_to_buffer (
buf_cntxt_ptr->buffer_length = frame_ptr->header.buffer_length;
}

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

/** Calculate destination loaction where to copy the frame*/
dest_loc_ptr = buf_cntxt_ptr->buffer_ptr + frame_ptr->header.frame_offset;
/** copy frame on to buffer */
Comment on lines 339 to 349

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand Down
2 changes: 1 addition & 1 deletion acdb/ats/transports/tcpip/linux/src/ats_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ void* ats_server_start_routine(void* arg)
if (AR_FAILED(status))
{
ATS_ERR("Error[%d]: Failed to create transmission thread #%d.\n", status, i);
ACDB_FREE(gateway_socket_ptr);
}
else
{
Comment on lines 434 to 440

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand All @@ -452,7 +453,6 @@ void* ats_server_start_routine(void* arg)
for (int thd = 0; thd < MAX_ATS_CLIENTS_ALLOWED; thd++)
{
ar_osal_thread_join_destroy(g_transmit_thread_holder[thd]);
ACDB_FREE(gateway_socket_ptr);
}
}
}
Expand Down
9 changes: 7 additions & 2 deletions acdb/ats/transports/tcpip_server/src/tcpip_cmd_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ int32_t TcpipCmdServer::set_connected_lock(uint8_t value)

is_connected = value;

status = ar_osal_mutex_lock(connection_lock);
status = ar_osal_mutex_unlock(connection_lock);
if (AR_FAILED(status))
{
return status;
Comment on lines 301 to 307

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand Down Expand Up @@ -464,19 +464,23 @@ void *TcpipCmdServer::transmit_routine(void *args)
recieve_buffer.buffer_size = TCPIP_CMD_SERVER_RECV_BUFFER_SIZE;
//Stores one message
message_buffer.buffer = (char_t*)ar_heap_malloc(message_buffer.buffer_size, &heap_inf);
if (NULL == message_buffer.buffer)
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) {
Comment on lines 464 to 473

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

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);
Comment on lines 469 to 479

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

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;
}
ar_mem_set(outbuf, 0, 1);
Comment on lines 476 to 486

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand Down Expand Up @@ -542,6 +546,7 @@ void *TcpipCmdServer::transmit_routine(void *args)

ar_heap_free(message_buffer.buffer, &heap_inf);
ar_heap_free(recieve_buffer.buffer, &heap_inf);
ar_heap_free(outbuf, &heap_inf);

is_connected = false;
ar_osal_mutex_unlock(connection_lock);
Comment on lines 546 to 552

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand Down
2 changes: 1 addition & 1 deletion acdb/ats/transports/tcpip_server/src/tcpip_dls_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ int32_t TcpipDlsServer::set_connected_lock(uint8_t value)

is_connected = value;

status = ar_osal_mutex_lock(connection_lock);
status = ar_osal_mutex_unlock(connection_lock);
if (AR_FAILED(status))
{
return status;
Comment on lines 113 to 119

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand Down
50 changes: 30 additions & 20 deletions acdb/src/acdb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,19 +448,23 @@ int32_t acdb_ioctl(uint32_t cmd_id,
{
status = AR_EBADPARAM;
}

status = AcdbCmdGetAmdbRegData(
(AcdbAmdbProcID*)cmd_struct, (AcdbBlob*)rsp_struct);
else
{
status = AcdbCmdGetAmdbRegData(
(AcdbAmdbProcID*)cmd_struct, (AcdbBlob*)rsp_struct);
}
break;
case ACDB_CMD_GET_AMDB_DEREGISTRATION_DATA:
if (IsNull(cmd_struct) || cmd_struct_size != sizeof(AcdbAmdbProcID) ||
IsNull(rsp_struct) || rsp_struct_size != sizeof(AcdbBlob))
{
status = AR_EBADPARAM;
}

status = AcdbCmdGetAmdbDeregData(
(AcdbAmdbProcID*)cmd_struct, (AcdbBlob*)rsp_struct);
else
{
status = AcdbCmdGetAmdbDeregData(
(AcdbAmdbProcID*)cmd_struct, (AcdbBlob*)rsp_struct);
}
break;
case ACDB_CMD_GET_SUBGRAPH_PROCIDS:
if (IsNull(cmd_struct) || cmd_struct_size != sizeof(AcdbCmdGetSubgraphProcIdsReq) ||
Expand Down Expand Up @@ -490,10 +494,11 @@ int32_t acdb_ioctl(uint32_t cmd_id,
{
status = AR_EBADPARAM;
}

status = AcdbCmdGetAmdbBootupLoadModules(
(AcdbAmdbProcID*)cmd_struct, (AcdbBlob*)rsp_struct);

else
{
status = AcdbCmdGetAmdbBootupLoadModules(
(AcdbAmdbProcID*)cmd_struct, (AcdbBlob*)rsp_struct);
}
break;
case ACDB_CMD_GET_TAGS_FROM_GKV:
if (IsNull(cmd_struct) || cmd_struct_size != sizeof(AcdbCmdGetTagsFromGkvReq) ||
Expand Down Expand Up @@ -683,30 +688,35 @@ int32_t acdb_ioctl(uint32_t cmd_id,
{
status = AR_EBADPARAM;
}

status = AcdbCmdGetAmdbRegDataV2(
(AcdbAmdbDbHandle*)cmd_struct, (AcdbBlob*)rsp_struct);
else
{
status = AcdbCmdGetAmdbRegDataV2(
(AcdbAmdbDbHandle*)cmd_struct, (AcdbBlob*)rsp_struct);
}
break;
case ACDB_CMD_GET_AMDB_DEREGISTRATION_DATA_V2:
if (IsNull(cmd_struct) || cmd_struct_size != sizeof(AcdbAmdbDbHandle) ||
IsNull(rsp_struct) || rsp_struct_size != sizeof(AcdbBlob))
{
status = AR_EBADPARAM;
}

status = AcdbCmdGetAmdbDeregDataV2(
(AcdbAmdbDbHandle*)cmd_struct, (AcdbBlob*)rsp_struct);
else
{
status = AcdbCmdGetAmdbDeregDataV2(
(AcdbAmdbDbHandle*)cmd_struct, (AcdbBlob*)rsp_struct);
}
break;
case ACDB_CMD_GET_AMDB_BOOTUP_LOAD_MODULES_V2:
if (IsNull(cmd_struct) || cmd_struct_size != sizeof(AcdbAmdbDbHandle) ||
IsNull(rsp_struct) || rsp_struct_size != sizeof(AcdbBlob))
{
status = AR_EBADPARAM;
}

status = AcdbCmdGetAmdbBootupLoadModulesV2(
(AcdbAmdbDbHandle*)cmd_struct, (AcdbBlob*)rsp_struct);

else
{
status = AcdbCmdGetAmdbBootupLoadModulesV2(
(AcdbAmdbDbHandle*)cmd_struct, (AcdbBlob*)rsp_struct);
}
break;
case ACDB_CMD_GET_GRAPH_ALIAS:
if (cmd_struct == NULL || cmd_struct_size != sizeof(AcdbGraphKeyVector) ||
Expand Down
1 change: 1 addition & 0 deletions acdb/src/acdb_file_mgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,7 @@ int32_t AcdbFileManAddDatabase(acdb_file_man_data_files_t *db_files,
{
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;
}
}
Comment on lines 647 to 653

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

Expand Down
Loading