Skip to content

Fix several issues uncovered by CVE-2025-7067 #5938

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
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix several issues uncovered by CVE-2025-7067 #5938

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
2 changes: 1 addition & 1 deletion src/H5FScache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,7 @@ H5FS__cache_sinfo_deserialize(const void *_image, size_t H5_ATTR_NDEBUG_UNUSED l

/* Insert section in free space manager, unless requested not to */
if (!(des_flags & H5FS_DESERIALIZE_NO_ADD))
if (H5FS_sect_add(udata->f, fspace, new_sect, H5FS_ADD_DESERIALIZING, udata) < 0)
if (H5FS_sect_add(udata->f, fspace, new_sect, H5FS_ADD_DESERIALIZING, udata, NULL) < 0)
HGOTO_ERROR(H5E_FSPACE, H5E_CANTINSERT, NULL,
"can't add section to free space manager");
} /* end for */
Expand Down
2 changes: 1 addition & 1 deletion src/H5FSprivate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ H5_DLL herr_t H5FS_free(H5F_t *f, H5FS_t *fspace, bool free_file_space);

/* Free space section routines */
H5_DLL herr_t H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *node, unsigned flags,
void *op_data);
void *op_data, bool *merged_or_shrunk);
H5_DLL htri_t H5FS_sect_try_merge(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flags,
void *op_data);
H5_DLL htri_t H5FS_sect_try_extend(H5F_t *f, H5FS_t *fspace, haddr_t addr, hsize_t size,
Expand Down
81 changes: 74 additions & 7 deletions src/H5FSsection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,7 +1297,8 @@ H5FS__sect_merge(H5FS_t *fspace, H5FS_section_info_t **sect, void *op_data)
*-------------------------------------------------------------------------
*/
herr_t
H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flags, void *op_data)
H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flags, void *op_data,
bool *merged_or_shrunk)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Callers of this function need to know whether the free space section passed in was merged or shrunk away, even on failure. Otherwise, calling code can incorrectly assume that it needs to free the passed in section if it was allocated by the calling code and H5FS_sect_add() failed. In some cases, the passed in section will have already been freed, eventually resulting in a double free situation when the calling code frees the section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An alternative approach would be to pass in a H5FS_section_info_t **, but that seemed uglier and prone to the caller accidentally losing a pointer.

