Skip to content

Commit

Permalink
Fix a bug in Subfiling VFD vector I/O setup (HDFGroup#4821)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jhendersonHDF @lrknox
jhendersonHDF authored and lrknox committed Sep 13, 2024
1 parent 8133308 commit b10775b
Show file tree
Hide file tree
Showing 3 changed files with 205 additions and 46 deletions.
18 changes: 18 additions & 0 deletions release_docs/RELEASE.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,24 @@ Bug Fixes since HDF5-1.14.4 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

Expand Down
117 changes: 71 additions & 46 deletions src/H5FDsubfiling/H5FDsubfiling.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand All @@ -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)
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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);

Expand All @@ -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:
Expand Down Expand Up @@ -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)
}

/*-------------------------------------------------------------------------
Expand Down
116 changes: 116 additions & 0 deletions testpar/t_subfiling_vfd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = {
Expand All @@ -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,
};

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b10775b

Please sign in to comment.