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.

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

Fixes crashes when size_hint > UINT32_MAX is passed to H5Gcreate1 #611

Merged
merged 32 commits into from
Apr 30, 2021
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ba9deae
Committing clang-format changes
github-actions[bot] Mar 19, 2021
00f9750
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 21, 2021
e997283
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 22, 2021
3eac585
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 23, 2021
71643b7
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 24, 2021
d242911
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 24, 2021
bc70f95
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 26, 2021
3d7ad8e
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 26, 2021
aeb16b7
Merge branch 'develop' of https://github.com/derobins/hdf5 into develop
derobins Mar 26, 2021
765cb5b
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 27, 2021
18401fc
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 29, 2021
222242c
Merge branch 'develop' of https://github.com/derobins/hdf5 into develop
derobins Mar 29, 2021
8d0cafa
Merge remote-tracking branch 'canonical/develop' into develop
derobins Mar 30, 2021
d3694d1
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 2, 2021
1c7bcc2
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 15, 2021
70551e3
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 16, 2021
2dfc7db
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 20, 2021
71d2fbf
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 21, 2021
c8925f8
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 22, 2021
dd3e970
Merge branch 'develop' of https://github.com/derobins/hdf5 into develop
derobins Apr 22, 2021
bb61990
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 23, 2021
2c2b368
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 25, 2021
be2ada6
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 27, 2021
439f3a8
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 28, 2021
649aa9b
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 29, 2021
3fa627f
Merge remote-tracking branch 'canonical/develop' into develop
derobins Apr 29, 2021
7a6ee4e
Fixes incorrect size_hint handling in H5Gcreate1
derobins Apr 30, 2021
17fc91e
Updates the size hint type for group creation
derobins Apr 30, 2021
3df386a
Updates the RELEASE.txt note
derobins Apr 30, 2021
5f825f9
Revert "Updates the RELEASE.txt note"
derobins Apr 30, 2021
850e79e
Reverts previous behavior to use a uint32_t struct field
derobins Apr 30, 2021
a4227fb
Updates RELEASE.txt
derobins Apr 30, 2021
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
16 changes: 16 additions & 0 deletions release_docs/RELEASE.txt
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,22 @@ New Features

Library:
--------
- H5Gcreate1() now handles large size_hint parameters correctly

On 64-bit systems, large values of the size_t size_hint parameter
would be passed through a uint32_t struct field, causing a crash
on 64-bit Windows w/ MSVC and probably incorrect behavior or crashes
on other 64-bit systems.

The documentation for this API call was also incorrect, stating that
passing a negative value would cause the library to use a default
value. Instead, passing a "negative" value actually passes a very large
value, which is probably not what the user intends and can cause
crashes on 64-bit systems.

(DER - 2021/04/29, HDFFV-11241)


- H5Pset_fapl_log() no longer crashes when passed an invalid fapl ID

When passed an invalid fapl ID, H5Pset_fapl_log() would usually
Expand Down
9 changes: 4 additions & 5 deletions src/H5Gdeprec.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,9 @@ H5G_map_obj_type(H5O_type_t obj_type)
* specified NAME. The group is opened for write access
* and it's object ID is returned.
*
* The optional SIZE_HINT specifies how much file space to
* reserve to store the names that will appear in this
* group. If a non-positive value is supplied for the SIZE_HINT
* then a default size is chosen.
* The SIZE_HINT parameter specifies how much file space to reserve
* to store the names that will appear in this group. If zero is
* supplied for the SIZE_HINT then a default size is chosen.
*
* Note: Deprecated in favor of H5Gcreate2
*
Expand Down Expand Up @@ -197,7 +196,7 @@ H5Gcreate1(hid_t loc_id, const char *name, size_t size_hint)
HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, H5I_INVALID_HID, "can't get group info")

/* Set the non-default local heap size hint */
H5_CHECKED_ASSIGN(ginfo.lheap_size_hint, uint32_t, size_hint, size_t);
ginfo.lheap_size_hint = size_hint;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change here? size_t is probably larger than uint32_t and would be truncated if too large.

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 switched this because I updated the struct field to use size_t, but that's being switched back.

if (H5P_set(gc_plist, H5G_CRT_GROUP_INFO_NAME, &ginfo) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTSET, H5I_INVALID_HID, "can't set group info")
}
Expand Down
11 changes: 4 additions & 7 deletions src/H5Gpublic.h
Original file line number Diff line number Diff line change
Expand Up @@ -569,8 +569,8 @@ typedef struct H5G_stat_t {
*
* \fgdta_loc_id
* \param[in] name Name of the group to create
* \param[in] size_hint Optional parameter indicating the number of bytes
* to reserve for the names that will appear in the group
* \param[in] size_hint The number of bytes to reserve for the names
* that will appear in the group
*
* \return \hid_t{group}
*
Expand All @@ -592,11 +592,8 @@ typedef struct H5G_stat_t {
* group, is not limited.
*
* \p size_hint is a hint for the number of bytes to reserve to store
* the names which will be eventually added to the new group. Passing a
* value of zero for \p size_hint is usually adequate since the library
* is able to dynamically resize the name heap, but a correct hint may
* result in better performance. If a non-positive value is supplied
* for \p size_hint, then a default size is chosen.
* the names which will be eventually added to the new group. If this
* parameter is zero, a default value will be used.
*
* The return value is a group identifier for the open group. This
* group identifier should be closed by calling H5Gclose() when it is
Expand Down
2 changes: 1 addition & 1 deletion src/H5Oprivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ typedef struct H5O_bogus_t {
*/
typedef struct H5O_ginfo_t {
/* "Old" format group info (not stored) */
uint32_t lheap_size_hint; /* Local heap size hint */
size_t lheap_size_hint; /* Local heap size hint */

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 probably need to go look at the file format to make sure this value makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the file format specifies this as a 32-bit value, so allowing it to be a size_t is probably not a good direction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I'm switching it back.

/* "New" format group info (stored) */

Expand Down
4 changes: 4 additions & 0 deletions test/tmisc.c
Original file line number Diff line number Diff line change
Expand Up @@ -4091,6 +4091,10 @@ test_misc23(void)
tmp_id = H5Gcreate1(file_id, "/A/grp", (size_t)0);
CHECK(tmp_id, FAIL, "H5Gcreate1");

/* Check that the largest size hint is acceptable */
tmp_id = H5Gcreate1(file_id, "/enormous_size_hint", SIZE_MAX);
CHECK(tmp_id, FAIL, "H5Gcreate1");

status = H5Gclose(tmp_id);
CHECK(status, FAIL, "H5Gclose");

Expand Down