GOTCHA icon indicating copy to clipboard operation
GOTCHA copied to clipboard

Don't rely on the bindings list in gotcha_wrap() being static!

Open daboehme opened this issue 3 years ago • 3 comments

Turns out Gotcha internally keeps pointers to the user-provided gotcha_binding_t structs in gotcha_wrap() and happily accesses them later on, e.g. when someone calls dl_open(). That was new to me! Not good when the original buffer is long since gone, as that leads to issues like LLNL/Caliper#288. Would be great if Gotcha would copy the user-provided list instead.

daboehme avatar Jul 13 '20 19:07 daboehme

Does the problem still exist if you reset the priority to point to the original function? I think this is done by resetting the priority to -1 or something like that

jrmadsen avatar Jul 13 '20 19:07 jrmadsen

While we could make this change, it seems a bit awkward. Part of the defined gotcha_wrap() behavior is modifying the gotcha_binding_t[] and handing it back to the user. It feels a little non-intuitive if the gotcha_binding_t we hand back isn't the definitive copy the user should use in the future (such as if I get around to implementing gotcha_unwrap).

Would it be much of a challenge to make Caliper keep the gotcha_binding_t[] around in the heap?

If so, I could be convinced to make this gotcha change. But my first gut feeling is this could change in Caliper.

(And either way, this needs to be better documented in Gotcha).

mplegendre avatar Jul 13 '20 20:07 mplegendre

It's possible to work around this in Caliper, but it's pretty ugly. For example, in the MPI wrapper, we create the bindings list dynamically based on what the user wants to wrap, and I don't really want to keep that list around just so gotcha has something to modify behind my back. It shouldn't be my job to manage gotcha's memory :-) And without unwrap(), how am I supposed to know when it's safe to release the data?

I wasn't even aware that gotcha is supposed to return the modified list - gotcha_get_wrappee() works just fine on the input handle. And what would the user even want with it? I get the gotcha_unwrap() case, but can't you also do that just with the wrappee handle?

Even so, having gotcha_wrap() modify the gotcha_binding_t[] would be one thing, but I for one find it extremely unintuitive that gotcha still accesses behind my back after gotcha_wrap() is done. To me that list looks like input data for gotcha and nothing else. If an API wants to keep modifying user-managed memory in the background, then this should be made very clear.

daboehme avatar Jul 13 '20 22:07 daboehme