From d0cd3c042e5b824ccac2de07aa89ab447682aa84 Mon Sep 17 00:00:00 2001 From: Shijie Lin Date: Tue, 8 Sep 2026 15:02:30 +0800 Subject: [PATCH] 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 --- .../transports/diag/linux/audtp/src/audtp.c | 5 ++ .../transports/tcpip/linux/src/ats_server.cpp | 2 +- .../tcpip_server/src/tcpip_cmd_server.cpp | 9 +++- .../tcpip_server/src/tcpip_dls_server.cpp | 2 +- acdb/src/acdb.c | 50 +++++++++++-------- acdb/src/acdb_file_mgr.c | 1 + 6 files changed, 45 insertions(+), 24 deletions(-) diff --git a/acdb/ats/transports/diag/linux/audtp/src/audtp.c b/acdb/ats/transports/diag/linux/audtp/src/audtp.c index 08620d92..a269fba1 100644 --- a/acdb/ats/transports/diag/linux/audtp/src/audtp.c +++ b/acdb/ats/transports/diag/linux/audtp/src/audtp.c @@ -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 */ diff --git a/acdb/ats/transports/tcpip/linux/src/ats_server.cpp b/acdb/ats/transports/tcpip/linux/src/ats_server.cpp index 8bd5a921..9dbaa7e8 100644 --- a/acdb/ats/transports/tcpip/linux/src/ats_server.cpp +++ b/acdb/ats/transports/tcpip/linux/src/ats_server.cpp @@ -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 { @@ -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); } } } diff --git a/acdb/ats/transports/tcpip_server/src/tcpip_cmd_server.cpp b/acdb/ats/transports/tcpip_server/src/tcpip_cmd_server.cpp index 44d66149..cf325253 100644 --- a/acdb/ats/transports/tcpip_server/src/tcpip_cmd_server.cpp +++ b/acdb/ats/transports/tcpip_server/src/tcpip_cmd_server.cpp @@ -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; @@ -464,12 +464,15 @@ 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) { ar_heap_free(message_buffer.buffer, &heap_inf); + ar_osal_mutex_unlock(connection_lock); return 0; } @@ -477,6 +480,7 @@ void *TcpipCmdServer::transmit_routine(void *args) 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); @@ -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); diff --git a/acdb/ats/transports/tcpip_server/src/tcpip_dls_server.cpp b/acdb/ats/transports/tcpip_server/src/tcpip_dls_server.cpp index b24dffcb..3ef24c01 100644 --- a/acdb/ats/transports/tcpip_server/src/tcpip_dls_server.cpp +++ b/acdb/ats/transports/tcpip_server/src/tcpip_dls_server.cpp @@ -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; diff --git a/acdb/src/acdb.c b/acdb/src/acdb.c index e5352b2c..bb697820 100644 --- a/acdb/src/acdb.c +++ b/acdb/src/acdb.c @@ -448,9 +448,11 @@ 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) || @@ -458,9 +460,11 @@ int32_t acdb_ioctl(uint32_t cmd_id, { 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) || @@ -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) || @@ -683,9 +688,11 @@ 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) || @@ -693,9 +700,11 @@ int32_t acdb_ioctl(uint32_t cmd_id, { 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) || @@ -703,10 +712,11 @@ int32_t acdb_ioctl(uint32_t cmd_id, { 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) || diff --git a/acdb/src/acdb_file_mgr.c b/acdb/src/acdb_file_mgr.c index 024845b7..199e8fbf 100644 --- a/acdb/src/acdb_file_mgr.c +++ b/acdb/src/acdb_file_mgr.c @@ -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; } }