gsl: Fix memory-safety, cleanup and buffer-management bugs

[shijlin-1224]

Fix a heap over-read and a bad rollback size when growing the shared-memory client list, a missing allocation check and buffer-size miscalculations that could underflow or index out of bounds, a duplicate signal-destroy call, a proc-id-list leak caused by an inverted null check, and read/write buffer confusion that freed the wrong direction's buffers.

CRs-Fixed: 4642470

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed Commits: 773525d

• 773525d: gsl: Fix memory-safety, cleanup and buffer-management bugs

Fix a heap over-read and a bad rollback size when growing the
shared-memory client list, a missing allocation check and
buffer-size miscalculations that could underflow or index out
of bounds, a duplicate signal-destroy call, a proc-id-list
leak caused by an inverted null check, and read/write buffer
confusion that freed the wrong direction's buffers.

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

Files Not Fully Analyzed

• gsl/src/gsl_graph.c - Full file content skipped due to token limit

Pull Request Overview

This 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

File	Lines Changed	Issues Found	Highest Severity
gsl/src/gsl_cshm_mgr.c	~15	2	High
gsl/src/gsl_datapath.c	~20	2	High
gsl/src/gsl_dls_client.c	~10	1	Medium
gsl/src/gsl_graph.c	~10	2	High
gsl/src/gsl_main.c	~10	1	Medium

Critical Issues Identified

1. [HIGH] Memory Management: Fixed incorrect memory restoration logic in gsl_cshm_mgr.c that could cause memory corruption
2. [HIGH] Resource Leak: Fixed missing NULL check in gsl_datapath.c that could lead to memory allocation failure without proper cleanup
3. [HIGH] Memory Leak: Fixed incorrect conditional check in gsl_graph.c that prevented proper memory deallocation
4. [MEDIUM] Buffer Boundary: Fixed off-by-one error in buffer validation that could allow invalid buffer access
5. [MEDIUM] Resource Management: Corrected buffer deallocation logic that was freeing wrong buffers

Overall Assessment

The PR addresses several important bugs related to memory management and resource handling. The fixes are generally well-implemented and improve code robustness. All identified issues have appropriate severity levels and should be addressed before merging.

[MEMORY MANAGEMENT - High] Incorrect memory size calculation in gsl_cshm_mgr.c

In gsl_get_mem_id(), when expanding the client list, the code uses the new expanded size for both source and destination in gsl_memcpy(). This is incorrect because the source buffer (cshm_ctx_glb.cshm_info_list) still has the old size, and copying with the new size could read beyond allocated memory, causing undefined behavior or crashes.

Issue: Line 77 uses cshm_ctx_glb.num_max_cshm_list (the new doubled size) for the source size parameter, but the source buffer was allocated with old_max size.

Impact: This could lead to reading uninitialized memory or memory access violations when the list is expanded.

Fixed Code Snippet:

gsl_memcpy(mem_info_list, (sizeof(cshm_info_t*) * cshm_ctx_glb.num_max_cshm_list),
            cshm_ctx_glb.cshm_info_list, sizeof(cshm_info_t*) * old_max);

The fix correctly uses old_max for the source size, ensuring only valid memory is copied.

[RESOURCE LEAK - High] Missing NULL check after memory allocation in gsl_datapath.c

In ext_mem_cache_init(), after allocating memory for ext_mem_cache.entries, there's no NULL check before proceeding to use the allocated memory. If the allocation fails, the code continues execution and attempts to initialize mutex locks on a NULL pointer, leading to undefined behavior or crashes.

Issue: Lines 63-64 allocate memory but don't verify success before line 72 attempts to use it.

Impact: If memory allocation fails in low-memory conditions, the application will crash or exhibit undefined behavior. Additionally, the refcount is incremented even on failure, causing state inconsistency.

Fixed Code Snippet:

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;
}

The fix adds proper error handling to decrement the refcount and exit gracefully on allocation failure.

[BUFFER BOUNDARY - Medium] Off-by-one error in buffer validation in gsl_datapath.c

