Fix AddressSanitizer issue in cache test #5235
Conversation
jhendersonHDF
added
Priority - 2. Medium ⏹
jhendersonHDF
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
jhendersonHDF
force-pushed
the
cache_test_asan_fix
branch
from
d56b315 to
1990254
Compare
jhendersonHDF
force-pushed
the
cache_test_asan_fix
branch
from
1990254 to
eab2480
Compare
|
Reworked this a little bit to handle the other stack issues as well |
Fix usages of stack variables after function return by removing extraneous H5CX_push/pop operations and hoisting a stack variable to a higher scope
jhendersonHDF
force-pushed
the
cache_test_asan_fix
branch
from
a6d5eac to
84d54ef
Compare
|
|
||
| if (show_progress) /* 3 */ | ||
| fprintf(stdout, "%s() - %0d -- pass = %d\n", __func__, mile_stone++, (int)pass); | ||
|
|
||
| /* Push API context */ | ||
| H5CX_push(&api_ctx); |
There was a problem hiding this comment.
All of the push/pop calls in these tests appear to have been unnecessary, as API contexts are all pushed right after setup_cache() (which pushes its own API context) and popped right after takedown_cache() (which pops its own API context), at least before the initial version of this fix. Just removed all the extra push/pop calls and kept the single context pushed/popped by the setup/takedown functions.
| haddr_t actual_base_addr; | ||
| hid_t fapl_id = H5P_DEFAULT; | ||
| hid_t fcpl_id = H5P_DEFAULT; | ||
| H5CX_node_t api_ctx = {{0}, NULL}; /* API context node to push */ |
There was a problem hiding this comment.
As H5CX_push() has since been refactored to use the passed in context object directly, rather then allocating a new node, this variable needs to be hoisted out of this function. Otherwise, the library will be holding onto a stale address once this function returns. The simplest way seemed to be just to pass in a pointer to a H5CX_node_t, but, again, this seems to point to design issues.
| @@ -26856,20 +26577,16 @@ check_flush_deps_err(unsigned paged) | |||
| } /* end switch */ | |||
|
|
|||
| takedown_cache(file_ptr, false, false); | |||
| if (!pass) | |||
| if (!pass) { | |||
| file_ptr = NULL; | |||
There was a problem hiding this comment.
Prevent accidentally calling takedown_cache() twice in case the first time fails
Fixes issues with usages of stack variables after function return related to API context pushing/popping