Skip to content

acdb: Fix stale delta file handle reuse #114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shijlin-1224 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

acdb: Fix stale delta file handle reuse #114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions acdb/src/acdb_delta_file_mgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,12 @@ int32_t AcdbDeltaDataCmdSave(void)

fhandle = &db_info->file_handle;

/* Close and delete old delta file */
ar_fclose(*fhandle);
//Close and delete old delta file
if (*fhandle != NULL)
{
ar_fclose(*fhandle);
*fhandle = NULL;
}

status = ar_fdelete(db_info->delta_file_path.path);
if (AR_FAILED(status))
Comment on lines 327 to 338

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +333 to 338

@qualcomm-ai-code-review-assistant qualcomm-ai-code-review-assistant Bot Sep 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down Expand Up @@ -403,7 +407,6 @@ int32_t AcdbDeltaDataCmdSave(void)
end:
AcdbListClear(p_map_list);
p_map_list = NULL;

return status;
}
int32_t AcdbDeltaInitHeap(acdb_context_handle_t *handle)
Expand Down Expand Up @@ -554,11 +557,15 @@ int32_t AcdbDeltaDeleteFile(uint32_t database_index)
&file_name_info, sizeof(file_name_info));

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

status = AcdbInitUtilDeleteDeltaFileData(
Expand Down Expand Up @@ -600,12 +607,16 @@ int32_t AcdbDeltaDataSwapDelta(AcdbDeltaDataSwapInfo *swap_info)
db_info = ACDB_DFM_DB_INFO_AT_INDEX(swap_info->file_index);

/* Close the previous delta file and open/create the new file */
status = ar_fclose(db_info->file_handle);
if (AR_FAILED(status))
if (db_info->file_handle != NULL)
{
ACDB_ERR("Error[%d]: Failed to close %s ", status,
db_info->delta_file_path.path);
return status;
status = ar_fclose(db_info->file_handle);
db_info->file_handle = NULL;
if (AR_FAILED(status))
{
ACDB_ERR("Error[%d]: Failed to close %s ", status,
db_info->delta_file_path.path);
return status;
}
}

status = acdb_file_man_ioctl(ACDB_FILE_MAN_GET_FILE_NAME,
Expand Down
Loading