Skip to content

Fix issue with handling of failure during discard of metadata cache entries #5817

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

Merged
merged 1 commit into from
Oct 16, 2025
Merged

Fix issue with handling of failure during discard of metadata cache entries #5817

Show file tree
Hide file tree
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
10 changes: 8 additions & 2 deletions release_docs/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,13 +548,19 @@ Added Fortran wrapper h5fdsubfiling_get_file_mapping_f() for the subfiling file
# 🪲 Bug Fixes

## Library

### Fixed security issue CVE-2025-7068

Failures during the discard process on a metadata cache entry could cause the library to skip calling the callback to free the cache entry. This could result in resource leaks and issues with flushing and closing the metadata cache during file close. This has been fixed by noting errors during the discard process, but attempting to fully free a cache entry before signalling that an error has occurred.

Fixes GitHub issue #5578

### Fix bugs in object header operations

In some rare circumstances, such as deleting hard links that point to their own parent group in a file using the new file format, memory corruption could occur due to recursive operations changing data structures being operated on by multiple levels of recursion. Made changes to delay changing the data structure in a dangerous way until recursion is complete.

Fixes GitHub issue #5854


### Fixed security issues CVE-2025-6816, CVE-2025-6856 and CVE-2025-2923

A specially constructed HDF5 file could contain a corrupted object header with a continuation message that points back to itself. This could result in an internal buffer being allocated with too small of a size, leading to a heap buffer overflow. This has been fixed by checking the expected number of object header chunks against the actual value as chunks are being deserialized.
Expand All @@ -580,7 +586,7 @@ Added Fortran wrapper h5fdsubfiling_get_file_mapping_f() for the subfiling file
Fixes GitHub issue #5861

### Fixed security issue CVE-2025-2153

The message flags field could be modified such that a message that is not sharable according to the share_flags field in H5O_msg_class_t can be treated as sharable. An assert has been added in H5O__msg_write_real to make sure messages that are not sharable can't be modified to shared. Additionally, the check in H5O__chunk_deserialize that catches unsharable messages being marked as sharable has been improved.