{
H5FS_section_class_t *cls; /* Section's class */
bool sinfo_valid = false; /* Whether the section info is valid */
Expand All @@ -1318,6 +1319,9 @@ H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flag
assert(H5_addr_defined(sect->addr));
assert(sect->size);

if (merged_or_shrunk)
*merged_or_shrunk = false;

/* Get a pointer to the section info */
if (H5FS__sinfo_lock(f, fspace, H5AC__NO_FLAGS_SET) < 0)
HGOTO_ERROR(H5E_FSPACE, H5E_CANTGET, FAIL, "can't get section info");
Expand All @@ -1344,9 +1348,12 @@ H5FS_sect_add(H5F_t *f, H5FS_t *fspace, H5FS_section_info_t *sect, unsigned flag
/* (If section has been completely merged or shrunk away, 'sect' will
* be NULL at this point - QAK)
*/
if (sect)
if (sect) {
if (H5FS__sect_link(fspace, sect, flags) < 0)
HGOTO_ERROR(H5E_FSPACE, H5E_CANTINSERT, FAIL, "can't insert free space section into skip list");
}
else if (merged_or_shrunk)
*merged_or_shrunk = true;

#ifdef H5FS_SINFO_DEBUG
fprintf(stderr, "%s: fspace->tot_space = %" PRIuHSIZE "\n", __func__, fspace->tot_space);
Expand Down Expand Up @@ -2314,11 +2321,15 @@ H5FS_sect_try_shrink_eoa(H5F_t *f, H5FS_t *fspace, void *op_data)
herr_t
H5FS_vfd_alloc_hdr_and_section_info_if_needed(H5F_t *f, H5FS_t *fspace, haddr_t *fs_addr_ptr)
{
hsize_t hdr_alloc_size;
hsize_t sinfo_alloc_size;
haddr_t sect_addr = HADDR_UNDEF; /* address of sinfo */
haddr_t eoa = HADDR_UNDEF; /* Initial EOA for the file */
herr_t ret_value = SUCCEED; /* Return value */
hsize_t hdr_alloc_size = 0;
hsize_t sinfo_alloc_size = 0;
haddr_t sect_addr = HADDR_UNDEF; /* address of sinfo */
haddr_t eoa = HADDR_UNDEF; /* Initial EOA for the file */
bool allocated_header = false; /* Whether a free space header was allocated */
bool inserted_header = false; /* Whether a free space header was inserted into the metadata cache */
bool allocated_section = false; /* Whether a free space section was allocated */
bool inserted_section = false; /* Whether a free space section was inserted into the metadata cache */
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_NOAPI_NOINIT

Expand Down Expand Up @@ -2367,10 +2378,12 @@ H5FS_vfd_alloc_hdr_and_section_info_if_needed(H5F_t *f, H5FS_t *fspace, haddr_t
/* Allocate space for the free space header */
if (HADDR_UNDEF == (fspace->addr = H5MF_alloc(f, H5FD_MEM_FSPACE_HDR, hdr_alloc_size)))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "file allocation failed for free space header");
allocated_header = true;

/* Cache the new free space header (pinned) */
if (H5AC_insert_entry(f, H5AC_FSPACE_HDR, fspace->addr, fspace, H5AC__PIN_ENTRY_FLAG) < 0)
HGOTO_ERROR(H5E_FSPACE, H5E_CANTINIT, FAIL, "can't add free space header to cache");
inserted_header = true;

*fs_addr_ptr = fspace->addr;
}
Expand All @@ -2396,6 +2409,7 @@ H5FS_vfd_alloc_hdr_and_section_info_if_needed(H5F_t *f, H5FS_t *fspace, haddr_t
/* allocate space for the section info */
if (HADDR_UNDEF == (sect_addr = H5MF_alloc(f, H5FD_MEM_FSPACE_SINFO, sinfo_alloc_size)))
HGOTO_ERROR(H5E_FSPACE, H5E_NOSPACE, FAIL, "file allocation failed for section info");
allocated_section = true;

/* update fspace->alloc_sect_size and fspace->sect_addr to reflect
* the allocation
Expand Down Expand Up @@ -2437,6 +2451,7 @@ H5FS_vfd_alloc_hdr_and_section_info_if_needed(H5F_t *f, H5FS_t *fspace, haddr_t
*/
if (H5AC_insert_entry(f, H5AC_FSPACE_SINFO, sect_addr, fspace->sinfo, H5AC__NO_FLAGS_SET) < 0)
HGOTO_ERROR(H5E_FSPACE, H5E_CANTINIT, FAIL, "can't add free space sinfo to cache");
inserted_section = true;

/* We have changed the sinfo address -- Mark free space header dirty */
if (H5AC_mark_entry_dirty(fspace) < 0)
Expand All @@ -2453,5 +2468,57 @@ H5FS_vfd_alloc_hdr_and_section_info_if_needed(H5F_t *f, H5FS_t *fspace, haddr_t
} /* end if */

done:
if (ret_value < 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When this function fails, remove everything that was previously inserted into the metadata cache. Otherwise, crashes or assertion failures can occur later on when trying to flush out half-initialized entries that were inserted.

/* Remove the free space section that was inserted into the metadata cache,
* making sure to free the file space that was allocated for it as well.
* Avoid expunging the entry, as the information needs to be kept around
* until we finish trying to settle the metadata free space manager(s).
*/
if (inserted_section || allocated_section) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this line?

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 believe these checks are actually redundant and can be removed at this point. Previously I was removing the cache entries with expunge_entry and the "free file space" flag, in which case I had two branches with different logic where I didn't want to double free the file space with the call to H5MF_xfree followed by expunge_entry. That caused some problems and needed a switch to remove_entry instead, so I think this line and the other line are no longer relevant.

if (allocated_section && (sect_addr == fspace->sect_addr)) {
assert(H5_addr_defined(fspace->sect_addr));

if (H5MF_xfree(f, H5FD_MEM_FSPACE_SINFO, sect_addr, sinfo_alloc_size) < 0)
HDONE_ERROR(H5E_FSPACE, H5E_CANTFREE, FAIL, "unable to free free space sections");
fspace->sect_addr = HADDR_UNDEF;
}

if (inserted_section) {
if (H5AC_remove_entry(fspace->sinfo) < 0)
HDONE_ERROR(H5E_FSPACE, H5E_CANTEXPUNGE, FAIL,
"can't remove file free space section from cache");
}
}

/* Remove the free space header that was inserted into the metadata cache,
* making sure to free the file space that was allocated for it as well.
* Avoid expunging the entry, as the information needs to be kept around
* until we finish trying to settle the metadata free space manager(s).
*/
if (inserted_header || allocated_header) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, is this one just for the assertion?

assert(H5_addr_defined(fspace->addr));

if (allocated_header) {
/* Free file space before removing entry from cache, as freeing the
* file space may depend on a valid cache pointer.
*/
if (H5MF_xfree(f, H5FD_MEM_FSPACE_HDR, fspace->addr, hdr_alloc_size) < 0)
HDONE_ERROR(H5E_FSPACE, H5E_CANTFREE, FAIL, "unable to free file free space header");
fspace->addr = HADDR_UNDEF;
}

if (inserted_header) {
if (H5AC_mark_entry_clean(fspace) < 0)
HDONE_ERROR(H5E_FSPACE, H5E_CANTMARKCLEAN, FAIL,
"can't mark file free space header as clean");
if (H5AC_unpin_entry(fspace) < 0)
HDONE_ERROR(H5E_FSPACE, H5E_CANTUNPIN, FAIL, "can't unpin file free space header");
if (H5AC_remove_entry(fspace) < 0)
HDONE_ERROR(H5E_FSPACE, H5E_CANTEXPUNGE, FAIL,
"can't remove file free space header from cache");
}
}
}

FUNC_LEAVE_NOAPI(ret_value)
} /* H5FS_vfd_alloc_hdr_and_section_info_if_needed() */
2 changes: 1 addition & 1 deletion src/H5HFspace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ H5HF__space_add(H5HF_hdr_t *hdr, H5HF_free_section_t *node, unsigned flags)
udata.hdr = hdr;