In gsl_mark_buffer_as_avail(), the condition if (buf_index <= dp_info->config.num_buffs) allows buf_index to equal num_buffs, which is out of bounds since valid indices are 0 to num_buffs - 1. This could lead to accessing memory beyond the allocated buffer array.

Issue: Line 438 uses <= instead of <, allowing an invalid index to pass validation.

Impact: If buf_index equals num_buffs, the subsequent clear_bit() operation will access memory outside the valid buffer range, potentially corrupting data or causing crashes.

Fixed Code Snippet:

if (buf_index < dp_info->config.num_buffs)
    clear_bit(dp_info->buff_used_status, buf_index);

The fix changes the comparison to < to ensure only valid buffer indices are processed.

[MEMORY LEAK - High] Incorrect conditional prevents memory deallocation in gsl_graph.c

In the cleanup section of gsl_graph_send_persist_cal(), the condition if(!rsp.sg_proc_ids) checks if the pointer is NULL before freeing it. This is backwards logic - memory should be freed when the pointer is NOT NULL. The current code will never free the allocated memory, causing a memory leak.

Issue: Line 1211 has inverted logic that prevents rsp.sg_proc_ids from being freed when it should be.

Impact: Every call to this function that allocates rsp.sg_proc_ids will leak memory, potentially causing memory exhaustion over time in long-running applications.

Fixed Code Snippet:

cleanup:
    if(rsp.sg_proc_ids)
        gsl_mem_free(rsp.sg_proc_ids);

The fix removes the negation operator to properly free memory when the pointer is valid.

[RESOURCE MANAGEMENT - Medium] Incorrect buffer deallocation in gsl_main.c

In the GSL_CMD_FREE_READ_BUFF and GSL_CMD_FREE_WRITE_BUFF cases, the code is freeing buffers from the wrong data path info structure. The read command is freeing write buffers and vice versa, which will leave the intended buffers allocated and free the wrong ones.

Issue: Lines 1816-1817 free write_info buffers for FREE_READ_BUFF, and lines 1821-1822 free read_info buffers for FREE_WRITE_BUFF.

Impact: This causes resource leaks for the intended buffers and potential double-free or use-after-free errors for the incorrectly freed buffers, leading to memory corruption or crashes.

Fixed Code Snippet:

case GSL_CMD_FREE_READ_BUFF:
    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->write_info.config.num_buffs; ++i)
        gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg);
    break;

The fix ensures each command frees the correct corresponding buffers.

---

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

[qualcomm-ai-code-review-assistant]

Files Not Fully Analyzed

• gsl/src/gsl_graph.c - Full file content skipped due to token limit

Qualcomm AI Review

[qualcomm-ai-code-review-assistant]

[MEMORY MANAGEMENT - High] Incorrect memory size calculation in gsl_cshm_mgr.c

In gsl_get_mem_id(), when expanding the client list, the code uses the new expanded size for both source and destination in gsl_memcpy(). This is incorrect because the source buffer (cshm_ctx_glb.cshm_info_list) still has the old size, and copying with the new size could read beyond allocated memory, causing undefined behavior or crashes.

Issue: Line 77 uses cshm_ctx_glb.num_max_cshm_list (the new doubled size) for the source size parameter, but the source buffer was allocated with old_max size.

Impact: This could lead to reading uninitialized memory or memory access violations when the list is expanded.

Fixed Code Snippet

gsl_memcpy(mem_info_list, (sizeof(cshm_info_t*) * cshm_ctx_glb.num_max_cshm_list),
            cshm_ctx_glb.cshm_info_list, sizeof(cshm_info_t*) * old_max);

The fix correctly uses old_max for the source size, ensuring only valid memory is copied.

[qualcomm-ai-code-review-assistant]

[RESOURCE LEAK - High] Missing NULL check after memory allocation in gsl_datapath.c

In ext_mem_cache_init(), after allocating memory for ext_mem_cache.entries, there's no NULL check before proceeding to use the allocated memory. If the allocation fails, the code continues execution and attempts to initialize mutex locks on a NULL pointer, leading to undefined behavior or crashes.