Fixes GitHub issue #5329
Expand Down
259 changes: 148 additions & 111 deletions src/H5Centry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ static herr_t H5C__pin_entry_from_client(H5C_t *cache_ptr, H5C_cache_entry_t *en
static herr_t H5C__unpin_entry_real(H5C_t *cache_ptr, H5C_cache_entry_t *entry_ptr, bool update_rp);
static herr_t H5C__unpin_entry_from_client(H5C_t *cache_ptr, H5C_cache_entry_t *entry_ptr, bool update_rp);
static herr_t H5C__generate_image(H5F_t *f, H5C_t *cache_ptr, H5C_cache_entry_t *entry_ptr);
static herr_t H5C__discard_single_entry(H5F_t *f, H5C_t *cache_ptr, H5C_cache_entry_t *entry_ptr,
bool destroy_entry, bool free_file_space,
bool suppress_image_entry_frees);
static herr_t H5C__verify_len_eoa(H5F_t *f, const H5C_class_t *type, haddr_t addr, size_t *len, bool actual);
static void *H5C__load_entry(H5F_t *f,
#ifdef H5_HAVE_PARALLEL
Expand Down Expand Up @@ -753,118 +756,13 @@ H5C__flush_single_entry(H5F_t *f, H5C_cache_entry_t *entry_ptr, unsigned flags)
* Now discard the entry if appropriate.
*/
if (destroy) {
/* Sanity check */
assert(0 == entry_ptr->flush_dep_nparents);

/* if both suppress_image_entry_frees and entry_ptr->include_in_image
* are true, simply set entry_ptr->image_ptr to NULL, as we have
* another pointer to the buffer in an instance of H5C_image_entry_t
* in cache_ptr->image_entries.
*
* Otherwise, free the buffer if it exists.
*/
if (suppress_image_entry_frees && entry_ptr->include_in_image)
entry_ptr->image_ptr = NULL;
else if (entry_ptr->image_ptr != NULL)
entry_ptr->image_ptr = H5MM_xfree(entry_ptr->image_ptr);

/* If the entry is not a prefetched entry, verify that the flush
* dependency parents addresses array has been transferred.
*
* If the entry is prefetched, the free_isr routine will dispose of
* the flush dependency parents addresses array if necessary.
*/
if (!entry_ptr->prefetched) {
assert(0 == entry_ptr->fd_parent_count);
assert(NULL == entry_ptr->fd_parent_addrs);
} /* end if */

/* Check whether we should free the space in the file that
* the entry occupies
*/
if (free_file_space) {
hsize_t fsf_size;

/* Sanity checks */
assert(H5_addr_defined(entry_ptr->addr));
assert(!H5F_IS_TMP_ADDR(f, entry_ptr->addr));
#ifndef NDEBUG
{
size_t curr_len;

/* Get the actual image size for the thing again */
entry_ptr->type->image_len((void *)entry_ptr, &curr_len);
assert(curr_len == entry_ptr->size);
}
#endif

/* If the file space free size callback is defined, use
* it to get the size of the block of file space to free.
* Otherwise use entry_ptr->size.
*/
if (entry_ptr->type->fsf_size) {
if ((entry_ptr->type->fsf_size)((void *)entry_ptr, &fsf_size) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFREE, FAIL, "unable to get file space free size");
} /* end if */
else /* no file space free size callback -- use entry size */
fsf_size = entry_ptr->size;

/* Release the space on disk */
if (H5MF_xfree(f, entry_ptr->type->mem_type, entry_ptr->addr, fsf_size) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFREE, FAIL, "unable to free file space for cache entry");
} /* end if ( free_file_space ) */

/* Reset the pointer to the cache the entry is within. -QAK */
entry_ptr->cache_ptr = NULL;

/* increment entries_removed_counter and set
* last_entry_removed_ptr. As we are likely abuut to
* free the entry, recall that last_entry_removed_ptr
* must NEVER be dereferenced.
*
* Recall that these fields are maintained to allow functions
* that perform scans of lists of entries to detect the
* unexpected removal of entries (via expunge, eviction,
* or take ownership at present), so that they can re-start
* their scans if necessary.
*
* Also check if the entry we are watching for removal is being
* removed (usually the 'next' entry for an iteration) and reset
* it to indicate that it was removed.
*/
cache_ptr->entries_removed_counter++;
cache_ptr->last_entry_removed_ptr = entry_ptr;

if (entry_ptr == cache_ptr->entry_watched_for_removal)
cache_ptr->entry_watched_for_removal = NULL;

/* Check for actually destroying the entry in memory */
/* (As opposed to taking ownership of it) */
if (destroy_entry) {
if (entry_ptr->is_dirty) {
/* Reset dirty flag */
entry_ptr->is_dirty = false;

/* If the entry's type has a 'notify' callback send a
* 'entry cleaned' notice now that the entry is fully
* integrated into the cache.
*/
if (entry_ptr->type->notify &&
(entry_ptr->type->notify)(H5C_NOTIFY_ACTION_ENTRY_CLEANED, entry_ptr) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTNOTIFY, FAIL,
"can't notify client about entry dirty flag cleared");
} /* end if */

/* verify that the image has been freed */
assert(entry_ptr->image_ptr == NULL);
/* Make sure one of either `destroy_entry` or `take_ownership` are true */
assert(destroy_entry != take_ownership);

if (entry_ptr->type->free_icr((void *)entry_ptr) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "free_icr callback failed");
} /* end if */
else {
assert(take_ownership);
} /* end else */
} /* if (destroy) */
if (H5C__discard_single_entry(f, cache_ptr, entry_ptr, destroy_entry, free_file_space,
suppress_image_entry_frees) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFREE, FAIL, "can't discard cache entry");
}

/* Check if we have to update the page buffer with cleared entries
* so it doesn't go out of date
Expand All @@ -891,6 +789,145 @@ H5C__flush_single_entry(H5F_t *f, H5C_cache_entry_t *entry_ptr, unsigned flags)
FUNC_LEAVE_NOAPI(ret_value)
} /* H5C__flush_single_entry() */

/*-------------------------------------------------------------------------
* Function: H5C__discard_single_entry
*
* Purpose: Helper routine to discard a cache entry, freeing as much
* of the relevant file space and data structures as possible
* along the way.
*
* Return: FAIL if error is detected, SUCCEED otherwise.
*
*-------------------------------------------------------------------------
*/
static herr_t
H5C__discard_single_entry(H5F_t *f, H5C_t *cache_ptr, H5C_cache_entry_t *entry_ptr, bool destroy_entry,
bool free_file_space, bool suppress_image_entry_frees)
{
herr_t ret_value = SUCCEED;

FUNC_ENTER_PACKAGE

assert(f);
assert(cache_ptr);
assert(entry_ptr);

/* Sanity check */
assert(0 == entry_ptr->flush_dep_nparents);

/* if both suppress_image_entry_frees and entry_ptr->include_in_image
* are true, simply set entry_ptr->image_ptr to NULL, as we have
* another pointer to the buffer in an instance of H5C_image_entry_t
* in cache_ptr->image_entries.
*
* Otherwise, free the buffer if it exists.
*/
if (suppress_image_entry_frees && entry_ptr->include_in_image)
entry_ptr->image_ptr = NULL;
else if (entry_ptr->image_ptr != NULL)
entry_ptr->image_ptr = H5MM_xfree(entry_ptr->image_ptr);

/* If the entry is not a prefetched entry, verify that the flush
* dependency parents addresses array has been transferred.
*
* If the entry is prefetched, the free_icr routine will dispose of
* the flush dependency parents addresses array if necessary.
*/
if (!entry_ptr->prefetched) {
assert(0 == entry_ptr->fd_parent_count);
assert(NULL == entry_ptr->fd_parent_addrs);
} /* end if */

/* Check whether we should free the space in the file that
* the entry occupies
*/
if (free_file_space) {
hsize_t fsf_size;

/* Sanity checks */
assert(H5_addr_defined(entry_ptr->addr));
assert(!H5F_IS_TMP_ADDR(f, entry_ptr->addr));
#ifndef NDEBUG
{
size_t curr_len;

/* Get the actual image size for the thing again */
entry_ptr->type->image_len((void *)entry_ptr, &curr_len);
assert(curr_len == entry_ptr->size);
}
#endif

/* If the file space free size callback is defined, use
* it to get the size of the block of file space to free.
* Otherwise use entry_ptr->size.
*/
if (entry_ptr->type->fsf_size) {
if ((entry_ptr->type->fsf_size)((void *)entry_ptr, &fsf_size) < 0)
/* Note error but keep going */
HDONE_ERROR(H5E_CACHE, H5E_CANTFREE, FAIL, "unable to get file space free size");
Copy link
Member

Choose a reason for hiding this comment

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

We should make sure fsf_size is set to 0 if this fails, since it gets passed to H5MM_xfree. Either set it here or initialize it to 0.

Copy link
Member

Choose a reason for hiding this comment

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

Or simply skip calling H5MM_xfree if fsf_size fails

Copy link
Collaborator Author

@jhendersonHDF jhendersonHDF Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean the call to H5MF_xfree below? If so, I added the (ret_value >= 0) for that case. A little hacky, but figured it was best to skip.

} /* end if */
else /* no file space free size callback -- use entry size */
fsf_size = entry_ptr->size;

/* Release the space on disk */
if ((ret_value >= 0) && H5MF_xfree(f, entry_ptr->type->mem_type, entry_ptr->addr, fsf_size) < 0)
/* Note error but keep going */
HDONE_ERROR(H5E_CACHE, H5E_CANTFREE, FAIL, "unable to free file space for cache entry");
} /* end if ( free_file_space ) */

/* Reset the pointer to the cache the entry is within. -QAK */
entry_ptr->cache_ptr = NULL;

/* increment entries_removed_counter and set
* last_entry_removed_ptr. As we are likely about to
* free the entry, recall that last_entry_removed_ptr
* must NEVER be dereferenced.
*
* Recall that these fields are maintained to allow functions
* that perform scans of lists of entries to detect the
* unexpected removal of entries (via expunge, eviction,
* or take ownership at present), so that they can re-start
* their scans if necessary.
*
* Also check if the entry we are watching for removal is being
* removed (usually the 'next' entry for an iteration) and reset
* it to indicate that it was removed.
*/
cache_ptr->entries_removed_counter++;
cache_ptr->last_entry_removed_ptr = entry_ptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any chance cache_ptr->last_entry_removed_ptr is pointing to something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to say no, but I didn't validate any of the comments here.


if (entry_ptr == cache_ptr->entry_watched_for_removal)
cache_ptr->entry_watched_for_removal = NULL;

/* Check for actually destroying the entry in memory */
/* (As opposed to taking ownership of it) */
if (destroy_entry) {
if (entry_ptr->is_dirty) {
/* Reset dirty flag */
entry_ptr->is_dirty = false;

/* If the entry's type has a 'notify' callback send a
* 'entry cleaned' notice now that the entry is fully
* integrated into the cache.
*/
if (entry_ptr->type->notify &&
(entry_ptr->type->notify)(H5C_NOTIFY_ACTION_ENTRY_CLEANED, entry_ptr) < 0)
/* Note error but keep going */
HDONE_ERROR(H5E_CACHE, H5E_CANTNOTIFY, FAIL,
"can't notify client about entry dirty flag cleared");
} /* end if */

/* verify that the image has been freed */
assert(entry_ptr->image_ptr == NULL);

if (entry_ptr->type->free_icr((void *)entry_ptr) < 0)
/* Note error but keep going */
HDONE_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "free_icr callback failed");
} /* end if */

FUNC_LEAVE_NOAPI(ret_value)
} /* end H5C__discard_single_entry() */

/*-------------------------------------------------------------------------
* Function: H5C__verify_len_eoa
*
Expand Down
Loading