Take user block into account when returning chunk addresses #4236

derobins · 2024-03-25T09:47:30Z

Both H5Dchunk_iter() and H5Dget_chunk_info(_by_coord)() did not take the size of the user block into account when reporting addresses. Since the #1 use of these functions is to root around in the file for the raw data, this is kind of a problem.

Fixes GitHub issue #3003

Both H5Dchunk_iter() and H5Dget_chunk_info(_by_coord)() did not take the size of the user block into account when reporting addresses. Since the HDFGroup#1 use of these functions is to root around in the file for the raw data, this is kind of a problem. Fixes GitHub issue HDFGroup#3003

derobins · 2024-03-25T10:03:25Z

test/chunk_info.c

There's a lot of cleanup in this file. The new test and its helper function are at the end.

derobins · 2024-03-25T10:04:43Z

src/H5Dchunk.c

-                                (hsize_t)chunk_rec->nbytes, data->op_data)) < 0)
+    if ((ret_value =
+             (data->op)(offset, (unsigned)chunk_rec->filter_mask, data->base_addr + chunk_rec->chunk_addr,
+                        (hsize_t)chunk_rec->nbytes, data->op_data)) < 0)


H5Dchunk_iter() fix here

derobins · 2024-03-25T10:04:50Z

src/H5Dchunk.c

+        ud.op        = op;
+        ud.op_data   = op_data;
+        ud.chunk     = &dset->shared->layout.u.chunk;
+        ud.base_addr = H5F_BASE_ADDR(dset->oloc.file);


H5Dchunk_iter() fix here

derobins · 2024-03-25T10:05:16Z

src/H5Dchunk.c

