-
-
Notifications
You must be signed in to change notification settings - Fork 145
Prevent FFI from deallocating data before returning it #741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
For completeness, here's a helpful GDB script: break src/ffi.rs:630
commands
disable $_hit_bpnum
end
set $drop_break = $bpnum
disable $drop_break
tcatch load ffi_lib
commands
silent
break dummy_md5 if out != 0
commands
set $msg=&(*m@len)
set $out=&(*out@len)
printf "Updated $msg and $out\n"
enable $drop_break
display
end
continue
end
display $out Run with |
@@ -639,10 +639,10 @@ mod enabled { | |||
|
|||
type ListStorage<T> = (*mut T, Box<[T]>); | |||
|
|||
#[derive(Default)] | |||
#[derive(Default, Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot to remove Debug
after debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it doesn't hurt to have a derived Debug I think? I mean, I can remove it, but I don't see a problem either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't see a reason not to derive Debug
if it's possible to.
dbgln!(" ptr type"); | ||
(unsafe { &**p }, Some(*p)) | ||
// SAFETY: TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolve this todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change how this code behaves, only rewrote it to make the intent clearer. I don't know if this code is sound because I didn't write the original code.
let ptr = if arg.is_empty() { | ||
// TODO: is this special case necessary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolve this TODO too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is impossible without input from Kai — I have no idea about why a null pointer is used there, and I'm not sure where the code dependent on this could be.
I definitely think this PR (and #742) need input from Kai before we can think about merging them. I personally very much don't know enough about what's going on in this PR to be able to comment on it at the moment unfortunately. |
How does this fix tests? |
|
This should just be fixed in |
What's the test case that was causing issues? |
Well, the better solution would be leaking just the needed data, but I do not understand Uiua code well enough for that. As noted in the PR description:
To be honest, after spending >24h studying the code, running |
Seemingly the only line testing output buffers: Line 41 in 8a9de2d
|
|
Well, this is not the cause for output buffers, since those are supposed to live after |
Would this make you have to use |
Well, it's not like you have to, but yeah, that's what happens. |
This is solved by out parameter pointers in #783 |
This effectively makes
&ffi
leak all allocated memory. It should probably be possible to only leak the necessary data, but the existing FFI code is a bit too convoluted to comprehend.