Skip to content
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

Jump to bottom

Fix divide-by-zero when page buf page size is 0 #4296

Merged
merged 2 commits into from
Apr 1, 2024
Merged

Fix divide-by-zero when page buf page size is 0 #4296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
367ea39
Fix divide-by-zero when page buf page size is 0
derobins Apr 1, 2024
5e2ea2b
Committing clang-format changes
github-actions[bot] Apr 1, 2024
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
9 changes: 9 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,15 @@ Bug Fixes since HDF5-1.14.0 release

Library
-------
- Fixed a divide-by-zero issue when a corrupt file sets the page size to 0

If a corrupt file sets the page buffer size in the superblock to zero,
the library could attempt to divide by zero when allocating space in
the file. The library now checks for valid page buffer sizes when
reading the superblock message.

Fixes oss-fuzz issue 58762

- Fixed a bug when using array datatypes with certain parent types

Array datatype conversion would never use a background buffer, even if the
Expand Down
7 changes: 5 additions & 2 deletions src/H5Fsuper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -800,8 +800,11 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, bool initial_read)
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, FAIL, "unable to set file space strategy");
} /* end if */

assert(f->shared->fs_page_size >= H5F_FILE_SPACE_PAGE_SIZE_MIN);
assert(fsinfo.page_size >= H5F_FILE_SPACE_PAGE_SIZE_MIN);
if (f->shared->fs_page_size < H5F_FILE_SPACE_PAGE_SIZE_MIN)
HGOTO_ERROR(H5E_FILE, H5E_BADVALUE, FAIL, "file space page size too small");
if (fsinfo.page_size < H5F_FILE_SPACE_PAGE_SIZE_MIN)
HGOTO_ERROR(H5E_FILE, H5E_BADVALUE, FAIL, "file space page size too small");

if (f->shared->fs_page_size != fsinfo.page_size) {
f->shared->fs_page_size = fsinfo.page_size;

Expand Down
6 changes: 4 additions & 2 deletions src/H5MFsection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,8 +606,10 @@ H5MF__sect_small_add(H5FS_section_info_t **_sect, unsigned *flags, void *_udata)
HGOTO_DONE(ret_value);

sect_end = (*sect)->sect_info.addr + (*sect)->sect_info.size;
rem = sect_end % udata->f->shared->fs_page_size;
prem = udata->f->shared->fs_page_size - rem;
if (0 == udata->f->shared->fs_page_size)
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 an assertion, since we already checked it when reading the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to not use asserts for error checking, aside from verifying input parameters, since a bunch of the CVE fixes have involved switching an assert out for real error checking (as here, in H5Fsuper.c). What if there were an overflow and something scribbled 0s on the struct member?

Copy link
Member

@fortnern fortnern Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's not practical to protect against those types of memory errors by checking for their effects. I feel strongly that asserts should be used for these sorts of internal consistency checks, otherwise we'd have to use error checks everywhere at a substantial performance penalty

Copy link
Member

Choose a reason for hiding this comment

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

Implementing a policy that internal consistency checks should be error checks and not asserts also creates an incentive to not add internal consistency checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed with @fortnern's points and I'd make the argument that asserts are exactly the appropriate tool to use when checking for something that may have overflowed. That's an internal consistency check for something that should not happen. Throwing an error in that case is nothing more than confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can switch it to an assert, but I'm a fan of "test what you ship" vs. "debug builds have the error checks". Also, our intuition for "should not happen" isn't always great, as we've had to switch asserts to real error checking to fix multiple issues, some of which were CVEs (again, as in this PR).

I also seriously doubt a couple of extra if statements are going to cause significant overhead. The vast majority of non-trivial statements in our library involve a bonus if check.

HGOTO_ERROR(H5E_RESOURCE, H5E_BADVALUE, FAIL, "page size of zero would result in division by zero");
rem = sect_end % udata->f->shared->fs_page_size;
prem = udata->f->shared->fs_page_size - rem;

/* Drop the section if it is at page end and its size is <= pgend threshold */
if (!rem && (*sect)->sect_info.size <= H5F_PGEND_META_THRES(udata->f) &&
Expand Down