/* Add to the free space for the heap */
if (H5FS_sect_add(hdr->f, hdr->fspace, (H5FS_section_info_t *)node, flags, &udata) < 0)
if (H5FS_sect_add(hdr->f, hdr->fspace, (H5FS_section_info_t *)node, flags, &udata, NULL) < 0)
HGOTO_ERROR(H5E_HEAP, H5E_CANTINSERT, FAIL, "can't add section to heap free space");

done:
Expand Down
58 changes: 44 additions & 14 deletions src/H5MF.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,8 @@ H5MF__close_fstype(H5F_t *f, H5F_mem_page_t type)
*-------------------------------------------------------------------------
*/
herr_t
H5MF__add_sect(H5F_t *f, H5FD_mem_t alloc_type, H5FS_t *fspace, H5MF_free_section_t *node)
H5MF__add_sect(H5F_t *f, H5FD_mem_t alloc_type, H5FS_t *fspace, H5MF_free_section_t *node,
bool *merged_or_shrunk)
{
H5AC_ring_t orig_ring = H5AC_RING_INV; /* Original ring value */
H5AC_ring_t fsm_ring = H5AC_RING_INV; /* Ring of FSM */
Expand Down Expand Up @@ -631,7 +632,8 @@ H5MF__add_sect(H5F_t *f, H5FD_mem_t alloc_type, H5FS_t *fspace, H5MF_free_sectio
__func__, node->sect_info.addr, node->sect_info.size);
#endif /* H5MF_ALLOC_DEBUG_MORE */
/* Add the section */
if (H5FS_sect_add(f, fspace, (H5FS_section_info_t *)node, H5FS_ADD_RETURNED_SPACE, &udata) < 0)
if (H5FS_sect_add(f, fspace, (H5FS_section_info_t *)node, H5FS_ADD_RETURNED_SPACE, &udata,
merged_or_shrunk) < 0)
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINSERT, FAIL, "can't re-add section to file free space");

