-
Notifications
You must be signed in to change notification settings - Fork 33
fix: Use uint64_t for RustBuffer fields to fix 32-bit ARM crash #313
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
base: main
Are you sure you want to change the base?
fix: Use uint64_t for RustBuffer fields to fix 32-bit ARM crash #313
Conversation
RustBuffer must use uint64_t (not size_t) for capacity/len fields to match UniFFI's C ABI on all platforms. Using size_t causes stack corruption on 32-bit ARM (armeabi-v7a) where size_t is 4 bytes but Rust expects 8-byte u64. Fixes crash in constructor marshalling on Amazon Fire TV. References: - https://mozilla.github.io/uniffi-rs/0.27/internals/api/uniffi/struct.RustBuffer.html - mozilla/uniffi-rs#2681
6d2fa96 to
adfb68d
Compare
…ility The pathdiff crate generates 'cpp/..' when calculating relative paths from android/ to cpp/, but CMake and build tools prefer the canonical '../cpp' form. This fix applies a simple string replacement to normalize the paths, matching the behavior of the existing workaround script. Fixes: jhugman#315
adfb68d to
20b48c5
Compare
Update: Simplified CMake path fix after testingInitial implementation used complex path component normalization logic, but testing revealed a simpler approach works better and matches existing community workaround patterns. Changes in latest commit:
The simpler approach is more maintainable and aligns with how existing users have been working around this issue in their build scripts. |
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.
There are two bug fixes here, so I'd like to see two PRs. The first with RufferBuffer is an instant approve. Thanks.
The second, I'm really not sure about.
If you could drop commit 20b48c5 from this we can deal with the two fixes at different speeds.
| size_t capacity; | ||
| size_t len; | ||
| uint64_t capacity; | ||
| uint64_t len; |
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.
Oh wow, this is an excellent catch. 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.
Thanks. Yeah this crashed on me running in 32 bit. Required quite some digging to find!
| // This happens when calculating paths between sibling directories (e.g., android/ to cpp/) | ||
| // CMake and other tools prefer the "../dir" form over "dir/.." even though both work | ||
| let path_str = path.as_str().replace("cpp/..", "../cpp"); | ||
| Utf8PathBuf::from(path_str) |
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. I'm not sure if this works accidentally, or if this doesn't work.
cpp/.. decends down into a directory called cpp and then comes back to where we started. It fails if there isn't a child directory of the pwd called cpp.
../cpp ascends to the parent of the pwd, and iff there is a sibling directory called cpp, then descends into it. If the pwd is called cpp, then we're back where we started.
Then, I'm also not sure where you're getting the cpp from?
Would you be able to cherry pick? Or let me see what I can do but it might take me a few days because I'm using locally a fork of this for this to work with my platform. I'm quite busy. I know that without this I couldn't use it and it was writing in the wrong place. I spent over a day trying to figure out how to make it work. At least for me it's cleaner and now it works consistently. |
|
I am having the same issue with the paths. I am having to manually change the paths in the CMakeLists.txt file every time I generate code. |
Summary
This PR fixes two bugs discovered during Android builds with 32-bit ARM (armeabi-v7a):
Bug 1: RustBuffer 32-bit ARM Crash (Critical)
Problem
RustBufferstruct usessize_tfor capacity/len fieldsuint64_t(8 bytes) on ALL platformssize_tis 4 bytes → ABI mismatch → stack corruption → crashRoot Cause
The crash occurred in constructor marshalling on Amazon Fire TV (armeabi-v7a) before any Rust code ran. The C++ JSI wrapper called
uniffi_metric_engine_wasm_fn_constructor_metricengine_newwith aRustBufferparameter, but the field size mismatch caused stack corruption.Impact
Fix
Changed
RustBufferstruct definition incpp/includes/RustBuffer.hto useuint64_texplicitly for capacity/len fields, matching UniFFI's documented C ABI specification.File changed:
cpp/includes/RustBuffer.hBug 2: Non-Canonical CMake Paths
Problem
While testing the RustBuffer fix, discovered that generated
android/CMakeLists.txtuses non-canonical paths likecpp/../file.cppinstead of../cpp/file.cpp.Impact
Fix
Added path normalization to
relative_to()function incrates/ubrn_cli/src/codegen/mod.rsthat lexically simplifies paths by eliminatingdir/..patterns.File changed:
crates/ubrn_cli/src/codegen/mod.rsTesting
References
RustBufferByReferencemisreports its size mozilla/uniffi-rs#2681