@@ -8102,11 +8105,11 @@ H5D__get_chunk_info_by_coord(const H5D_t *dset, const hsize_t *offset, unsigned
            if (filter_mask)
                *filter_mask = udata.filter_mask;
            if (addr)
-                *addr = udata.chunk_addr;
+                *addr = udata.chunk_addr + H5F_BASE_ADDR(dset->oloc.file);


H5Dget_chunk_info_by_coord() fix

derobins · 2024-03-25T10:05:30Z

src/H5Dchunk.c

@@ -7967,14 +7968,14 @@ H5D__get_chunk_info(const H5D_t *dset, const H5S_t H5_ATTR_UNUSED *space, hsize_
            if (filter_mask)
                *filter_mask = udata.filter_mask;
            if (addr)
-                *addr = udata.chunk_addr;
+                *addr = udata.chunk_addr + H5F_BASE_ADDR(dset->oloc.file);


H5Dget_chunk_info() fix

jhendersonHDF · 2024-03-25T18:32:02Z

test/chunk_info.c

+    }
+
+    HDclose(fd);
+    HDclose(fd_ub);


Not a big deal, but reset FDs to -1 in case below closes fail and try to do the close again.

jhendersonHDF · 2024-03-25T18:32:33Z

test/chunk_info.c

+    free(addresses_ub);
+
+    HDremove(UBLOCK_FILE_NAME);
+    HDremove(NO_UBLOCK_FILE_NAME);


Can use H5Fdelete here for compatibility

fortnern · 2024-03-25T21:41:02Z

Are there other places in the code that do take the user block into account when returning addresses?

fortnern · 2024-03-25T21:49:01Z

Are there other places in the code that do take the user block into account when returning addresses?

It looks like H5Dget_offset does take the user block into account, but H5Oget_info3 does not, perhaps because it returns a "token" instead of an address

…#4236) Both H5Dchunk_iter() and H5Dget_chunk_info(_by_coord)() did not take the size of the user block into account when reporting addresses. Since the HDFGroup#1 use of these functions is to root around in the file for the raw data, this is kind of a problem. Fixes GitHub issue HDFGroup#3003

* Take user block into account when returning chunk addresses (#4236) Both H5Dchunk_iter() and H5Dget_chunk_info(_by_coord)() did not take the size of the user block into account when reporting addresses. Since the #1 use of these functions is to root around in the file for the raw data, this is kind of a problem. Fixes GitHub issue #3003 * Fix a minor warning in h5test.c (#4242) * Turn on -Werror for Java in GitHub -Werror workflows (#4243) * Update Windows CI to not install ninja (#4230) * Rework Fortran macros to use the proper code. (#4240) * Correct reference copy for 16 API (#4244) * Determine MPI LOGICAL during build, used in tests. (#4246) * Skip userblock test in chunk_info.c for multi-file VFDs (#4249) * Match generators with real cmake -G output on Windows (#4252) * Add Julia GitHub Actions. (#4123) * Re-revert to using autoreconf in autogen.sh (#4253) We previously tried removing the per-tool invocation of the Autotools and instead simply invoked autoreconf (PR #1906). This was reverted when it turned out that the NAG Fortran compiler had trouble with an undecorated -shared linker flag. It turns out that this is due to a bug in libtool 2.4.2 and earlier. Since this version of libtool is over a decade old, we're un-reverting the change. We've added a release note for anyone who has to build from source on elderly platforms. Fixes #1343 * Rewrite H5T__path_find_real for clarity (#4225) * Move conversion path free logic to helper function * Add tgz extensions on names (#4255) * Remove an error check regarding large cache objects (#4254) * Remove an error check regarding large cache objects In PR#4231 an assert() call was converted to a normal HDF5 error check. It turns out that the original assert() was added by a developer as a way of being alerted that large cache objects existed instead of as a guard against incorrect behavior, making it unnecessary in either debug or release builds. The error check has been removed. * Update RELEASE.txt * File format security issues (#4234) * Add job timeout to cygwin workflow (#4260) * Replace user-define with user-defined (#4261) * Improve the CMake clang -fsanitize=memory flags (#4267) -fsanitize=memory is almost useless without using -fsanitize-memory-track-origins=2 and we shoud probably add -fno-optimize-sibling-calls as well. * Add documentation (H5M) (#4259) * Add documentation (H5P) (#4262) * MPI type correction (#4268) * corrected type for MPI_*_f2c APIs * fixed return type of callback * reset compilation flags of logical test program * Clean up test/cmpd_dtransform.c (#4270) * Clean up test/cmpd_dtransform.c * Fix uninitialized memory warning from sanitizers * FAIL_STACK_ERROR --> TEST_ERROR * Emit output * Delete test file when done * Fix typo * H5Fdelete() --> remove() * Fix uninitialized memory issues in packet table (#4271) * replace deprecated CMAKE_COMPILER_IS_GNU** (#4272) * Prevent stack overflows in H5E__push_stack (#4264) * Minor fixes after merge of file format security fixes (#4263) * Update H5_IS_BUFFER_OVERFLOW to account for 'size' of 0 * Invert a few checks to avoid function call * CHECK --> CHECK_PTR in tmisc.c (#4274) * Add release note for CVE-2017-17507 (#4275) * Update Cygwin installation guide (#4265) * Addresses configuration fortran testing flags (#4276) * turn warnings to errors in fortran configure test * Intel fortran test fix * Merge julia workflows into standard ci format (#4273) * Fix range check in H5_addr_overlap (#4278) When the H5_addr_overlap macro was updated to use H5_RANGE_OVERLAP, it failed to take into account that H5_RANGE_OVERLAP expects the range to be inclusive. This lead to an assertion failure in H5MM_memcpy due to a memcpy operation on overlapping memory. This has been fixed by subtracting 1 from the calculated high bound values passed to H5_RANGE_OVERLAP * Fix potential buffer read overflows in H5PB_read (#4279) H5PB_read previously did not account for the fact that the size of the read it's performing could overflow the page buffer pointer, depending on the calculated offset for the read. This has been fixed by adjusting the size of the read if it's determined that it would overflow the page.

derobins requested review from lrknox, byrnHDF, fortnern, jhendersonHDF, qkoziol, vchoi-hdfgroup, bmribler, glennsong09, mattjala and brtnfld as code owners March 25, 2024 09:47

Fix spelling

a9b4cde

derobins linked an issue Mar 25, 2024 that may be closed by this pull request

H5Dchunk_iter() does not account for user block when reporting chunk file address #3003

Closed

derobins commented Mar 25, 2024

View reviewed changes

test/chunk_info.c

Copy link

Member Author

derobins Mar 25, 2024

Choose a reason for hiding this comment

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

There's a lot of cleanup in this file. The new test and its helper function are at the end.

derobins commented Mar 25, 2024

View reviewed changes

Add Doxygen notes

50f840e

jhendersonHDF reviewed Mar 25, 2024

View reviewed changes

jhendersonHDF approved these changes Mar 25, 2024

View reviewed changes

qkoziol approved these changes Mar 25, 2024

View reviewed changes

fortnern approved these changes Mar 25, 2024

View reviewed changes

Address comments from review

429c9c5

derobins merged commit fc46c83 into HDFGroup:develop Mar 26, 2024
52 checks passed

derobins deleted the chunk_address_mess branch March 27, 2024 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take user block into account when returning chunk addresses #4236

Take user block into account when returning chunk addresses #4236

derobins commented Mar 25, 2024

derobins Mar 25, 2024

derobins Mar 25, 2024

derobins Mar 25, 2024

derobins Mar 25, 2024

derobins Mar 25, 2024

jhendersonHDF Mar 25, 2024

jhendersonHDF Mar 25, 2024

fortnern commented Mar 25, 2024

fortnern commented Mar 25, 2024

Take user block into account when returning chunk addresses #4236

Take user block into account when returning chunk addresses #4236

Conversation

derobins commented Mar 25, 2024

derobins Mar 25, 2024

Choose a reason for hiding this comment

derobins Mar 25, 2024

Choose a reason for hiding this comment

derobins Mar 25, 2024

Choose a reason for hiding this comment

derobins Mar 25, 2024

Choose a reason for hiding this comment

derobins Mar 25, 2024

Choose a reason for hiding this comment

jhendersonHDF Mar 25, 2024

Choose a reason for hiding this comment

jhendersonHDF Mar 25, 2024

Choose a reason for hiding this comment

fortnern commented Mar 25, 2024

fortnern commented Mar 25, 2024