done:
Expand Down Expand Up @@ -711,8 +713,11 @@ H5MF__find_sect(H5F_t *f, H5FD_mem_t alloc_type, hsize_t size, H5FS_t *fspace, h
#endif /* H5MF_ALLOC_DEBUG_MORE */

/* Re-add the section to the free-space manager */
if (H5MF__add_sect(f, alloc_type, fspace, node) < 0)
if (H5MF__add_sect(f, alloc_type, fspace, node, NULL) < 0) {
node->sect_info.addr -= size;
node->sect_info.size += size;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverse the modifications made above on failure.

HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINSERT, FAIL, "can't re-add section to file free space");
}
} /* end else */
} /* end if */

Expand Down Expand Up @@ -852,9 +857,10 @@ H5MF_alloc(H5F_t *f, H5FD_mem_t alloc_type, hsize_t size)
static haddr_t
H5MF__alloc_pagefs(H5F_t *f, H5FD_mem_t alloc_type, hsize_t size)
{
H5F_mem_page_t ptype; /* Free-space manager type */
H5MF_free_section_t *node = NULL; /* Free space section pointer */
haddr_t ret_value = HADDR_UNDEF; /* Return value */
H5F_mem_page_t ptype; /* Free-space manager type */
H5MF_free_section_t *node = NULL; /* Free space section pointer */
bool section_merged_or_shrunk = false; /* Whether free space section was merged or shrunk away */
haddr_t ret_value = HADDR_UNDEF; /* Return value */

FUNC_ENTER_PACKAGE

Expand Down Expand Up @@ -900,9 +906,13 @@ H5MF__alloc_pagefs(H5F_t *f, H5FD_mem_t alloc_type, hsize_t size)
"can't initialize free space section");

/* Add the fragment to the large free-space manager */
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[ptype], node) < 0)
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[ptype], node, &section_merged_or_shrunk) <
0) {
if (section_merged_or_shrunk)
node = NULL;
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINSERT, HADDR_UNDEF,
"can't re-add section to file free space");
}

node = NULL;
} /* end if */
Expand Down Expand Up @@ -931,9 +941,13 @@ H5MF__alloc_pagefs(H5F_t *f, H5FD_mem_t alloc_type, hsize_t size)
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINIT, HADDR_UNDEF, "can't initialize free space section");

/* Add the remaining space in the page to the manager */
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[ptype], node) < 0)
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[ptype], node, &section_merged_or_shrunk) <
0) {
if (section_merged_or_shrunk)
node = NULL;
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINSERT, HADDR_UNDEF,
"can't re-add section to file free space");
}

node = NULL;

Expand Down Expand Up @@ -1154,15 +1168,21 @@ H5MF_xfree(H5F_t *f, H5FD_mem_t alloc_type, haddr_t addr, hsize_t size)

