From b3329e3019ea082df7653d526ac3feb1899c3cab Mon Sep 17 00:00:00 2001 From: Binh-Minh Date: Wed, 18 Sep 2024 02:59:40 -0400 Subject: [PATCH] Fix security issue from malformed data This PR - fixes an abort of the user's program (issue GH-4435) because the malformed data caused a very large size of memory to be allocated. - removes memory leaks causing the free-list code to abort in addition to the reported issue. NF provided helpful code to the fix. --- src/H5Centry.c | 5 ++++- src/H5Epublic.h | 2 +- src/H5Faccum.c | 13 ++++++++----- src/H5Fint.c | 7 ++++--- src/H5Fio.c | 2 +- src/H5Fpkg.h | 2 +- test/accum.c | 2 +- 7 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/H5Centry.c b/src/H5Centry.c index 6883e897186..7b6303e6f82 100644 --- a/src/H5Centry.c +++ b/src/H5Centry.c @@ -594,8 +594,11 @@ H5C__flush_single_entry(H5F_t *f, H5C_cache_entry_t *entry_ptr, unsigned flags) else mem_type = entry_ptr->type->mem_type; - if (H5F_block_write(f, mem_type, entry_ptr->addr, entry_ptr->size, entry_ptr->image_ptr) < 0) + if (H5F_block_write(f, mem_type, entry_ptr->addr, entry_ptr->size, entry_ptr->image_ptr) < 0) { + H5C__REMOVE_ENTRY_FROM_SLIST(cache_ptr, entry_ptr, during_flush, FAIL); + HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "Can't write image to file"); + } #ifdef H5_HAVE_PARALLEL } #endif /* H5_HAVE_PARALLEL */ diff --git a/src/H5Epublic.h b/src/H5Epublic.h index 49628efb9be..20e1f9f00f3 100644 --- a/src/H5Epublic.h +++ b/src/H5Epublic.h @@ -210,7 +210,7 @@ typedef herr_t (*H5E_auto2_t)(hid_t estack, void *client_data); * \param[in] lib_name Name of the client library or application to which the error class belongs * \param[in] version Version of the client library or application to which the error class belongs. It can be \c NULL. - * \return Returns a class identifier on success; otherwise returns H5I_INVALID_ID. + * \return Returns a class identifier on success; otherwise returns H5I_INVALID_HID. * * \details H5Eregister_class() registers a client library or application * program to the HDF5 error API so that the client library or 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 e9817b13048..f66dfa83cd8 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1152,7 +1152,8 @@ H5F__new(H5F_shared_t *shared, unsigned flags, hid_t fcpl_id, hid_t fapl_id, H5F */ if (NULL == (plist = (H5P_genplist_t *)H5I_object(fcpl_id))) HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, NULL, "not property list"); - f->shared->fcpl_id = H5P_copy_plist(plist, false); + if ((f->shared->fcpl_id = H5P_copy_plist(plist, false)) < 0) + HGOTO_ERROR(H5E_PLIST, H5E_CANTCOPY, NULL, "can't copy property list"); /* Get the FCPL values to cache */ if (H5P_get(plist, H5F_CRT_ADDR_BYTE_NUM_NAME, &f->shared->sizeof_addr) < 0) @@ -1559,7 +1560,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 +3894,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 */