Fix segfault on VOL termination #100
Conversation
| rv_hash_table_free(RV_type_info_array_g[i]->table); | ||
| RV_free(RV_type_info_array_g[i]); | ||
| RV_type_info_array_g[i] = NULL; | ||
| if (RV_type_info_array_g) { |
There was a problem hiding this comment.
It's likely the fact that it isn't explicitly initialized to NULL entries, so it ends up trying to free random pointers. The condition here probably doesn't do much since RV_type_info_array_g is an array of pointers. Rather, I think the real fix should be changing RV_type_info *RV_type_info_array_g[H5I_MAX_NUM_TYPES]; to RV_type_info *RV_type_info_array_g[H5I_MAX_NUM_TYPES] = {0}; at the top of this file, while also keeping the added check below of if (RV_type_info_array_g[i]) {.
There was a problem hiding this comment.
Note that freeing a NULL pointer is allowed by the standard, so this is almost certainly due to the array having uninitialized entries that the code is attempting to free.
There was a problem hiding this comment.
This particular issue was due to the top level RV_type_info_array_g pointer being NULL when referenced in line 633, so I think we should keep that check around. It's still true that we should avoid passing around uninitialized pointers - I've added the initialization.
|
The failures in the fetchcontent actions can be ignored, as they're a commit behind due to the nature of fetching the current HEAD commit of the upstream repo rather than working from the commit in the PR. We should probably have the fetchcontent actions be a separate daily action, or remove them from this repo's CI testing as I'm testing them elsewhere now and they'll soon be in a daily build in the HDF5 repo. |
Sometimes the global type array
RV_type_info_array_gcan have a NULL value, and if it did, the VOL termination would attempt to dereference a NULL pointer while freeing it. It now checks that the array exists before trying to free it.I'm not exactly sure what factors cause the array to be NULL - it's declared in
rest_vol.h, defined inrest_vol.c, and never explicitly set to NULL. This issue didn't come up with the dynamically linked or manually linked tests on their own - it only cropped up when using the manually linked tests (test_rest_vol) when they were built under the library with fetch content, and when running them through the autotools-generated script.