From 9a3ccd2b5ad146be1d4f1b5ac4c75dd9a67b81c6 Mon Sep 17 00:00:00 2001 From: Jordan Henderson Date: Tue, 10 Sep 2024 16:17:54 -0500 Subject: [PATCH 1/2] Fix a bug in Subfiling VFD vector I/O setup Fixes a bug where the vector I/O sizes weren't being extended when one of the entries in the array is 0. This caused an over-read of the I/O sizes buffer and on some machines would cause a memory allocation failure due to the calculated I/O vector size being too large. --- src/H5FDsubfiling/H5FDsubfiling.c | 117 ++++++++++++++++++------------ testpar/t_subfiling_vfd.c | 116 +++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 46 deletions(-) diff --git a/src/H5FDsubfiling/H5FDsubfiling.c b/src/H5FDsubfiling/H5FDsubfiling.c index affcfcbe94a..796654254ad 100644 --- a/src/H5FDsubfiling/H5FDsubfiling.c +++ b/src/H5FDsubfiling/H5FDsubfiling.c @@ -196,14 +196,14 @@ static herr_t H5FD__subfiling_mirror_writes_to_stub(H5FD_subfiling_t *file, uint const void *bufs[]); static herr_t H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_count, H5FD_mem_t types[], haddr_t file_offsets[], - size_t nelemts[], H5_flexible_const_ptr_t bufs[], - H5FD_subfiling_io_type_t io_type, size_t *ioreq_count, - uint32_t *iovec_len, H5FD_mem_t **io_types, - haddr_t **io_addrs, size_t **io_sizes, - H5_flexible_const_ptr_t **io_bufs); -static void H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count, - haddr_t file_offsets[], size_t nelemts[], size_t *max_iovec_depth, - size_t *max_num_subfiles); + size_t io_sizes[], H5_flexible_const_ptr_t bufs[], + H5FD_subfiling_io_type_t io_type, size_t *ioreq_count_out, + uint32_t *iovec_len_out, H5FD_mem_t **io_types_out, + haddr_t **io_addrs_out, size_t **io_sizes_out, + H5_flexible_const_ptr_t **io_bufs_out); +static herr_t H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count, + haddr_t file_offsets[], size_t io_sizes[], + size_t *max_iovec_depth, size_t *max_num_subfiles); static herr_t H5FD__subfiling_translate_io_req_to_iovec( subfiling_context_t *sf_context, size_t iovec_idx, size_t iovec_len, size_t iovec_count, H5FD_mem_t type, haddr_t addr, size_t io_size, H5_flexible_const_ptr_t io_buf, H5FD_subfiling_io_type_t io_type, @@ -2023,7 +2023,7 @@ H5FD__subfiling_io_helper(H5FD_subfiling_t *file, size_t io_count, H5FD_mem_t ty io_count, /* IN: Number of entries in `types`, `addrs`, `sizes` and `bufs` */ types, /* IN: Array of memory types */ addrs, /* IN: Array of starting file offsets */ - sizes, /* IN: Array of I/O sizes (in terms of elements) */ + sizes, /* IN: Array of I/O sizes */ bufs, /* IN: Array of I/O buffers */ io_type, /* IN: Type of I/O being performed (IO_TYPE_WRITE or IO_TYPE_READ) */ &ioreq_count, /* OUT: Number of I/O requests to be made */ @@ -2335,30 +2335,30 @@ H5FD__subfiling_mirror_writes_to_stub(H5FD_subfiling_t *file, uint32_t count, H5 * - the type of I/O being performed (IO_TYPE_WRITE or * IO_TYPE_READ) * - * ioreq_count (OUT) + * ioreq_count_out (OUT) * - the number of I/O requests needed to fully satisfy the * I/O operation * - * iovec_len (OUT) + * iovec_len_out (OUT) * - the size of each I/O vector (in terms of array elements) * for each I/O request to be made * - * io_types (OUT) + * io_types_out (OUT) * - I/O vector of memory types for the I/O operation. * Allocated by this function and must be freed by the * caller. * - * io_addrs (OUT) + * io_addrs_out (OUT) * - I/O vector of file addresses for the I/O operation. * Allocated by this function and must be freed by the * caller. * - * io_sizes (OUT) + * io_sizes_out (OUT) * - I/O vector of the I/O sizes for the I/O operation. * Allocated by this function and must be freed by the * caller. * - * io_bufs (OUT) + * io_bufs_out (OUT) * - I/O vector of the I/O buffers for the I/O operation. * Allocated by this function and must be freed by the * caller. @@ -2368,10 +2368,11 @@ H5FD__subfiling_mirror_writes_to_stub(H5FD_subfiling_t *file, uint32_t count, H5 */ static herr_t H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_count, H5FD_mem_t types[], - haddr_t file_offsets[], size_t nelemts[], H5_flexible_const_ptr_t bufs[], - H5FD_subfiling_io_type_t io_type, size_t *ioreq_count, - uint32_t *iovec_len, H5FD_mem_t **io_types, haddr_t **io_addrs, - size_t **io_sizes, H5_flexible_const_ptr_t **io_bufs) + haddr_t file_offsets[], size_t io_sizes[], H5_flexible_const_ptr_t bufs[], + H5FD_subfiling_io_type_t io_type, size_t *ioreq_count_out, + uint32_t *iovec_len_out, H5FD_mem_t **io_types_out, + haddr_t **io_addrs_out, size_t **io_sizes_out, + H5_flexible_const_ptr_t **io_bufs_out) { H5_flexible_const_ptr_t *loc_io_bufs = NULL; H5FD_mem_t *loc_io_types = NULL; @@ -2395,18 +2396,18 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c assert(sf_context->topology); assert(types || in_count == 0); assert(file_offsets || in_count == 0); - assert(nelemts || in_count == 0); + assert(io_sizes || in_count == 0); assert(bufs || in_count == 0); - assert(ioreq_count); - assert(iovec_len); - assert(io_types); - assert(io_addrs); - assert(io_sizes); - assert(io_bufs); + assert(ioreq_count_out); + assert(iovec_len_out); + assert(io_types_out); + assert(io_addrs_out); + assert(io_sizes_out); + assert(io_bufs_out); /* Set some returned values early */ - *ioreq_count = 0; - *iovec_len = 0; + *ioreq_count_out = 0; + *iovec_len_out = 0; /* Nothing to do */ if (in_count == 0) @@ -2416,11 +2417,16 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c * Do some initial pre-processing to determine how large of I/O vectors we * will need to allocate to satisfy the entire I/O request */ - H5FD__subfiling_get_iovec_sizes(sf_context, in_count, file_offsets, nelemts, &max_iovec_depth, - &max_num_subfiles_touched); + if (H5FD__subfiling_get_iovec_sizes(sf_context, in_count, file_offsets, io_sizes, &max_iovec_depth, + &max_num_subfiles_touched) < 0) + HGOTO_ERROR(H5E_VFL, H5E_CANTGET, FAIL, "can't determine maximum I/O request size"); tot_iovec_len = in_count * max_iovec_depth * max_num_subfiles_touched; + /* Nothing to do */ + if (tot_iovec_len == 0) + HGOTO_DONE(SUCCEED); + #ifdef H5_SUBFILING_DEBUG H5FD__subfiling_log( sf_context->sf_context_id, @@ -2456,10 +2462,10 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c } if (!extend_sizes) { - if (io_idx > 0 && nelemts[io_idx] == 0) + if (io_idx > 0 && io_sizes[io_idx] == 0) extend_sizes = true; else - io_size = nelemts[io_idx]; + io_size = io_sizes[io_idx]; } if (H5FD__subfiling_translate_io_req_to_iovec(sf_context, iovec_idx, max_num_subfiles_touched, @@ -2469,13 +2475,13 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c HGOTO_ERROR(H5E_VFL, H5E_CANTINIT, FAIL, "can't translate I/O request to I/O vectors"); } - *ioreq_count = in_count * max_iovec_depth; + *ioreq_count_out = in_count * max_iovec_depth; H5_CHECK_OVERFLOW(max_num_subfiles_touched, size_t, uint32_t); - *iovec_len = (uint32_t)max_num_subfiles_touched; - *io_types = loc_io_types; - *io_addrs = loc_io_addrs; - *io_sizes = loc_io_sizes; - *io_bufs = loc_io_bufs; + *iovec_len_out = (uint32_t)max_num_subfiles_touched; + *io_types_out = loc_io_types; + *io_addrs_out = loc_io_addrs; + *io_sizes_out = loc_io_sizes; + *io_bufs_out = loc_io_bufs; done: if (ret_value < 0) { @@ -2497,26 +2503,28 @@ H5FD__subfiling_generate_io_vectors(subfiling_context_t *sf_context, size_t in_c * info is used to calculate the total size of I/O vectors we * need to allocate to satisfy an entire I/O request. * - * Return: Maximum I/O vector depth and maximum number of subfiles - * touched (can't fail) + * Return: Non-negative on success/negative on failure * *------------------------------------------------------------------------- */ -static void +static herr_t H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count, haddr_t file_offsets[], - size_t nelemts[], size_t *max_iovec_depth, size_t *max_num_subfiles) + size_t io_sizes[], size_t *max_iovec_depth, size_t *max_num_subfiles) { int64_t stripe_size = 0; int64_t block_size = 0; size_t loc_max_iovec_depth = 0; size_t loc_max_num_subfiles = 0; + size_t io_size = 0; + bool extend_sizes = false; int num_subfiles = 0; + herr_t ret_value = SUCCEED; - FUNC_ENTER_PACKAGE_NOERR + FUNC_ENTER_PACKAGE assert(sf_context); assert(file_offsets); - assert(nelemts); + assert(io_sizes); assert(max_iovec_depth); assert(max_num_subfiles); @@ -2538,7 +2546,23 @@ H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count size_t cur_iovec_depth; H5_CHECKED_ASSIGN(cur_file_offset, int64_t, file_offsets[io_idx], haddr_t); - H5_CHECKED_ASSIGN(data_size, int64_t, nelemts[io_idx], size_t); + + if (!extend_sizes) { + if (io_idx > 0 && io_sizes[io_idx] == 0) + extend_sizes = true; + else + io_size = io_sizes[io_idx]; + } + + H5_CHECKED_ASSIGN(data_size, int64_t, io_size, size_t); + + if (cur_file_offset < 0) + HGOTO_ERROR(H5E_VFL, H5E_BADVALUE, FAIL, + "file offset of %" PRIuHADDR " at index %zu too large; wrapped around", + file_offsets[io_idx], io_idx); + if (data_size < 0) + HGOTO_ERROR(H5E_VFL, H5E_BADVALUE, FAIL, "I/O size of %zu at index %zu too large; wrapped around", + io_size, io_idx); /* * Calculate the following from the starting file offset: @@ -2645,7 +2669,8 @@ H5FD__subfiling_get_iovec_sizes(subfiling_context_t *sf_context, size_t in_count *max_iovec_depth = loc_max_iovec_depth; *max_num_subfiles = loc_max_num_subfiles; - FUNC_LEAVE_NOAPI_VOID +done: + FUNC_LEAVE_NOAPI(ret_value) } /*------------------------------------------------------------------------- diff --git a/testpar/t_subfiling_vfd.c b/testpar/t_subfiling_vfd.c index 27c48250be4..aa2da851eaa 100644 --- a/testpar/t_subfiling_vfd.c +++ b/testpar/t_subfiling_vfd.c @@ -105,6 +105,7 @@ static void test_read_different_stripe_size(void); static void test_subfiling_precreate_rank_0(void); static void test_subfiling_write_many_read_one(void); static void test_subfiling_write_many_read_few(void); +static void test_subfiling_vector_io_extension(void); static void test_subfiling_h5fuse(void); static test_func tests[] = { @@ -118,6 +119,7 @@ static test_func tests[] = { test_subfiling_precreate_rank_0, test_subfiling_write_many_read_one, test_subfiling_write_many_read_few, + test_subfiling_vector_io_extension, test_subfiling_h5fuse, }; @@ -2568,6 +2570,120 @@ test_subfiling_write_many_read_few(void) #undef SUBF_HDF5_TYPE #undef SUBF_C_TYPE +/* + * Test to check for a bug where the vector I/O sizes + * array wasn't being extended when an entry in the + * array was 0. + */ +#define SUBF_FILENAME "test_subfiling_vector_io_extension.h5" +#define SUBF_C_TYPE int +static void +test_subfiling_vector_io_extension(void) +{ + H5FD_subfiling_params_t cfg; + h5_stat_size_t file_size; + SUBF_C_TYPE *read_buf = NULL; + H5FD_mem_t *types = NULL; + h5_stat_t file_info; + uint32_t count = 64; + haddr_t *addrs = NULL; + haddr_t file_end_addr; + herr_t read_status; + size_t *sizes = NULL; + H5FD_t *file_ptr = NULL; + hid_t file_id = H5I_INVALID_HID; + hid_t fapl_id = H5I_INVALID_HID; + hid_t dxpl_id = H5I_INVALID_HID; + void **bufs = NULL; + + curr_nerrors = nerrors; + + if (MAINPROCESS) + TESTING_2("I/O vector size extension functionality"); + + /* Must use at least 2 subfiles to cause generation of + * I/O vectors within the VFD. + */ + cfg.ioc_selection = SELECT_IOC_ONE_PER_NODE; + cfg.stripe_size = (stripe_size_g > 0) ? stripe_size_g : 1048576; + cfg.stripe_count = num_iocs_g > 1 ? num_iocs_g : 2; + + fapl_id = create_subfiling_ioc_fapl(comm_g, info_g, true, &cfg, H5FD_IOC_DEFAULT_THREAD_POOL_SIZE); + VRFY((fapl_id >= 0), "FAPL creation succeeded"); + + file_id = H5Fcreate(SUBF_FILENAME, H5F_ACC_TRUNC, H5P_DEFAULT, fapl_id); + VRFY((file_id >= 0), "H5Fcreate succeeded"); + + VRFY((H5Fclose(file_id) >= 0), "File close succeeded"); + + /* Re-open file through H5FDopen for direct reads */ + file_ptr = H5FDopen(SUBF_FILENAME, H5F_ACC_RDWR, fapl_id, HADDR_UNDEF); + VRFY(file_ptr, "H5FDopen succeeded"); + + /* + * Get the current file size to see where we can safely + * write to in the file without overwriting the superblock + */ + memset(&file_info, 0, sizeof(h5_stat_t)); + VRFY((HDstat(SUBF_FILENAME, &file_info) >= 0), "HDstat succeeded"); + file_size = (h5_stat_size_t)file_info.st_size; + + H5_CHECK_OVERFLOW(file_size, h5_stat_size_t, haddr_t); + file_end_addr = (haddr_t)file_size; + + dxpl_id = H5Pcreate(H5P_DATASET_XFER); + VRFY((dxpl_id >= 0), "DXPL creation succeeded"); + + /* Set independent I/O on DXPL */ + VRFY((H5Pset_dxpl_mpio(dxpl_id, H5FD_MPIO_INDEPENDENT) >= 0), "H5Pset_dxpl_mpio succeeded"); + + /* Set EOA for following read call */ + VRFY((H5FDset_eoa(file_ptr, H5FD_MEM_DEFAULT, file_end_addr + (count * sizeof(int))) >= 0), + "H5FDset_eoa succeeded"); + + read_buf = malloc(count * sizeof(*read_buf)); + types = malloc(count * sizeof(*types)); + addrs = malloc(count * sizeof(*addrs)); + sizes = malloc(2 * sizeof(size_t)); + bufs = malloc(count * sizeof(*bufs)); + + sizes[0] = sizeof(SUBF_C_TYPE); + sizes[1] = 0; + + for (size_t i = 0; i < count; i++) { + types[i] = H5FD_MEM_DRAW; + addrs[i] = file_end_addr + (i * sizeof(SUBF_C_TYPE)); + bufs[i] = (void *)&(read_buf[i]); + } + + read_status = H5FDread_vector(file_ptr, dxpl_id, count, types, addrs, sizes, bufs); + VRFY((read_status >= 0), "H5FDread_vector succeeded"); + + VRFY((H5FDclose(file_ptr) >= 0), "H5FDclose succeeded"); + + mpi_code_g = MPI_Barrier(comm_g); + VRFY((mpi_code_g == MPI_SUCCESS), "MPI_Barrier succeeded"); + + H5E_BEGIN_TRY + { + H5Fdelete(SUBF_FILENAME, fapl_id); + } + H5E_END_TRY + + VRFY((H5Pclose(fapl_id) >= 0), "FAPL close succeeded"); + VRFY((H5Pclose(dxpl_id) >= 0), "DXPL close succeeded"); + + free(bufs); + free(sizes); + free(addrs); + free(types); + free(read_buf); + + CHECK_PASSED(); +} +#undef SUBF_FILENAME +#undef SUBF_C_TYPE + /* * Test that the subfiling file can be read with the * sec2 driver after being fused back together with From a1f4d43f64ca66cc9b2191b8b14ef92ad709b4e4 Mon Sep 17 00:00:00 2001 From: Jordan Henderson Date: Wed, 11 Sep 2024 12:34:12 -0500 Subject: [PATCH 2/2] Add a release note --- release_docs/RELEASE.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 0686bd47739..68f75531adf 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -823,6 +823,24 @@ Bug Fixes since HDF5-1.14.0 release =================================== Library ------- + - Fixed a bug in the Subfiling VFD that could cause a buffer over-read + and memory allocation failures + + When performing vector I/O with the Subfiling VFD, making use of the + vector I/O size extension functionality could cause the VFD to read + past the end of the "I/O sizes" array that is passed in. When an entry + in the "I/O sizes" array has the value 0 and that entry is at an array + index greater than 0, this signifies that the value in the preceding + array entry should be used for the rest of the I/O vectors, effectively + extending the last valid I/O size across the remaining entries. This + allows an application to save a bit on memory by passing in a smaller + "I/O sizes" array. The Subfiling VFD didn't implement a check for this + functionality in the portion of the code that generates I/O vectors, + causing it to read past the end of the "I/O sizes" array when it was + shorter than expected. This could also result in memory allocation + failures, as the nearby memory allocations are based off the values + read from that array, which could be uninitialized. + - Fixed H5Rget_attr_name to return the length of the attribute's name without the null terminator