acdb: Fix stale delta file handle reuse

[shijlin-1224]

Several delta-file management paths could operate on a FILE handle after it had already been closed, leading to stale-pointer dereferences and double-close errors. Invalidate each handle immediately after fclose() so subsequent cleanup paths cannot reuse it.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed Commits: 15f58ad

• 15f58ad: acdb: Fix stale delta file handle reuse

Several delta-file management paths could operate on a
FILE handle after it had already been closed, leading
to stale-pointer dereferences and double-close errors.
Invalidate each handle immediately after fclose() so
subsequent cleanup paths cannot reuse it.

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

PR Overview

This pull request adds null pointer checks before closing file handles in the delta file manager to prevent potential crashes from attempting to close already-closed or uninitialized file handles.

Files Changed Summary

File	Lines Changed	Issues Found	Highest Severity
acdb/src/acdb_delta_file_mgr.c	~15 lines modified	2	High

Changes Made

• Added null checks before ar_fclose() calls in three functions:
  • AcdbDeltaDataCmdSave() - Lines 331-335
  • AcdbDeltaDeleteFile() - Lines 560-569
  • AcdbDeltaDataSwapDelta() - Lines 610-620
• Set file handle to NULL after closing in all three locations
• Removed one blank line at line 409

Critical Issues Identified

1. [High Severity - Error Handling] Missing error handling for ar_fclose() in AcdbDeltaDataCmdSave() could lead to silent failures

Positive Aspects

• The changes properly prevent null pointer dereferences
• Consistent pattern applied across multiple functions
• File handles are properly nullified after closing

[ERROR HANDLING] Missing error handling for ar_fclose() in AcdbDeltaDataCmdSave() - High Severity

In the AcdbDeltaDataCmdSave() function at lines 331-335, the code calls ar_fclose(*fhandle) but does not check the return status. This is inconsistent with the other two modified functions (AcdbDeltaDeleteFile() and AcdbDeltaDataSwapDelta()) which both check the return status and handle errors appropriately.

If ar_fclose() fails, the function continues execution and attempts to delete the file, which could lead to:

• File corruption if the close operation failed due to buffered data not being flushed
• Resource leaks if the file handle isn't properly released
• Inconsistent state where the file handle is set to NULL but the underlying resource isn't freed

This is particularly critical since this function is responsible for persisting delta data, and a failed close operation could result in data loss.

Fixed Code Snippet:

//Close and delete old delta file
if (*fhandle != NULL)
{
    status = ar_fclose(*fhandle);
    *fhandle = NULL;
    if (AR_FAILED(status))
    {
        ACDB_ERR("Error[%d]: Failed to close delta file before deletion", status);
        goto end;
    }
}

[FUNCTIONALITY] Potential double-free risk in error path of AcdbDeltaDataCmdSave() - Medium Severity

In AcdbDeltaDataCmdSave(), after closing and deleting the old delta file (lines 331-342), if ar_fdelete() fails, the function goes to the end label. However, the file handle has already been closed and set to NULL at line 334.

Later in the function, if any subsequent operation fails and execution reaches the end label, the file handle is already NULL, which is safe. However, there's a subtle issue: if the delete fails but the file was successfully closed, the db_info->file_handle is NULL, but db_info->exists and db_info->file_size are not updated to reflect the current state.

This could lead to inconsistent state where:

• The file handle is NULL (file is closed)
• But db_info->exists still indicates the file exists
• And db_info->file_size still has the old size

This inconsistency could cause issues in subsequent operations that check these flags.

Fixed Code Snippet:

status = ar_fdelete(db_info->delta_file_path.path);
if (AR_FAILED(status))
{
    ACDB_ERR("Error[%d]: Failed to delete delta file", status);
    db_info->exists = FALSE;  // Mark as non-existent since we can't delete it
    db_info->file_size = 0;
    goto end;
}

---

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

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

[qualcomm-ai-code-review-assistant]

[ERROR HANDLING] Missing error handling for ar_fclose() in AcdbDeltaDataCmdSave() - High Severity

In the AcdbDeltaDataCmdSave() function at lines 331-335, the code calls ar_fclose(*fhandle) but does not check the return status. This is inconsistent with the other two modified functions (AcdbDeltaDeleteFile() and AcdbDeltaDataSwapDelta()) which both check the return status and handle errors appropriately.

If ar_fclose() fails, the function continues execution and attempts to delete the file, which could lead to:

• File corruption if the close operation failed due to buffered data not being flushed
• Resource leaks if the file handle isn't properly released
• Inconsistent state where the file handle is set to NULL but the underlying resource isn't freed

This is particularly critical since this function is responsible for persisting delta data, and a failed close operation could result in data loss.

Fixed Code Snippet

//Close and delete old delta file
if (*fhandle != NULL)
{
    status = ar_fclose(*fhandle);
    *fhandle = NULL;
    if (AR_FAILED(status))
    {
        ACDB_ERR("Error[%d]: Failed to close delta file before deletion", status);
        goto end;
    }
}

[qualcomm-ai-code-review-assistant]

[FUNCTIONALITY] Potential double-free risk in error path of AcdbDeltaDataCmdSave() - Medium Severity

In AcdbDeltaDataCmdSave(), after closing and deleting the old delta file (lines 331-342), if ar_fdelete() fails, the function goes to the end label. However, the file handle has already been closed and set to NULL at line 334.

Later in the function, if any subsequent operation fails and execution reaches the end label, the file handle is already NULL, which is safe. However, there's a subtle issue: if the delete fails but the file was successfully closed, the db_info->file_handle is NULL, but db_info->exists and db_info->file_size are not updated to reflect the current state.

This could lead to inconsistent state where:

• The file handle is NULL (file is closed)
• But db_info->exists still indicates the file exists
• And db_info->file_size still has the old size

This inconsistency could cause issues in subsequent operations that check these flags.

Fixed Code Snippet

status = ar_fdelete(db_info->delta_file_path.path);
if (AR_FAILED(status))
{
    ACDB_ERR("Error[%d]: Failed to delete delta file", status);
    db_info->exists = FALSE;  // Mark as non-existent since we can't delete it
    db_info->file_size = 0;
    goto end;
}