ar_osal: Fix leaks, lock handling and error-path issues - #111
shijlin-1224 wants to merge 1 commit into
Conversation
Fix a batch of issues in ar_osal, including missing error-path handling in file I/O, a mutex double-unlock in the signal module, an unchecked allocation in the log-packet helper, leaked file descriptors in the service-registry module, and a lock left held on an early return in the ION shared-memory module. Signed-off-by: Shijie Lin <shijlin@qti.qualcomm.com>
Qualcomm AI ReviewClick to expand Code ReviewReviewed Commits: 39bfac5
Fix a batch of issues in ar_osal, including missing error-path Signed-off-by: Shijie Lin shijlin@qti.qualcomm.com Pull Request OverviewThis PR addresses multiple control flow and resource management issues across the AR OSAL (Operating System Abstraction Layer) codebase for Linux platforms. Files Changed Summary
Critical Issues Identified
The changes primarily focus on fixing control flow issues (missing goto/break statements) and improving resource cleanup in error paths. These are important correctness fixes that prevent resource leaks and undefined behavior. [FUNCTIONALITY] Missing goto statement after error logging - High SeverityIn Impact: The code will attempt to check file size and perform operations on a NULL pointer, causing a crash or unpredictable behavior. Fixed Code Snippet: if (NULL == file_ptr) {
rc = AR_EBADPARAM;
AR_LOG_ERR(AR_OSAL_FILE_IO_LOG_TAG,"%s fail for %s err:%d %s\n",__func__, path, rc, strerror(errno));
goto done; // Added to prevent NULL pointer dereference
}[FUNCTIONALITY] Missing break statement in switch case - High SeverityIn Impact: Valid seek operations with Fixed Code Snippet: case AR_FSEEK_CURRENT:
fseek_ref = SEEK_CUR;
break; // Added to prevent fall-through
default :
AR_LOG_ERR(AR_OSAL_FILE_IO_LOG_TAG,"Invalid reference id %d\n", fseek_ref);
return AR_EFAILED;[FUNCTIONALITY] Missing goto statements causing fall-through to error handlers - High SeverityIn multiple signal functions ( Impact: Double-unlocking a mutex leads to undefined behavior and potential crashes. The mutex state becomes corrupted, affecting all threads using this signal. Fixed Code Snippet: rc = pthread_mutex_unlock(&the_signal->osal_mutex);
if (rc) {
AR_LOG_ERR(AR_OSAL_SIGNAL_LOG_TAG,"%s: Failed to unlock, rc = %d\n", __func__, rc);
rc = AR_EFAILED;
}
goto done; // Added to prevent fall-through to error handler
err_cond:
pthread_mutex_unlock(&the_signal->osal_mutex);[FUNCTIONALITY] Missing NULL pointer check before pointer arithmetic - High SeverityIn Impact: Attempting to add an offset to a NULL pointer results in undefined behavior and will likely cause a segmentation fault. Fixed Code Snippet: uint8_t *ptr = (uint8_t*)ar_osal_log_alloc(
logcode,
AR_OSAL_LOG_DIAG_HEADER_LENGTH + length);
if (NULL == ptr)
return NULL; // Added NULL check
return ptr + AR_OSAL_LOG_DIAG_HEADER_LENGTH;[RESOURCE MANAGEMENT] File descriptor leak in error path - Medium SeverityIn Impact: Repeated failures will exhaust available file descriptors, eventually preventing the process from opening new files or sockets. Fixed Code Snippet: fd_dsplder = open(ADSP_LOADER_PATH, O_WRONLY);
if(fd_dsplder < 0) {
AR_LOG_ERR(AR_OSAL_SERVREG_TAG, "%s: open (%s) fail - %s (%d)",
__func__, ADSP_LOADER_PATH, strerror(errno), errno);
rc = errno;
} else {
if (write(fd_dsplder, "1", 1) < 0) {
AR_LOG_ERR(AR_OSAL_SERVREG_TAG, "%s: write (%s) fail - %s (%d)",
__func__, ADSP_LOADER_PATH, strerror(errno), errno);
rc = errno;
}
close(fd_dsplder); // Added to prevent resource leak
}[RESOURCE MANAGEMENT] Multiple resource leaks in error paths - High SeverityIn Impact: Failed initialization attempts will leak file descriptors and memory, potentially exhausting system resources over time. Fixed Code Snippet: pdata->ion_fd = open(ION_DRIVER_PATH, O_RDWR);
if (pdata->ion_fd < 0) {
status = AR_ENOTEXIST;
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s ion fd open failed %s status %d\n", __func__, ION_DRIVER_PATH, status);
#if TARGET_ION_ABI_VERSION >= 2
ion_close(pdata->ion_handle); // Added cleanup
#else
close(pdata->ion_handle); // Added cleanup
#endif
free(pdata); // Added cleanup
pdata = NULL; // Added to prevent use-after-free
goto end;
}[CONCURRENCY] Missing mutex unlock in early return path - High SeverityIn Impact: After the first call with NULL pdata, the mutex remains locked permanently, causing all subsequent calls to Fixed Code Snippet: pthread_mutex_lock(&ar_shmem_lock);
if (pdata == NULL) {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s not in init state\n", __func__);
pthread_mutex_unlock(&ar_shmem_lock); // Added to prevent deadlock
return AR_EOK;
}[FUNCTIONALITY] Incorrect file descriptor validation - High SeverityIn Impact: If a valid file descriptor with value 0 is used, it won't be closed, causing a resource leak. The correct check should be Fixed Code Snippet: if (shmem_handle->ion_mem_fd >= 0) { // Changed from if (shmem_handle->ion_mem_fd)
close(shmem_handle->ion_mem_fd);
} else {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s Invalid ion_mem_fd \n", __func__);
} |
![qualcomm-ai-code-review-assistant[bot]](https://avatars.githubusercontent.com/in/4128059?s=60&v=4){kind=small}
| break; | ||
| case AR_FSEEK_CURRENT: | ||
| fseek_ref = SEEK_CUR; | ||
| break; | ||
| default : | ||
| AR_LOG_ERR(AR_OSAL_FILE_IO_LOG_TAG,"Invalid reference id %d\n", fseek_ref); | ||
| return AR_EFAILED; |
There was a problem hiding this comment.
[FUNCTIONALITY] Missing break statement in switch case - High Severity
In ar_fseek(), the AR_FSEEK_CURRENT case is missing a break statement, causing fall-through to the default case. This results in an error being returned even when the seek reference is valid.
Impact: Valid seek operations with AR_FSEEK_CURRENT will incorrectly fail and return AR_EFAILED, breaking file positioning functionality.
Fixed Code Snippet
case AR_FSEEK_CURRENT:
fseek_ref = SEEK_CUR;
break; // Added to prevent fall-through
default :
AR_LOG_ERR(AR_OSAL_FILE_IO_LOG_TAG,"Invalid reference id %d\n", fseek_ref);
return AR_EFAILED;| free(pdata); | ||
| pdata = NULL; | ||
| goto end; | ||
| } | ||
| pdata->ion_fd = open(ION_DRIVER_PATH, O_RDWR); | ||
| if (pdata->ion_fd < 0) { | ||
| status = AR_ENOTEXIST; | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s ion fd open failed %s status %d\n", __func__, ION_DRIVER_PATH, status); | ||
| #if TARGET_ION_ABI_VERSION >= 2 | ||
| ion_close(pdata->ion_handle); | ||
| #else |
There was a problem hiding this comment.
[RESOURCE MANAGEMENT] Multiple resource leaks in error paths - High Severity
In ar_shmem_init(), when opening ION file descriptors fails, previously allocated resources (ion_handle, ion_fd) are not properly cleaned up before returning. This causes resource leaks on initialization failures.
Impact: Failed initialization attempts will leak file descriptors and memory, potentially exhausting system resources over time.
Fixed Code Snippet
pdata->ion_fd = open(ION_DRIVER_PATH, O_RDWR);
if (pdata->ion_fd < 0) {
status = AR_ENOTEXIST;
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s ion fd open failed %s status %d\n", __func__, ION_DRIVER_PATH, status);
#if TARGET_ION_ABI_VERSION >= 2
ion_close(pdata->ion_handle); // Added cleanup
#else
close(pdata->ion_handle); // Added cleanup
#endif
free(pdata); // Added cleanup
pdata = NULL; // Added to prevent use-after-free
goto end;
}| pthread_mutex_lock(&ar_shmem_lock); | ||
| if (pdata == NULL) { | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s not in init state\n", __func__); | ||
| pthread_mutex_unlock(&ar_shmem_lock); | ||
| return AR_EOK; | ||
| } | ||
| if (pdata->ion_handle){ |
There was a problem hiding this comment.
[CONCURRENCY] Missing mutex unlock in early return path - High Severity
In ar_shmem_deinit(), when pdata is NULL, the function returns early without unlocking the mutex that was acquired at the beginning of the function. This causes a deadlock on subsequent calls.
Impact: After the first call with NULL pdata, the mutex remains locked permanently, causing all subsequent calls to ar_shmem_deinit() and other functions using the same mutex to hang indefinitely.
Fixed Code Snippet
pthread_mutex_lock(&ar_shmem_lock);
if (pdata == NULL) {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s not in init state\n", __func__);
pthread_mutex_unlock(&ar_shmem_lock); // Added to prevent deadlock
return AR_EOK;
}| if (status) { | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s:unmap failed. status %d\n", __func__, status); | ||
| } | ||
| if (shmem_handle->ion_mem_fd) { | ||
| if (shmem_handle->ion_mem_fd >= 0) { | ||
| close(shmem_handle->ion_mem_fd); | ||
| } else { | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s Invalid ion_mem_fd \n", __func__); |
There was a problem hiding this comment.
[FUNCTIONALITY] Incorrect file descriptor validation - High Severity
In ar_shmem_free(), the condition if (shmem_handle->ion_mem_fd) is used to check if the file descriptor is valid. However, file descriptor 0 (stdin) is a valid descriptor, so this check incorrectly treats fd=0 as invalid.
Impact: If a valid file descriptor with value 0 is used, it won't be closed, causing a resource leak. The correct check should be >= 0 to handle all valid file descriptors.
Fixed Code Snippet
if (shmem_handle->ion_mem_fd >= 0) { // Changed from if (shmem_handle->ion_mem_fd)
close(shmem_handle->ion_mem_fd);
} else {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s Invalid ion_mem_fd \n", __func__);
}
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: c197fa0 "gsl: Fix memory-safety, cleanup and buffer-management bugs Fix a heap over-read and a bad rollback size when growing the Signed-off-by: Shijie Lin shijlin@qti.qualcomm.com" Files Not Fully Analyzed
Pull Request OverviewThis PR contains bug fixes and improvements across multiple GSL (Graph Service Layer) components, focusing on memory management, error handling, and resource cleanup. Files Changed Summary
Critical Issues Identified
Overall AssessmentThe changes address important memory safety and resource management issues. Most fixes are well-implemented, though there are a few areas requiring attention before merge. [FUNCTIONALITY][HIGH] Incorrect off-by-one comparison in buffer validationIn The original code allowed Fixed Code Snippet: static void gsl_mark_buffer_as_avail(struct gsl_data_path_info *dp_info,
uint32_t buf_index)
{
GSL_MUTEX_LOCK(dp_info->lock);
if (buf_index < dp_info->config.num_buffs) // Changed from <= to <
clear_bit(dp_info->buff_used_status, buf_index);
GSL_MUTEX_UNLOCK(dp_info->lock);
}[RESOURCE MANAGEMENT][HIGH] Memory leak on allocation failureIn More critically, the code doesn't check return values from Fixed Code Snippet: if (ext_mem_cache.num_extern_mem_datapaths++ == 0) {
GSL_DBG("Init ext mem cache");
ext_mem_cache.entries = gsl_mem_zalloc(
sizeof(struct gsl_ext_mem_cache_entry) * GSL_MAX_CACHE_SIZE);
if (ext_mem_cache.entries == NULL) {
GSL_ERR("Failed to allocate ext mem cache entries");
ext_mem_cache.num_extern_mem_datapaths--;
GSL_MUTEX_UNLOCK(ext_mem_cache.num_dps_lock);
return;
}
ext_mem_cache.age_counter = 0;
if (ar_osal_mutex_create(&ext_mem_cache.global_cache_lock) != AR_EOK) {
gsl_mem_free(ext_mem_cache.entries); // Add cleanup
ext_mem_cache.entries = NULL;
ext_mem_cache.num_extern_mem_datapaths--;
GSL_MUTEX_UNLOCK(ext_mem_cache.num_dps_lock);
return;
}
for (i = 0; i < GSL_MAX_CACHE_SIZE; ++i)
ar_osal_mutex_create(&ext_mem_cache.entries[i].lock);
}[RESOURCE MANAGEMENT][HIGH] Memory leak on error path in graph calibrationIn The fix corrects the NULL check and moves the cleanup to a proper location, but there's still a potential issue: the code jumps to Fixed Code Snippet: rc = acdb_ioctl(ACDB_CMD_GET_SUBGRAPH_PROCIDS, &req, sizeof(req), &rsp,
sizeof(rsp));
if (rc != AR_EOK) {
GSL_ERR("ACDB get subgraph procids failed %d", rc);
goto cleanup; // rsp.sg_proc_ids is allocated, will be freed at cleanup
}
// ... processing code ...
cleanup:
if (rsp.sg_proc_ids) // Correct NULL check
gsl_mem_free(rsp.sg_proc_ids);
gsl_mem_free(cma_sg_info.subgraph_list);The fix is correct, but ensure all error paths after line 1072 go through the cleanup label. [FUNCTIONALITY][MEDIUM] Swapped buffer free operationsIn Fixed Code Snippet: case GSL_CMD_FREE_READ_BUFF:
for (i = 0; i < graph->read_info.config.num_buffs; ++i) // Correct: read_info
gsl_msg_free(&graph->read_info.buff_list[i].gsl_msg);
break;
case GSL_CMD_FREE_WRITE_BUFF:
for (i = 0; i < graph->write_info.config.num_buffs; ++i) // Correct: write_info
gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg);
break;[RESOURCE MANAGEMENT][MEDIUM] Incorrect cleanup order in deinitIn The corrected order ensures:
This is the correct cleanup sequence, though the severity is medium since the race condition window is small. Fixed Code Snippet: int32_t gsl_dls_client_deinit()
{
int32_t rc = AR_EOK;
GSL_DBG("deinitializing gls dls client");
gsl_dls_client_register_deregister_commit_log_buffer_event(GLS_DLS_EVENT_FLAG_DEREGISTER_COMMIT_LOG_BUFFER);
rc = __gpr_cmd_deregister(GSL_DLS_CLIENT_GPR_SRC_PORT);
if (AR_FAILED(rc)) {
GSL_ERR("failed to deregister dls source port with gpr. status %d", rc);
}
rc = gsl_shmem_free(&dls_client_ctxt.dls_shmem);
if (AR_FAILED(rc)) {
GSL_ERR("failed to free dls client shared memory. status %d", rc);
}
rc = gsl_signal_destroy(&dls_client_ctxt.sig); // Destroy signal last
if (AR_FAILED(rc)) {
GSL_ERR("failed to destroy dls client signal. status %d", rc);
}
dls_client_ctxt.is_dls_buffer_configured = 0;
dls_client_ctxt.buffer_ready_callback = NULL;
return rc;
} |
| { | ||
| GSL_MUTEX_LOCK(dp_info->lock); | ||
|
|
||
| if (buf_index <= dp_info->config.num_buffs) | ||
| if (buf_index < dp_info->config.num_buffs) | ||
| clear_bit(dp_info->buff_used_status, buf_index); | ||
|
|
||
| GSL_MUTEX_UNLOCK(dp_info->lock); |
There was a problem hiding this comment.
[FUNCTIONALITY][HIGH] Incorrect off-by-one comparison in buffer validation
In gsl_datapath.c, the function gsl_mark_buffer_as_avail uses <= comparison when it should use < for array bounds checking. This could allow accessing one element beyond the valid buffer array range, potentially causing undefined behavior or crashes.
The original code allowed buf_index to equal num_buffs, which would access buff_used_status[num_buffs] - one past the valid array indices (0 to num_buffs-1).
Fixed Code Snippet
static void gsl_mark_buffer_as_avail(struct gsl_data_path_info *dp_info,
uint32_t buf_index)
{
GSL_MUTEX_LOCK(dp_info->lock);
if (buf_index < dp_info->config.num_buffs) // Changed from <= to <
clear_bit(dp_info->buff_used_status, buf_index);
GSL_MUTEX_UNLOCK(dp_info->lock);
}| GSL_MUTEX_UNLOCK(ext_mem_cache.num_dps_lock); | ||
| return; | ||
| } | ||
| ext_mem_cache.age_counter = 0; | ||
| ar_osal_mutex_create(&ext_mem_cache.global_cache_lock); | ||
|
|
There was a problem hiding this comment.
[RESOURCE MANAGEMENT][HIGH] Memory leak on allocation failure
In gsl_datapath.c, the function ext_mem_cache_init allocates memory for cache entries but doesn't properly handle the failure case. When allocation fails, the function decrements the refcount and returns, but if this was not the first initialization attempt, there could be inconsistent state.
More critically, the code doesn't check return values from ar_osal_mutex_create calls that follow. If mutex creation fails after successful memory allocation, the allocated memory is never freed.
Fixed Code Snippet
if (ext_mem_cache.num_extern_mem_datapaths++ == 0) {
GSL_DBG("Init ext mem cache");
ext_mem_cache.entries = gsl_mem_zalloc(
sizeof(struct gsl_ext_mem_cache_entry) * GSL_MAX_CACHE_SIZE);
if (ext_mem_cache.entries == NULL) {
GSL_ERR("Failed to allocate ext mem cache entries");
ext_mem_cache.num_extern_mem_datapaths--;
GSL_MUTEX_UNLOCK(ext_mem_cache.num_dps_lock);
return;
}
ext_mem_cache.age_counter = 0;
if (ar_osal_mutex_create(&ext_mem_cache.global_cache_lock) != AR_EOK) {
gsl_mem_free(ext_mem_cache.entries); // Add cleanup
ext_mem_cache.entries = NULL;
ext_mem_cache.num_extern_mem_datapaths--;
GSL_MUTEX_UNLOCK(ext_mem_cache.num_dps_lock);
return;
}
for (i = 0; i < GSL_MAX_CACHE_SIZE; ++i)
ar_osal_mutex_create(&ext_mem_cache.entries[i].lock);
}| goto cleanup; | ||
| } | ||
| } | ||
| free_sg_proc_ids: | ||
| if(!rsp.sg_proc_ids) | ||
| gsl_mem_free(rsp.sg_proc_ids); | ||
| cleanup: | ||
| if(rsp.sg_proc_ids) | ||
| gsl_mem_free(rsp.sg_proc_ids); | ||
| gsl_mem_free(cma_sg_info.subgraph_list); | ||
| free_status_list: | ||
| gsl_mem_free(sg_cma_status_list.list); |
There was a problem hiding this comment.
[RESOURCE MANAGEMENT][HIGH] Memory leak on error path in graph calibration
In gsl_graph.c, the function gsl_graph_send_persist_cal has a memory leak where rsp.sg_proc_ids is allocated but not freed on certain error paths. The original code had an inverted NULL check (if(!rsp.sg_proc_ids)) which would attempt to free a NULL pointer and skip freeing valid allocations.
The fix corrects the NULL check and moves the cleanup to a proper location, but there's still a potential issue: the code jumps to cleanup label from line 1083, but rsp.sg_proc_ids might not be allocated yet at that point (allocation happens at line 1072).
Fixed Code Snippet
rc = acdb_ioctl(ACDB_CMD_GET_SUBGRAPH_PROCIDS, &req, sizeof(req), &rsp,
sizeof(rsp));
if (rc != AR_EOK) {
GSL_ERR("ACDB get subgraph procids failed %d", rc);
goto cleanup; // rsp.sg_proc_ids is allocated, will be freed at cleanup
}
// ... processing code ...
cleanup:
if (rsp.sg_proc_ids) // Correct NULL check
gsl_mem_free(rsp.sg_proc_ids);
gsl_mem_free(cma_sg_info.subgraph_list);The fix is correct, but ensure all error paths after line 1072 go through the cleanup label.
| break; | ||
|
|
||
| case GSL_CMD_FREE_READ_BUFF: | ||
| for (i = 0; i < graph->write_info.config.num_buffs; ++i) | ||
| gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg); | ||
| for (i = 0; i < graph->read_info.config.num_buffs; ++i) | ||
| gsl_msg_free(&graph->read_info.buff_list[i].gsl_msg); | ||
| break; | ||
|
|
||
| case GSL_CMD_FREE_WRITE_BUFF: | ||
| for (i = 0; i < graph->read_info.config.num_buffs; ++i) | ||
| gsl_msg_free(&graph->read_info.buff_list[i].gsl_msg); | ||
| for (i = 0; i < graph->write_info.config.num_buffs; ++i) | ||
| gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg); |
There was a problem hiding this comment.
[FUNCTIONALITY][MEDIUM] Swapped buffer free operations
In gsl_main.c, the gsl_ioctl function has swapped the read and write buffer operations for GSL_CMD_FREE_READ_BUFF and GSL_CMD_FREE_WRITE_BUFF cases. This would cause the wrong buffers to be freed, leading to resource leaks and potential use-after-free bugs.
Fixed Code Snippet
case GSL_CMD_FREE_READ_BUFF:
for (i = 0; i < graph->read_info.config.num_buffs; ++i) // Correct: read_info
gsl_msg_free(&graph->read_info.buff_list[i].gsl_msg);
break;
case GSL_CMD_FREE_WRITE_BUFF:
for (i = 0; i < graph->write_info.config.num_buffs; ++i) // Correct: write_info
gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg);
break;
c197fa0 to
39bfac5
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: 39bfac5 "ar_osal: Fix leaks, lock handling and error-path issues Fix a batch of issues in ar_osal, including missing error-path Signed-off-by: Shijie Lin shijlin@qti.qualcomm.com" Pull Request OverviewThis PR addresses multiple control flow and resource management issues across the AR OSAL (Operating System Abstraction Layer) codebase for Linux platforms. The changes primarily focus on fixing missing control flow statements, improving error handling paths, and ensuring proper resource cleanup. Files Changed Summary
Critical Issues Identified
Overall AssessmentThe PR makes important fixes to control flow and resource management, but several additional issues need to be addressed before merging. The most critical concerns are around resource cleanup on error paths and proper null pointer handling. [FUNCTIONALITY - High] Missing file handle cleanup on error path in ar_fopenIn Impact: File descriptor leak that can exhaust system resources over time. Fixed Code Snippet: if(access == AR_FOPEN_WRITE_ONLY || access == AR_FOPEN_WRITE_ONLY_APPEND)
{
if(AR_FILE_WRITE_MAX_SIZE <= ar_fsize(file_ptr))
{
AR_LOG_ERR(AR_OSAL_FILE_IO_LOG_TAG,
"%s max size detected - truncating to zero size \n", __func__ );
if(0 != ftruncate(fileno(file_ptr), 0))
{
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s fail to truncate %s err: %s\n",
__func__, path, strerror(errno));
fclose(file_ptr); // Add cleanup
rc = AR_EFAILED;
goto done;
}
}
}[SECURITY - High] Potential null pointer dereference in ar_log_pkt_allocIn More critically, the function returns Impact: Potential undefined behavior or security vulnerability if pointer arithmetic is performed on invalid memory addresses. Fixed Code Snippet: void *ar_log_pkt_alloc(uint16_t logcode, uint32_t length)
{
uint8_t *ptr = (uint8_t*)ar_osal_log_alloc(
logcode,
AR_OSAL_LOG_DIAG_HEADER_LENGTH + length);
if (NULL == ptr) {
return NULL;
}
return ptr + AR_OSAL_LOG_DIAG_HEADER_LENGTH;
}The fix is already present in the patch, which is good. This is a confirmation that the change correctly addresses the issue. [FUNCTIONALITY - High] Multiple resource leaks in ar_shmem_init error pathsIn
The patches add proper cleanup, but there's still a potential issue: after setting Impact: Resource leaks (file descriptors) that can exhaust system resources. Potential null pointer dereference if error handling is modified incorrectly in the future. Fixed Code Snippet: if (pdata->ion_handle < 0) {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s:ion dev open failed errno:%d\n", __func__, pdata->ion_handle);
status = AR_ENOTEXIST;
free(pdata);
pdata = NULL;
goto end;
}
pdata->ion_fd = open(ION_DRIVER_PATH, O_RDWR);
if (pdata->ion_fd < 0) {
status = AR_ENOTEXIST;
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s ion fd open failed %s status %d\n", __func__, ION_DRIVER_PATH, status);
#if TARGET_ION_ABI_VERSION >= 2
ion_close(pdata->ion_handle);
#else
close(pdata->ion_handle);
#endif
free(pdata);
pdata = NULL;
goto end;
}The patches correctly address these issues by adding proper cleanup code. [FUNCTIONALITY - High] Invalid file descriptor check in ar_shmem_freeIn
The patch correctly changes this to Impact: If Fixed Code Snippet: if (shmem_handle->ion_mem_fd >= 0) {
close(shmem_handle->ion_mem_fd);
} else {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s Invalid ion_mem_fd \n", __func__);
}[FUNCTIONALITY - Medium] Missing mutex unlock in ar_shmem_deinit early returnIn The patch correctly adds Impact: Deadlock condition where any subsequent call to functions using Fixed Code Snippet: pthread_mutex_lock(&ar_shmem_lock);
if (pdata == NULL) {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s not in init state\n", __func__);
pthread_mutex_unlock(&ar_shmem_lock);
return AR_EOK;
}[FUNCTIONALITY - Medium] Missing file descriptor close in ar_osal_servreg functionsIn
Impact: File descriptor leaks that accumulate over time, potentially exhausting system resources. Fixed Code Snippet for ar_osal_servreg_ssr: fd_dsplder = open(ADSP_LOADER_PATH, O_WRONLY);
if(fd_dsplder < 0) {
AR_LOG_ERR(AR_OSAL_SERVREG_TAG, "%s: open (%s) fail - %s (%d)",
__func__, ADSP_LOADER_PATH, strerror(errno), errno);
rc = errno;
} else {
if (write(fd_dsplder, "1", 1) < 0) {
AR_LOG_ERR(AR_OSAL_SERVREG_TAG, "%s: write (%s) fail - %s (%d)",
__func__, ADSP_LOADER_PATH, strerror(errno), errno);
rc = errno;
}
close(fd_dsplder);
}[FUNCTIONALITY - Medium] Inconsistent control flow in signal functionsIn
The patches add Impact: Without these goto statements, execution would fall through to the error handling code even on success, potentially causing incorrect behavior. Fixed Code Snippet for ar_osal_signal_wait: rc = pthread_mutex_unlock(&the_signal->osal_mutex);
if (rc) {
AR_LOG_ERR(AR_OSAL_SIGNAL_LOG_TAG,"%s: Failed to unlock, rc = %d\n", __func__, rc);
rc = AR_EFAILED;
}
goto done;
err_cond:
pthread_mutex_unlock(&the_signal->osal_mutex);
done:
return rc; |
| if (status) { | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s:unmap failed. status %d\n", __func__, status); | ||
| } | ||
| if (shmem_handle->ion_mem_fd) { | ||
| if (shmem_handle->ion_mem_fd >= 0) { | ||
| close(shmem_handle->ion_mem_fd); | ||
| } else { | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s Invalid ion_mem_fd \n", __func__); |
There was a problem hiding this comment.
[FUNCTIONALITY - High] Invalid file descriptor check in ar_shmem_free
In ar_osal_shmem_ion.c at line 396, the code checks if (shmem_handle->ion_mem_fd) to determine if the file descriptor is valid. This is incorrect because:
- File descriptor 0 (stdin) is a valid file descriptor
- The check should be
>= 0to properly validate the descriptor - Invalid file descriptors are typically -1
The patch correctly changes this to if (shmem_handle->ion_mem_fd >= 0), which is the proper way to check for valid file descriptors.
Impact: If ion_mem_fd is 0 (a valid descriptor), it would be treated as invalid, causing the else branch to execute and log an error incorrectly. This could lead to file descriptor leaks.
Fixed Code Snippet
if (shmem_handle->ion_mem_fd >= 0) {
close(shmem_handle->ion_mem_fd);
} else {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s Invalid ion_mem_fd \n", __func__);
}| pthread_mutex_lock(&ar_shmem_lock); | ||
| if (pdata == NULL) { | ||
| AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s not in init state\n", __func__); | ||
| pthread_mutex_unlock(&ar_shmem_lock); | ||
| return AR_EOK; | ||
| } | ||
| if (pdata->ion_handle){ |
There was a problem hiding this comment.
[FUNCTIONALITY - Medium] Missing mutex unlock in ar_shmem_deinit early return
In ar_osal_shmem_ion.c at line 689-692, when pdata is NULL, the function logs an error and returns early. However, the mutex ar_shmem_lock was acquired at line 688 and is not released before the return, causing a deadlock on subsequent calls.
The patch correctly adds pthread_mutex_unlock(&ar_shmem_lock); before the return statement.
Impact: Deadlock condition where any subsequent call to functions using ar_shmem_lock will hang indefinitely.
Fixed Code Snippet
pthread_mutex_lock(&ar_shmem_lock);
if (pdata == NULL) {
AR_LOG_ERR(AR_OSAL_SHMEM_LOG_TAG,"%s not in init state\n", __func__);
pthread_mutex_unlock(&ar_shmem_lock);
return AR_EOK;
}1 participant
Fix a batch of issues in ar_osal, including missing error-path handling in file I/O, a mutex double-unlock in the signal module, an unchecked allocation in the log-packet helper, leaked file descriptors in the service-registry module, and a lock left held on an early return in the ION shared-memory module.
CRs-Fixed: 4642470