Skip to content

Conversation

jhendersonHDF
Copy link
Collaborator

@jhendersonHDF jhendersonHDF commented Sep 14, 2025

When discarding a metadata cache entry after flushing it, errors during the discard process could cause the library to skip calling the 'free_icr' callback for the entry. This could result in resource leaks and the inability of the cache to be fully flushed and closed due to issues such as pinned entries remaining in the cache. 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 CVE-2025-7068

Fixes #5578
Also fixes #4586


Important

Fixes resource leak issue in metadata cache discard process, addressing CVE-2025-7068 by ensuring proper cleanup in H5Centry.c.

  • Behavior:
    • Fixes issue in H5C__flush_single_entry() in H5Centry.c where errors during discard could skip free_icr callback, causing resource leaks.
    • Introduces H5C__discard_single_entry() to handle entry discard, ensuring cleanup even on errors.
  • Security:
    • Fixes CVE-2025-7068, preventing resource leaks and ensuring cache can be fully flushed and closed.
  • Documentation:

This description was created by Ellipsis for 749a292. You can customize this summary. It will automatically update as commits are pushed.

@jhendersonHDF jhendersonHDF added the Component - C Library Core C library issues (usually in the src directory) label Sep 14, 2025
@github-project-automation github-project-automation bot moved this to To be triaged in HDF5 - TRIAGE & TRACK Sep 14, 2025
@jhendersonHDF
Copy link
Collaborator Author

jhendersonHDF commented Sep 14, 2025

Context:
When opening a corrupted HDF5 file with read-write access, the process of reading the file superblock checks if a message exists inside the superblock extension object header using H5O_msg_exists, which causes H5O_protect to load an object header continuation chunk into the cache and deserialize it, increasing the reference count on the superblock extension object header and pinning it in the cache as part of H5O__inc_rc. A bit later during the file superblock reading process, the library finds a message in the superblock extension indicating that there is a metadata cache image to load and calls H5AC_load_cache_image_on_next_protect to have the metadata cache load it on the next H5C_protect call. When attempting to open a dataset in the corrupted file, the library eventually tries to load the cache image as part of H5G__obj_lookup, but fails. During the process of trying to load the cache image, the library is unable to delete the cache image by removing the cache image message from the superblock extension due to corruption in the file free space manager. When the metadata cache fails to free the file space for the relevant object header chunk, it throws an error and skips over calling the free_icr callback for the chunk, eventually leaving the superblock extension object header pinned in the cache with an increased reference count. When the file is closed later on, the superblock extension object header can't be flushed, leaking the memory for the chunk proxy structure allocated for the continuation chunk during H5O_protect.

mattjala
mattjala previously approved these changes Sep 16, 2025
mattjala
mattjala previously approved these changes Sep 17, 2025
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

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.

Copy link
Member

@fortnern fortnern left a comment

Choose a reason for hiding this comment

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

See comment but looks good otherwise

@github-project-automation github-project-automation bot moved this from Scheduled/On-Deck to In progress in HDF5 - TRIAGE & TRACK Sep 23, 2025
@nbagha1 nbagha1 added this to the Release 2.0.0 milestone Sep 26, 2025
mattjala
mattjala previously approved these changes Sep 26, 2025
* 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.

Copy link
Collaborator

@bmribler bmribler left a comment

Choose a reason for hiding this comment

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

See my other comments

@brtnfld
Copy link
Collaborator

brtnfld commented Oct 8, 2025

@fortnern @bmribler @mattjala, can we get this approved as soon as possible, as it should resolve other CVE issues? If you can not approve it, please be clear as to why.

bmribler
bmribler previously approved these changes Oct 9, 2025
…ntries

When discarding a metadata cache entry after flushing it, errors
during the discard process could cause the library to skip calling
the 'free_icr' callback for the entry. This could result in resource
leaks and the inability of the cache to be fully flushed and closed
due to issues such as pinned entries remaining in the cache. 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 CVE-2025-7068
@bmribler
Copy link
Collaborator

@fortnern @mattjala Could you please re-review and re-approve this when you get the chance?

@lrknox lrknox merged commit 7dd1102 into HDFGroup:develop Oct 16, 2025
90 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in HDF5 - TRIAGE & TRACK Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component - C Library Core C library issues (usually in the src directory)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG] Memory leaks in H5FL__malloc at src/H5FL.c:211:30 A memory leak was found while testing h5_extended_fuzzer with libfuzzer

8 participants