Fix a bug in Subfiling VFD vector I/O setup (#4821)

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.

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
 

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)
 }
 
 /*-------------------------------------------------------------------------

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