acdb: fix dangling payload and stale tail references

[shijlin-1224]

A freed payload pointer could remain visible to
concurrent readers, and removing a tail node could leave p_tail pointing to a detached list node.
Invalidate freed payload pointers and update p_tail when removing the current tail node to prevent
dangling references and potential heap corruption.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed Commits: 2e56ea9

• 2e56ea9: acdb: fix dangling payload and stale tail references

A freed payload pointer could remain visible to
concurrent readers, and removing a tail node could
leave p_tail pointing to a detached list node.
Invalidate freed payload pointers and update p_tail
when removing the current tail node to prevent
dangling references and potential heap corruption.

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

Pull Request Overview

This PR introduces critical bug fixes for memory management and linked list operations in the ACDB (Audio Calibration Database) software.

Files Changed Summary

File	Lines Changed	Issues Found	Highest Severity
acdb/src/acdb_data_proc.c	+3 lines	1	High
acdb/src/acdb_utility.c	+2, -11 lines	1	High

Key Changes

1. acdb_data_proc.c: Added NULL pointer assignment and size reset after freeing heap calibration data payload
2. acdb_utility.c: Fixed linked list operations including tail pointer management and simplified move-to-end logic

Critical Issues Identified

• High Severity: Missing tail pointer update in list removal operation could cause dangling pointer access
• High Severity: Potential use-after-free vulnerability in heap calibration data update

Overall Assessment

The changes address important memory safety issues. However, one critical bug was introduced in the list removal logic that needs immediate attention before merge.

[FUNCTIONALITY] Missing tail pointer update in AcdbListRemove - High Severity

In the AcdbListRemove function at line 518-527, when removing the tail node, the code sets prev->p_next = NULL and node->p_next = NULL, but the new change adds list->p_tail = prev at line 525. However, this logic has a critical flaw: if prev is NULL (which can happen when the list has exactly 2 elements and we're removing the tail), this will set the tail pointer to NULL incorrectly, leaving the list in an inconsistent state.

The condition else if (list->p_tail == node) at line 518 doesn't guarantee that prev is non-NULL. When the list has 2 elements and we remove the tail, prev could be NULL if not properly tracked by the caller.

Impact: This can lead to a dangling tail pointer or NULL tail pointer when the list still has elements, causing crashes or memory corruption on subsequent list operations.

Fixed Code Snippet:

else if (list->p_tail == node)
{
    list->length--;
    if (!IsNull(prev))
    {
        prev->p_next = NULL;
        list->p_tail = prev;
    }
    else
    {
        // If prev is NULL and we're removing tail, list becomes empty or invalid
        list->p_tail = NULL;
        list->p_head = NULL;
    }
    node->p_next = NULL;
    return AR_EOK;
}

[SECURITY] Potential use-after-free in heap calibration data update - High Severity

In acdb_data_proc.c at lines 805-813, the code frees heap_caldata->param_payload and then immediately assigns a new pointer to it. While the added NULL assignment (line 806) and size reset (line 807) are good practices, there's a critical window where if req_caldata->param_payload is NULL or invalid, the heap_caldata structure will be left in an inconsistent state with a NULL payload but potentially non-zero size from the assignment at line 810.

The code should validate req_caldata->param_payload before freeing the old payload to ensure atomic update semantics.

Impact: If req_caldata->param_payload is NULL, the heap entry will have a NULL payload pointer but the size will be updated to req_caldata->param_size, leading to potential crashes when this calibration data is accessed later.

Fixed Code Snippet:

// Validate request data before modifying heap data
if (IsNull(req_caldata->param_payload) && req_caldata->param_size > 0)
{
    ACDB_ERR("Error: Request calibration data has NULL payload with non-zero size");
    continue; // Skip this update
}

// Remove old param data from heap map
heap_map->map_size -= heap_caldata->param_size;
heap_sg_data->subgraph_data_size -= heap_caldata->param_size;
heap_sg_data->non_global_data.data_size -= heap_caldata->param_size;

//Add request maps data size
heap_map->map_size += req_caldata->param_size;
heap_sg_data->subgraph_data_size += req_caldata->param_size;
heap_sg_data->non_global_data.data_size += req_caldata->param_size;

ACDB_FREE(heap_caldata->param_payload);
heap_caldata->param_payload = req_caldata->param_payload;
heap_caldata->param_size = req_caldata->param_size;

req_caldata->param_payload = NULL;
req_caldata->param_size = 0;

---

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

[qualcomm-ai-code-review-assistant]

Error Summary

All 2 suggested inline comments were outside the diff context and were skipped

Qualcomm AI Review