Issue: Lines 63-64 allocate memory but don't verify success before line 72 attempts to use it.

Impact: If memory allocation fails in low-memory conditions, the application will crash or exhibit undefined behavior. Additionally, the refcount is incremented even on failure, causing state inconsistency.

Fixed Code Snippet

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;
}

The fix adds proper error handling to decrement the refcount and exit gracefully on allocation failure.

[qualcomm-ai-code-review-assistant]

[BUFFER BOUNDARY - Medium] Off-by-one error in buffer validation in gsl_datapath.c

In gsl_mark_buffer_as_avail(), the condition if (buf_index <= dp_info->config.num_buffs) allows buf_index to equal num_buffs, which is out of bounds since valid indices are 0 to num_buffs - 1. This could lead to accessing memory beyond the allocated buffer array.

Issue: Line 438 uses <= instead of <, allowing an invalid index to pass validation.

Impact: If buf_index equals num_buffs, the subsequent clear_bit() operation will access memory outside the valid buffer range, potentially corrupting data or causing crashes.

Fixed Code Snippet

if (buf_index < dp_info->config.num_buffs)
    clear_bit(dp_info->buff_used_status, buf_index);

The fix changes the comparison to < to ensure only valid buffer indices are processed.

[qualcomm-ai-code-review-assistant]

[MEMORY LEAK - High] Incorrect conditional prevents memory deallocation in gsl_graph.c

In the cleanup section of gsl_graph_send_persist_cal(), the condition if(!rsp.sg_proc_ids) checks if the pointer is NULL before freeing it. This is backwards logic - memory should be freed when the pointer is NOT NULL. The current code will never free the allocated memory, causing a memory leak.

Issue: Line 1211 has inverted logic that prevents rsp.sg_proc_ids from being freed when it should be.

Impact: Every call to this function that allocates rsp.sg_proc_ids will leak memory, potentially causing memory exhaustion over time in long-running applications.

Fixed Code Snippet

cleanup:
    if(rsp.sg_proc_ids)
        gsl_mem_free(rsp.sg_proc_ids);

The fix removes the negation operator to properly free memory when the pointer is valid.

[qualcomm-ai-code-review-assistant]

[RESOURCE MANAGEMENT - Medium] Incorrect buffer deallocation in gsl_main.c

In the GSL_CMD_FREE_READ_BUFF and GSL_CMD_FREE_WRITE_BUFF cases, the code is freeing buffers from the wrong data path info structure. The read command is freeing write buffers and vice versa, which will leave the intended buffers allocated and free the wrong ones.

Issue: Lines 1816-1817 free write_info buffers for FREE_READ_BUFF, and lines 1821-1822 free read_info buffers for FREE_WRITE_BUFF.

Impact: This causes resource leaks for the intended buffers and potential double-free or use-after-free errors for the incorrectly freed buffers, leading to memory corruption or crashes.

Fixed Code Snippet

case GSL_CMD_FREE_READ_BUFF:
    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->write_info.config.num_buffs; ++i)
        gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg);
    break;

The fix ensures each command frees the correct corresponding buffers.

[qualcomm-ai-code-review-assistant]

[RESOURCE MANAGEMENT - Medium] Incorrect buffer deallocation in gsl_main.c

In the GSL_CMD_FREE_READ_BUFF and GSL_CMD_FREE_WRITE_BUFF cases, the code is freeing buffers from the wrong data path info structure. The read command is freeing write buffers and vice versa, which will leave the intended buffers allocated and free the wrong ones.

Issue: Lines 1816-1817 free write_info buffers for FREE_READ_BUFF, and lines 1821-1822 free read_info buffers for FREE_WRITE_BUFF.

Impact: This causes resource leaks for the intended buffers and potential double-free or use-after-free errors for the incorrectly freed buffers, leading to memory corruption or crashes.

Fixed Code Snippet

case GSL_CMD_FREE_READ_BUFF:
    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->write_info.config.num_buffs; ++i)
        gsl_msg_free(&graph->write_info.buff_list[i].gsl_msg);
    break;

The fix ensures each command frees the correct corresponding buffers.