-
Notifications
You must be signed in to change notification settings - Fork 662
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
fix ownership of c stream error #4660
fix ownership of c stream error #4660
Conversation
8e82b30
to
cbb4aa1
Compare
match ffi_stream.get_last_error() { | ||
Some(err_string) => err_string.as_ptr(), | ||
None => std::ptr::null(), | ||
} |
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.
👍 nice catch
@@ -503,4 +512,32 @@ mod tests { | |||
|
|||
_test_round_trip_import(vec![array.clone(), array.clone(), array]) | |||
} | |||
|
|||
#[test] | |||
fn test_error_import() -> Result<()> { |
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.
Hmm, this test also passes in current branch without the fix. Doesn't?
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.
Yes that is true. We export an owned string, and then import an owned string. So round tripping to ourselves works. But other implementations (such as PyArrow) export a borrowed string, so our current implementation will raise a double free. To reproduce, within our implementation, add the changes for export at the top of the file, but leave out the changes for import. You should get a double free.
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've added a PyArrow integration test that will fail on master.
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.
Thank you.
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.
The fix looks good to me. Just a question about the added test.
d0f12c7
to
d6b70f2
Compare
Which issue does this PR close?
Closes #4659.
Rationale for this change
The current implementation doesn't obey the ownership semantics laid out in the spec. When exporting to other Arrow implementations, it will leak memory through the
get_last_error()
method. When importing from other Arrow implementations, it will cause a double free. Also, null pointers aren't being handled.What changes are included in this PR?
Changes
get_last_error
implementations to treat the returned value as a borrowed and not owned string.Are there any user-facing changes?
I don't think so. It's possible that someone using the C Stream Interface to move data between an older version of arrow-rs and one after this change may face some FFI issues, but I doubt anyone is doing that.