Add new size field to H5D_compact_storage_t to address buffer overflows

[glennsong09]

Important

Add size field to H5D_compact_storage_t and update related functions to prevent buffer overflows.

• Behavior:
  • Add size field to H5D_compact_storage_t to track buffer size.
  • Update H5D__compact_readvv() in H5Dcompact.c to check buffer size before reading.
  • Modify H5D__chunk_lock() in H5Dchunk.c to initialize and set alloc_chunk_size.
• Functions:
  • Update H5D__chunk_lock() to accept size_t *alloc_chunk_size and set it during chunk allocation.
  • Modify H5D__chunk_read() and H5D__chunk_write() in H5Dchunk.c to use alloc_chunk_size.
• Structures:
  • Add size field to H5D_compact_storage_t in H5Dpkg.h to store buffer size.

^{This description was created by for 72bdcc4. You can customize this summary. It will automatically update as commits are pushed.}

[jhendersonHDF]

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.

[glennsong09]

I made an update to this in my PR.

[jhendersonHDF]

The values are the same, but since this function returns an ssize_t currently, (-1) should be explicitly returned for consistency.

[glennsong09]

I addressed this.

[jhendersonHDF]

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.

[glennsong09]

I addressed this in my update. However, I kept two SIZE_MAX because I looked into finding their size, but does not appear to be a good way to find or easily store their size due to many places messing with the size of dset_info->buf.cvp. Do you mind taking a look and letting me know if you have any good ideas?

[jhendersonHDF]

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_t structure 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 of buf_alloc that comes back rather than setting alloc_chunk_size to 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.

[glennsong09]

I addressed this comment.

[fortnern]

Where does the invalid chunk size come from? In H5D__chunk_lock it's just taken from the layout as far as I can see so I'm not sure why that function needs to return it.

[fortnern]

The size of the valid data in the chunk should be constant an predictable without needing H5D__chunk_lock to return it. Does the buffer overflow occur because the selection goes beyond the chunk or because the chunk is allocated too small? Is it the case where an unfiltered edge chunk of an otherwise filtered dataset has the wrong size stored in the index?

[jhendersonHDF]

The size of the valid data in the chunk should be constant an predictable without needing H5D__chunk_lock to return it. Does the buffer overflow occur because the selection goes beyond the chunk or because the chunk is allocated too small? Is it the case where an unfiltered edge chunk of an otherwise filtered dataset has the wrong size stored in the index?

I don't remember exactly where the invalid value exists in the file, but if I'm remembering correctly it's the case that file corruption causes H5D__chunk_lock() to incorrectly allocate a buffer that is too small without giving back any information about how big that buffer is. Later on, the compact I/O routines run off the buffer since there's an assumption of how big the buffer they're passed is going to be.

[fortnern]

H5D__chunk_lock shouldn't need to report the size of the buffer, it should always be the same size.