From 5ac5105b34fab3acace9aa810a618da995354c8a Mon Sep 17 00:00:00 2001 From: Matt L <124107509+mattjala@users.noreply.github.com> Date: Thu, 26 Sep 2024 14:02:55 -0500 Subject: [PATCH 1/5] RELEASE note for threadsafety warning (#4883) --- release_docs/RELEASE.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 117f923e7e5..969550cddb5 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -818,7 +818,15 @@ New Features Documentation: -------------- - - + - Documented that leaving HDF5 threads running at termination is unsafe + + Added doc/threadsafety-warning.md as a warning that threads which use HDF5 + resources must be closed before either process exit or library close. + If HDF5 threads are alive during either of these operations, their resources + will not be cleaned up properly and undefined behavior is possible. + + This document also includes a discussion on potential ways to mitigate this issue. + Support for new platforms, languages and compilers From c2baceab9aa5f032d90bb7b99bcfe311dbaaf9a0 Mon Sep 17 00:00:00 2001 From: bmribler <39579120+bmribler@users.noreply.github.com> Date: Thu, 26 Sep 2024 17:05:50 -0400 Subject: [PATCH 2/5] Fixed a memory leak from H5FL_blk_malloc (#4882) In H5F__accum_reset(), when H5F__accum_flush() failed, the freeing of f_sh->accum.buf was never reached, causing resource leak. @fortnern added the third argument to H5F__accum_reset() so we can free f_sh->accum.buf when we close the file, that is, when H5F__accum_reset() is called from the H5F__dest() route, and can leave the accumulator in place otherwise. --- src/H5Faccum.c | 13 ++++++++----- src/H5Fint.c | 4 ++-- src/H5Fio.c | 2 +- src/H5Fpkg.h | 2 +- test/accum.c | 2 +- 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/H5Faccum.c b/src/H5Faccum.c index 9c4c8cdbbda..5fabf5266a0 100644 --- a/src/H5Faccum.c +++ b/src/H5Faccum.c @@ -725,7 +725,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s /* Make certain that data in accumulator is visible before new write */ if ((H5F_SHARED_INTENT(f_sh) & H5F_ACC_SWMR_WRITE) > 0) /* Flush if dirty and reset accumulator */ - if (H5F__accum_reset(f_sh, true) < 0) + if (H5F__accum_reset(f_sh, true, false) < 0) HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator"); /* Write the data */ @@ -776,7 +776,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s } /* end if */ else { /* Access covers whole accumulator */ /* Reset accumulator, but don't flush */ - if (H5F__accum_reset(f_sh, false) < 0) + if (H5F__accum_reset(f_sh, false, false) < 0) HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator"); } /* end else */ } /* end if */ @@ -1039,7 +1039,7 @@ H5F__accum_flush(H5F_shared_t *f_sh) *------------------------------------------------------------------------- */ herr_t -H5F__accum_reset(H5F_shared_t *f_sh, bool flush) +H5F__accum_reset(H5F_shared_t *f_sh, bool flush, bool force) { herr_t ret_value = SUCCEED; /* Return value */ @@ -1050,8 +1050,11 @@ H5F__accum_reset(H5F_shared_t *f_sh, bool flush) /* Flush any dirty data in accumulator, if requested */ if (flush) - if (H5F__accum_flush(f_sh) < 0) - HGOTO_ERROR(H5E_FILE, H5E_CANTFLUSH, FAIL, "can't flush metadata accumulator"); + if (H5F__accum_flush(f_sh) < 0) { + HDONE_ERROR(H5E_FILE, H5E_CANTFLUSH, FAIL, "can't flush metadata accumulator"); + if (!force) + HGOTO_DONE(FAIL); + } /* Check if we need to reset the metadata accumulator information */ if (f_sh->feature_flags & H5FD_FEAT_ACCUMULATE_METADATA) { diff --git a/src/H5Fint.c b/src/H5Fint.c index f653e0b71f0..3a9c65f1783 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1559,7 +1559,7 @@ H5F__dest(H5F_t *f, bool flush, bool free_on_failure) } /* end if */ /* Destroy other components of the file */ - if (H5F__accum_reset(f->shared, true) < 0) + if (H5F__accum_reset(f->shared, true, true) < 0) /* Push error, but keep going*/ HDONE_ERROR(H5E_FILE, H5E_CANTRELEASE, FAIL, "problems closing file"); if (H5FO_dest(f) < 0) @@ -3893,7 +3893,7 @@ H5F__start_swmr_write(H5F_t *f) } /* end if */ /* Flush and reset the accumulator */ - if (H5F__accum_reset(f->shared, true) < 0) + if (H5F__accum_reset(f->shared, true, false) < 0) HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator"); /* Turn on SWMR write in shared file open flags */ diff --git a/src/H5Fio.c b/src/H5Fio.c index 2cd8a53ba53..3d50b50fc51 100644 --- a/src/H5Fio.c +++ b/src/H5Fio.c @@ -422,7 +422,7 @@ H5F_flush_tagged_metadata(H5F_t *f, haddr_t tag) HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to flush tagged metadata"); /* Flush and reset the accumulator */ - if (H5F__accum_reset(f->shared, true) < 0) + if (H5F__accum_reset(f->shared, true, false) < 0) HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator"); /* Flush file buffers to disk. */ diff --git a/src/H5Fpkg.h b/src/H5Fpkg.h index f60841ec55f..7acd243aa82 100644 --- a/src/H5Fpkg.h +++ b/src/H5Fpkg.h @@ -445,7 +445,7 @@ H5_DLL herr_t H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t type, haddr_t addr const void *buf); H5_DLL herr_t H5F__accum_free(H5F_shared_t *f, H5FD_mem_t type, haddr_t addr, hsize_t size); H5_DLL herr_t H5F__accum_flush(H5F_shared_t *f_sh); -H5_DLL herr_t H5F__accum_reset(H5F_shared_t *f_sh, bool flush); +H5_DLL herr_t H5F__accum_reset(H5F_shared_t *f_sh, bool flush, bool force); /* Shared file list related routines */ H5_DLL herr_t H5F__sfile_add(H5F_shared_t *shared); diff --git a/test/accum.c b/test/accum.c index 62d1c9fd88a..308b64ad32c 100644 --- a/test/accum.c +++ b/test/accum.c @@ -61,7 +61,7 @@ void accum_printf(const H5F_t *f); #define accum_read(a, s, b) H5F_block_read(f, H5FD_MEM_DEFAULT, (haddr_t)(a), (size_t)(s), (b)) #define accum_free(f, a, s) H5F__accum_free(f->shared, H5FD_MEM_DEFAULT, (haddr_t)(a), (hsize_t)(s)) #define accum_flush(f) H5F__accum_flush(f->shared) -#define accum_reset(f) H5F__accum_reset(f->shared, true) +#define accum_reset(f) H5F__accum_reset(f->shared, true, false) /* ================= */ /* Main Test Routine */ From 1fe032d3e03405391d07cc1d1436f8a26f6703f8 Mon Sep 17 00:00:00 2001 From: "H. Joe Lee" Date: Fri, 27 Sep 2024 07:30:27 -0500 Subject: [PATCH 3/5] Improve grammar and style of comment block in bin/genparser (#4879) --- bin/genparser | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bin/genparser b/bin/genparser index 619dbfaa3ed..3e113739447 100755 --- a/bin/genparser +++ b/bin/genparser @@ -200,14 +200,14 @@ if [ "$verbose" = true ] ; then fi ${HDF5_FLEX} --nounistd -PH5LTyy -o ${path_to_hl_src}/H5LTanalyze.c ${path_to_hl_src}/H5LTanalyze.l -# fix H5LTparse.c and H5LTlparse.h to declare H5LTyyparse return type as an -# hid_t instead of int. Currently the generated function H5LTyyparse is -# generated with a return value of type int, which is a mapping to the +# Fix H5LTparse.c and H5LTparse.h to declare H5LTyyparse return type as an +# hid_t instead of int. Currently, the H5LTyyparse function is generated +# with a return value of type int, which is a mapping to the # flex yyparse function. The return value in the HL library should be # an hid_t. -# I propose to not use flex to generate this function, but for now I am -# adding a perl command to find and replace this function declaration in -# H5LTparse.c. +# Use Perl command to find and replace this function declaration +# in H5LTparse.c. This is a temporary solution until a method that does not +# use flex is implemented. perl -0777 -pi -e 's/int yyparse/hid_t yyparse/igs' ${path_to_hl_src}/H5LTparse.c perl -0777 -pi -e 's/int\nyyparse/hid_t\nyyparse/igs' ${path_to_hl_src}/H5LTparse.c perl -0777 -pi -e 's/int H5LTyyparse/hid_t H5LTyyparse/igs' ${path_to_hl_src}/H5LTparse.c From 37a31135ccca69dd31b0bb3b78f6577014c88659 Mon Sep 17 00:00:00 2001 From: bmribler <39579120+bmribler@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:06:59 -0400 Subject: [PATCH 4/5] Added an entry for the GH-4585 fix (#4889) --- release_docs/RELEASE.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 969550cddb5..75c463a5d7a 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -837,6 +837,16 @@ Bug Fixes since HDF5-1.14.0 release =================================== Library ------- + + - Fixed a memory leak in H5F__accum_write() + + The memory was allocated in H5F__accum_write() and was to be freed in + H5F__accum_reset() during the closing process but a failure occurred just + before the deallocation, leaving the memory un-freed. The problem is + now fixed. + + Fixes GitHub #4585 + - Fixed an incorrect returned value by H5LTfind_dataset() H5LTfind_dataset() returned true for non-existing datasets because it only From 6a5514be78a39de8f278dddf8a3fc3698e8154ed Mon Sep 17 00:00:00 2001 From: Allen Byrne <50328838+byrnHDF@users.noreply.github.com> Date: Fri, 27 Sep 2024 10:08:04 -0500 Subject: [PATCH 5/5] Correct inputs ref (#4886) * Add filebase info --- .github/workflows/cmake-script.yml | 8 +++++++- .github/workflows/daily-build.yml | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cmake-script.yml b/.github/workflows/cmake-script.yml index 7c1d4274654..121ce1e3806 100644 --- a/.github/workflows/cmake-script.yml +++ b/.github/workflows/cmake-script.yml @@ -96,6 +96,7 @@ jobs: set (CTEST_DROP_SITE_INIT "my.cdash.org") # Change following line to submit to your CDash dashboard to a different CDash project #set (CTEST_DROP_LOCATION_INIT "/submit.php?project=HDF5") + set (SITE_BUILDNAME_SUFFIX "${{ steps.set-file-base.outputs.FILE_BASE }}") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} --log-level=VERBOSE") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=ON") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_CPP_LIB:BOOL=ON") @@ -184,6 +185,7 @@ jobs: set (CTEST_DROP_SITE_INIT "my.cdash.org") # Change following line to submit to your CDash dashboard to a different CDash project #set (CTEST_DROP_LOCATION_INIT "/submit.php?project=HDF5") + set (SITE_BUILDNAME_SUFFIX "${{ steps.set-file-base.outputs.FILE_BASE }}") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} --log-level=VERBOSE") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=ON") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_CPP_LIB:BOOL=ON") @@ -284,6 +286,7 @@ jobs: set (CTEST_DROP_SITE_INIT "my.cdash.org") # Change following line to submit to your CDash dashboard to a different CDash project #set (CTEST_DROP_LOCATION_INIT "/submit.php?project=HDF5") + set (SITE_BUILDNAME_SUFFIX "${{ steps.set-file-base.outputs.FILE_BASE }}") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} --log-level=VERBOSE") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=ON") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_CPP_LIB:BOOL=ON") @@ -369,6 +372,7 @@ jobs: set (CTEST_DROP_SITE_INIT "my.cdash.org") # Change following line to submit to your CDash dashboard to a different CDash project #set (CTEST_DROP_LOCATION_INIT "/submit.php?project=HDF5") + set (SITE_BUILDNAME_SUFFIX "${{ steps.set-file-base.outputs.FILE_BASE }}") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} --log-level=VERBOSE") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=ON") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_CPP_LIB:BOOL=ON") @@ -468,7 +472,8 @@ jobs: set (CTEST_DROP_SITE_INIT "my.cdash.org") # Change following line to submit to your CDash dashboard to a different CDash project #set (CTEST_DROP_LOCATION_INIT "/submit.php?project=HDF5") - set (CMAKE_GENERATOR_TOOLSET "Intel C++ Compiler 2024,fortran=ifx") + #set (CMAKE_GENERATOR_TOOLSET "Intel C++ Compiler 2024,fortran=ifx") + set (SITE_BUILDNAME_SUFFIX "${{ steps.set-file-base.outputs.FILE_BASE }}") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} --log-level=VERBOSE") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DCMAKE_TOOLCHAIN_FILE:STRING=config/toolchain/intel.cmake") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=ON") @@ -566,6 +571,7 @@ jobs: set (CTEST_DROP_SITE_INIT "my.cdash.org") # Change following line to submit to your CDash dashboard to a different CDash project #set (CTEST_DROP_LOCATION_INIT "/submit.php?project=HDF5") + set (SITE_BUILDNAME_SUFFIX "${{ steps.set-file-base.outputs.FILE_BASE }}") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} --log-level=VERBOSE") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=ON") set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_CPP_LIB:BOOL=ON") diff --git a/.github/workflows/daily-build.yml b/.github/workflows/daily-build.yml index d3e50b41f65..c65355a598a 100644 --- a/.github/workflows/daily-build.yml +++ b/.github/workflows/daily-build.yml @@ -40,7 +40,7 @@ jobs: - name: Read inputs id: getinputs run: | - echo "INPUTS_IGNORE=${{ github.event.inputs.use_ignore }}" >> $GITHUB_OUTPUT + echo "INPUTS_IGNORE=${{ inputs.use_ignore }}" >> $GITHUB_OUTPUT - run: echo "use_ignore is ${{ steps.getinputs.outputs.INPUTS_IGNORE }}."