Fix several undefined behavior issues noted by UBsan#6319
Fix several undefined behavior issues noted by UBsan#6319jhendersonHDF wants to merge 2 commits intoHDFGroup:developfrom
Conversation
Disable float16 support for undefined sanitizer workflow for now as it causes a crash in UBSan
jhendersonHDF
added
Component - C Library
| /********************/ | ||
|
|
||
| static herr_t H5A__close_cb(H5VL_object_t *attr_vol_obj, void **request); | ||
| static herr_t H5A__close_cb(void *attr_vol_obj, void **request); |
There was a problem hiding this comment.
There were several instances in the library where an H5I_free_t callback was declared with some type parameter instead of void * and then casted to H5I_free_t, causing UBSan to complain. The typing is lost for these callbacks, but several other H5I_free_t callbacks in the library already use the correct void * parameter anyway.
| ((const uint8_t *)write_bufs[i] - (const uint8_t *)io_info->tconv_buf)); | ||
| void *tmp_write_buf; | ||
|
|
||
| if (io_info->sel_pieces[i]->in_place_tconv) { |
There was a problem hiding this comment.
@fortnern do these changes look correct? UBSan was complaining because in this particular case (where modifying of write buffers is enabled), the type conversion buffer wasn't allocated, so the previous logic was adding a non-zero offset to a NULL pointer. I'm not entirely certain how the testing passes currently given that, but it was definitely wrong. In the in_place_tconv case, it seemed like the logic above in the first loop already set each write_bufs[i] to point to the correct location in the write buffer. In fact, it also looks like each write_bufs[i] gets set to the correct place in io_info->tconv_buf above as well, so both cases might be able to be simplified to just the two lines below without needing all the casting in the in_place_tconv == false case.
| set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_JAVA:BOOL=OFF") | ||
| set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_CPP_LIB:BOOL=ON") | ||
| set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_BUILD_FORTRAN:BOOL=OFF") | ||
| set (ADD_BUILD_OPTIONS "${ADD_BUILD_OPTIONS} -DHDF5_ENABLE_NONSTANDARD_FEATURE_FLOAT16:BOOL=OFF") |
There was a problem hiding this comment.
The _Float16 type currently crashes UBSan, preventing further analysis in dt_arith.c.
| *thing = iblock; | ||
| *thing_elmt_buf = (uint8_t *)iblock->elmts; | ||
| *thing_elmt_idx = idx; | ||
| *thing_unprot_func = (H5EA__unprotect_func_t)H5EA__iblock_unprotect; |
There was a problem hiding this comment.
Similar to H5I_free_t, these callbacks were declaring in an incompatible way with H5EA__unprotect_func_t and then casted into it, causing UBSan to complain.
| { | ||
| bool skip; | ||
| H5T_t *dt = NULL; | ||
| const uint8_t *p_end = p + p_size - 1; |
There was a problem hiding this comment.
There were a handful of cases in the library where this type of calculation was performed when p_size is SIZE_MAX (the case where we can't know how big the buffer is for checking during decoding). This would cause p to overflow and cause UBSan to complain. Not really an issue, but easy to fix by checking p_size before doing the addition.
| { | ||
| H5F_t *src_f = NULL; | ||
| hid_t file_id = H5I_INVALID_HID; | ||
| H5R_ref_priv_t *dst_ref = (H5R_ref_priv_t *)dst_buf; |
There was a problem hiding this comment.
This casting can form an unaligned pointer which could potentially be problematic and which UBSan complains about. As the only place it is actually used is in a memcpy() from a temporary, aligned local reference object, just memcpy() directly into dst_buf instead of trying to go through dst_ref.
| assert(src_size); | ||
| assert(dst_buf); | ||
| assert(dst_size == H5T_REF_MEM_SIZE); | ||
| HDcompile_assert(sizeof(*dst_ref) == sizeof(tmp_ref)); |
There was a problem hiding this comment.
This assertion isn't really needed because *dst_ref would always be H5R_ref_priv_t in the previous logic, and tmp_ref is a H5R_ref_priv_t, making this trivially true. For catching future size issues, using dst_size would probably be more appropriate, but that was a bit outside the scope of this fix.
jhendersonHDF
requested review from
bmribler,
brtnfld,
derobins,
fortnern,
glennsong09,
lrknox,
mattjala,
qkoziol and
vchoi-hdfgroup
as code owners
|
Not part of PR, but minor observation: At H5T.c:2502, the error line reads: HGOTO_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to free VOL object"); |
6 participants
Disable float16 support for undefined sanitizer workflow for now as it causes a crash in UBSan