Add new size field to H5D_compact_storage_t to address buffer overflows #5950
Conversation
glennsong09
requested review from
bmribler,
byrnHDF,
derobins,
fortnern,
jhendersonHDF,
lrknox,
mattjala,
qkoziol and
vchoi-hdfgroup
as code owners
|
|
||
| /* Check if src buffer size is less than expected */ | ||
| if (src_alloc_size > 0) { | ||
| if (src_alloc_size < tmp_src_len) |
There was a problem hiding this comment.
Just checking tmp_src_len isn't enough here, as there may be more than one sequence and depending on which branch is taken below, either tmp_src_len or tmp_dst_len will need to be checked. You can see that tmp_src_len and tmp_dst_len are continually updated in the loops below, so you'll need to check tmp_src_len/tmp_dst_len against the remaining space left in _src on each iteration, which will involve keeping track of the end of _src and using that alongside the current offset into it (src). The checking can probably be done in the usual way with the H5_IS_BUFFER_OVERFLOW macro.
| /* Check if src buffer size is less than expected */ | ||
| if (src_alloc_size > 0) { | ||
| if (src_alloc_size < tmp_src_len) | ||
| HGOTO_ERROR(H5E_INTERNAL, H5E_CANTOPERATE, FAIL, "src buffer size is less than expected"); |
There was a problem hiding this comment.
The values are the same, but since this function returns an ssize_t currently, (-1) should be explicitly returned for consistency.
| /* Perform vectorized memcpy from src_buf to dst_buf */ | ||
| if ((bytes_copied = H5VM_memcpyvv(dst_buf, dst_nseq, &curr_dst_seq, dst_len, dst_off, src_buf, | ||
| src_nseq, &curr_src_seq, src_len, src_off)) < 0) | ||
| src_nseq, &curr_src_seq, src_len, src_off, 0)) < 0) |
There was a problem hiding this comment.
I'd maybe consider SIZE_MAX instead of 0 for an unknown value, but in this case we should probably make sure we always have a known value to pass to H5VM_memcpyvv.
| static void * | ||
| H5D__chunk_lock(const H5D_io_info_t H5_ATTR_NDEBUG_UNUSED *io_info, const H5D_dset_io_info_t *dset_info, | ||
| H5D_chunk_ud_t *udata, bool relax, bool prev_unfilt_chunk) | ||
| H5D_chunk_ud_t *udata, bool relax, bool prev_unfilt_chunk, size_t *alloc_chunk_size) |
There was a problem hiding this comment.
In this function, it looks like there are several places where the final value for alloc_chunk_size should be set. At least:
- After the chunk entry is retrieved from the cache on line 4372 (which means the
H5D_rdcc_ent_tstructure is probably also going to need to track the size of the allocated chunk buffer) - After the call to
H5D__chunk_mem_alloc()on line 4408 - After the call to
H5D__chunk_mem_alloc()on line 4434 - After the call to
H5D__chunk_mem_alloc()on line 4515 - After the call to
H5D__chunk_mem_alloc()on line 4535 (as is done currently in this PR) - After the call to
H5Z_pipeline()on line 4553 (as is done currently in this PR, but using the value ofbuf_allocthat comes back rather than settingalloc_chunk_sizeto 0) - After the call to
H5D__chunk_mem_alloc()on line 4561 - After the call to
H5D__chunk_mem_alloc()on line 4582
Due to all these places, it would probably be easiest to keep track of the value in a local variable and assign the value to alloc_chunk_size somewhere toward the end of the function.
4 participants
Important
Add
sizefield toH5D_compact_storage_tand update related functions to prevent buffer overflows.sizefield toH5D_compact_storage_tto track buffer size.H5D__compact_readvv()inH5Dcompact.cto check buffer size before reading.H5D__chunk_lock()inH5Dchunk.cto initialize and setalloc_chunk_size.H5D__chunk_lock()to acceptsize_t *alloc_chunk_sizeand set it during chunk allocation.H5D__chunk_read()andH5D__chunk_write()inH5Dchunk.cto usealloc_chunk_size.sizefield toH5D_compact_storage_tinH5Dpkg.hto store buffer size.This description was created by
for 72bdcc4. You can customize this summary. It will automatically update as commits are pushed.