-
Notifications
You must be signed in to change notification settings - Fork 287
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 segfault when passing DartOpaque through ffi boundaries #2259
Conversation
Hi! Thanks for opening this pull request! 😄 |
Great catch!
EDIT: I see the problem: It is truncated to (If you need I can provide some suggestions for e.g. what precommit subcommand to run to make CI happy) |
Yeah, i've seen that precommit script but it wasnt working for me until i've rolled back local rust and flutter versions. And i would like to correct myself - Another thing that bothers me is that there is a pretty high possibility of a similar issue happening in some other cases, since passing pointers as Other than that i believe its ready for review. |
Great job! Yes, I agree it is the case when numbers is bigger than isize (still within usize range).
That looks reasonable. To test cst & dco codec, you can (temporarily) set Btw, is it possible we somehow add some tests for this scenario? Then we can ensure it never regress in the future by using CI, and we will also auto test the DCO codec etc. Another btw: It seems https://api.dart.dev/stable/3.5.2/dart-ffi/Pointer/address.html, i.e. a pointer is of type Too busy and a bit tired now, thus will try to review this PR within a day :) |
So i've run some tests and returning
Meh, i dont think there is an easy way to do this. Running test on CI on a Another option is implementing allocator(wrapping default one?) that will always return pointers that are bigger then Another option is not using
So web is out of the question since its 32-bit. And io will be fine since u64 can be represented as i64. E.g: Rust will return
|
Great!
I also think so... The environment is not trivial to create, and the unit tests may not cover wide enough.
Yes, I guess it would be great for the doc to mention something about that (though surely optional) (I will review the PR in detail probably tonight) |
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.
So, i believe this fix can be reworked to continue using usize for pointers, so let me know if you think this is the right way.
I guess that would have some benefits:
- The address is unsigned, so usize looks intuitive
- Other parts of the code often use usize (not isize) for addresses, so this aligns with those parts
- Also, PR size is smaller, making the change clearer
Thus if you like to continue using usize, I am happy to choose that way!
Btw, feel free to ping me (or use |
Great! I will probably review within 24hours |
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.
Great job! Only a nit (other 4 are pointers to point out specific line) and I am ready to merge
frb_codegen/src/library/codegen/generator/codec/sse/ty/dart_opaque.rs
Outdated
Show resolved
Hide resolved
frb_codegen/src/library/codegen/generator/codec/sse/ty/dart_opaque.rs
Outdated
Show resolved
Hide resolved
frb_codegen/src/library/codegen/generator/codec/sse/ty/dart_opaque.rs
Outdated
Show resolved
Hide resolved
frb_codegen/src/library/codegen/generator/codec/sse/ty/dart_opaque.rs
Outdated
Show resolved
Hide resolved
Hi! Congrats on merging your first pull request! 🎉 |
Great job! |
@all-contributors please add @alexlapa for code |
I've put up a pull request to add @alexlapa! 🎉 |
Changes
So, ive encountered a segfault on Android devices after upgrading to frb v2.2.
So whats going on is if a pointer does not fit in u32, it is being changed to u32::max right here which i was able to confirm with additional logging:
Another thing is that for some reason most targets tend to create pointer with values that fit into u32 so it works fine on macOs on Intel silicon, Linux and android x86_64 thats probably why this problem was able to stay under the radar.
Thats also possible that the same exact problem might happen in some other places, since passing pointers as usize is a pretty common thing i believe, but with this fix all my tests pass and app is working properly. However there might be some other scenarios that im not aware of (perhaps when using other codecs?).
Checklist
./frb_internal precommit --mode slow
(orfast
) is run (it internal runs code generator, does auto formatting, etc)../website
folder) are updated.Remark for PR creator
./frb_internal --help
shows utilities for development.