/* If size of the freed section is larger than threshold, add it to the free space manager */
if (size >= f->shared->fs_threshold) {
bool section_merged_or_shrunk = false; /* Whether free space section was merged or shrunk away */

assert(f->shared->fs_man[fs_type]);

#ifdef H5MF_ALLOC_DEBUG_MORE
fprintf(stderr, "%s: Before H5FS_sect_add()\n", __func__);
#endif /* H5MF_ALLOC_DEBUG_MORE */

/* Add to the free space for the file */
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[fs_type], node) < 0)
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[fs_type], node, &section_merged_or_shrunk) < 0) {
if (section_merged_or_shrunk)
node = NULL;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Avoid double-freeing this section that was allocated above in case H5MF__add_sect() already freed it.

HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINSERT, FAIL, "can't add section to file free space");
}

node = NULL;

#ifdef H5MF_ALLOC_DEBUG_MORE
Expand Down Expand Up @@ -1316,7 +1336,7 @@ H5MF_try_extend(H5F_t *f, H5FD_mem_t alloc_type, haddr_t addr, hsize_t size, hsi
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINIT, FAIL, "can't initialize free space section");

/* Add the fragment to the large-sized free-space manager */
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[fs_type], node) < 0)
if (H5MF__add_sect(f, alloc_type, f->shared->fs_man[fs_type], node, NULL) < 0)
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTINSERT, FAIL, "can't re-add section to file free space");

node = NULL;
Expand Down Expand Up @@ -3059,8 +3079,13 @@ H5MF_settle_meta_data_fsm(H5F_t *f, bool *fsm_settled)
assert(sm_fssinfo_fs_type > H5F_MEM_PAGE_DEFAULT);
assert(sm_fssinfo_fs_type < H5F_MEM_PAGE_LARGE_SUPER);

assert(!H5_addr_defined(f->shared->fs_addr[sm_fshdr_fs_type]));
assert(!H5_addr_defined(f->shared->fs_addr[sm_fssinfo_fs_type]));
if (H5_addr_defined(f->shared->fs_addr[sm_fshdr_fs_type]))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These assertions can be triggered by a file space info message in the file's superblock extension that was specifically crafted. Throwing an error here causes the library to fail to be able to settle the MDFSM, which eventually leads to issues closing the library and leaked memory due to the metadata cache not being able to be flushed and destroyed. In Debug builds this still results in an assertion failure when specifically-crafted files are opened, but addressing that will require significant changes to the metadata cache shutdown process.

An alternative is to simply wipe out these addresses to HADDR_UNDEF and let the following logic take over, but this could end up dropping free space on the floor and also causes far more problems when given a specifically crafted file.

Copy link
Member

Choose a reason for hiding this comment

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

Could this be detected when the file space info message is decoded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In theory yes, however there was no obvious documentation or information I could find on which of the free space managers are and aren't allowed to have a defined address right after decoding and after asking around the opinion seemed to be that any of the managers could be defined. In this specific case however it seems the assumption is that these particular ones won't have a defined address.

HGOTO_ERROR(H5E_FSPACE, H5E_BADVALUE, FAIL,
"small free space header block manager should not have had file space allocated");
if (H5_addr_defined(f->shared->fs_addr[sm_fssinfo_fs_type]))
HGOTO_ERROR(
H5E_FSPACE, H5E_BADVALUE, FAIL,
"small free space serialized section manager should not have had file space allocated");

/* Note that in most cases, sm_hdr_fspace will equal sm_sinfo_fspace. */
sm_hdr_fspace = f->shared->fs_man[sm_fshdr_fs_type];
Expand All @@ -3078,8 +3103,13 @@ H5MF_settle_meta_data_fsm(H5F_t *f, bool *fsm_settled)
assert(lg_fssinfo_fs_type >= H5F_MEM_PAGE_LARGE_SUPER);
assert(lg_fssinfo_fs_type < H5F_MEM_PAGE_NTYPES);

assert(!H5_addr_defined(f->shared->fs_addr[lg_fshdr_fs_type]));
assert(!H5_addr_defined(f->shared->fs_addr[lg_fssinfo_fs_type]));
if (H5_addr_defined(f->shared->fs_addr[lg_fshdr_fs_type]))
HGOTO_ERROR(H5E_FSPACE, H5E_BADVALUE, FAIL,
"large free space header block manager should not have had file space allocated");
if (H5_addr_defined(f->shared->fs_addr[lg_fssinfo_fs_type]))
HGOTO_ERROR(
H5E_FSPACE, H5E_BADVALUE, FAIL,
"large free space serialized section manager should not have had file space allocated");

/* Note that in most cases, lg_hdr_fspace will equal lg_sinfo_fspace. */
lg_hdr_fspace = f->shared->fs_man[lg_fshdr_fs_type];
Expand Down
3 changes: 2 additions & 1 deletion src/H5MFpkg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ H5_DLLVAR const H5FS_section_class_t H5MF_FSPACE_SECT_CLS_LARGE[1];
H5_DLL herr_t H5MF__open_fstype(H5F_t *f, H5F_mem_page_t type);
H5_DLL herr_t H5MF__start_fstype(H5F_t *f, H5F_mem_page_t type);
H5_DLL htri_t H5MF__find_sect(H5F_t *f, H5FD_mem_t alloc_type, hsize_t size, H5FS_t *fspace, haddr_t *addr);
H5_DLL herr_t H5MF__add_sect(H5F_t *f, H5FD_mem_t alloc_type, H5FS_t *fspace, H5MF_free_section_t *node);
H5_DLL herr_t H5MF__add_sect(H5F_t *f, H5FD_mem_t alloc_type, H5FS_t *fspace, H5MF_free_section_t *node,
bool *merged_or_shrunk);
H5_DLL void H5MF__alloc_to_fs_type(H5F_shared_t *f_sh, H5FD_mem_t alloc_type, hsize_t size,
H5F_mem_page_t *fs_type);

Expand Down
3 changes: 0 additions & 3 deletions src/H5MFsection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,6 @@ H5MF__sect_simple_can_merge(const H5FS_section_info_t *_sect1, const H5FS_sectio
assert(sect1);
assert(sect2);
assert(sect1->sect_info.type == sect2->sect_info.type); /* Checks "MERGE_SYM" flag */
assert(H5_addr_lt(sect1->sect_info.addr, sect2->sect_info.addr));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the file given in #5577, an attempt is made to place an overlapping section of file free space into the free space manager. That eventually causes this assertion to trigger. While a true fix would likely involve detecting this situation earlier on, this is probably a non-trivial effort. Since these functions are only checking whether two free space sections can be merged and not actually doing the merging, removing these assertions and just letting them return false seemed appropriate.

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 leave open an issue to add the correct fix, and note that these assertions should be re enabled at that point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do. Should I instead comment these out for now then?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good


/* Check if second section adjoins first section */
ret_value = H5_addr_eq(sect1->sect_info.addr + sect1->sect_info.size, sect2->sect_info.addr);
Expand Down Expand Up @@ -663,7 +662,6 @@ H5MF__sect_small_can_merge(const H5FS_section_info_t *_sect1, const H5FS_section
assert(sect1);
assert(sect2);
assert(sect1->sect_info.type == sect2->sect_info.type); /* Checks "MERGE_SYM" flag */
assert(H5_addr_lt(sect1->sect_info.addr, sect2->sect_info.addr));

/* Check if second section adjoins first section */
ret_value = H5_addr_eq(sect1->sect_info.addr + sect1->sect_info.size, sect2->sect_info.addr);
Expand Down Expand Up @@ -769,7 +767,6 @@ H5MF__sect_large_can_merge(const H5FS_section_info_t *_sect1, const H5FS_section
assert(sect1);
assert(sect2);
assert(sect1->sect_info.type == sect2->sect_info.type); /* Checks "MERGE_SYM" flag */
assert(H5_addr_lt(sect1->sect_info.addr, sect2->sect_info.addr));

ret_value = H5_addr_eq(sect1->sect_info.addr + sect1->sect_info.size, sect2->sect_info.addr);

Expand Down
Loading
Loading