Fix segfault when passing DartOpaque through ffi boundaries #2259

alexlapa · 2024-08-28T02:05:30Z

Changes

So, ive encountered a segfault on Android devices after upgrading to frb v2.2.

F/libc    (26466): Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x7fffffffffffffff in tid 26639 (1.ui), pid 26466
(a_jason_example)
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
Build fingerprint: 'Redmi/rosemary_eea/rosemary:13/TP1A.220624.014/V14.0.11.0.TKLEUXM:user/release-keys'
Revision: '0'
ABI: 'arm64'
Timestamp: 2024-08-28 04:17:38.660867792+0300
Process uptime: 7s
ZygotePid: 848
Cmdline: com.instrumentisto.medea_jason_example
pid: 26466, tid: 26639, name: 1.ui  >>> com.instrumentisto.medea_jason_example <<<
uid: 10320
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x7fffffffffffffff
    x0  7fffffffffffffff  x1  b400007859d20b00  x2  7fffffffffffffff  x3  0000000000000014
    x4  000000760000a431  x5  0000007859ce5200  x6  00000077fa0bb400  x7  00000077fa0bb000
    x8  7fffffffffffffff  x9  fffffffffffffff0  x10 0000000000000010  x11 0000000000000008
    x12 0000007859ce3830  x13 000000000781a8e0  x14 000000000781a4e0  x15 00000077ed6de080
    x16 0000000000000000  x17 b4000077fa075198  x18 00000077eaefe000  x19 000000785b4adddc
    x20 0000007606259759  x21 b4000077fa074a00  x22 0000007600008081  x23 00000076062596d9
    x24 0000007600008081  x25 00000077ed4fd000  x26 b4000077fa074a00  x27 0000007606592f70
    x28 0000000800000076  x29 00000077ed6de098
    lr  00000077b9459ae4  sp  00000077ed6ddfb0  pc  00000077b945f6ac  pst 0000000080001000
backtrace:
      #00 pc 000000000079a6ac
      /data/app/~~ffeMhpu5eN-Cb-D2BNeVnw==/com.instrumentisto.medea_jason_example-XtxdlZivlameAOYXlt7bTQ==/base.apk
      #01 pc 0000000000794ae0
      /data/app/~~ffeMhpu5eN-Cb-D2BNeVnw==/com.instrumentisto.medea_jason_example-XtxdlZivlameAOYXlt7bTQ==/base.apk
      #02 pc 000000000078ba5c
      /data/app/~~ffeMhpu5eN-Cb-D2BNeVnw==/com.instrumentisto.medea_jason_example-XtxdlZivlameAOYXlt7bTQ==/base.apk
      #03 pc 000000000078e818
      /data/app/~~ffeMhpu5eN-Cb-D2BNeVnw==/com.instrumentisto.medea_jason_example-XtxdlZivlameAOYXlt7bTQ==/base.apk
      #04 pc 000000000078e7ec
      /data/app/~~ffeMhpu5eN-Cb-D2BNeVnw==/com.instrumentisto.medea_jason_example-XtxdlZivlameAOYXlt7bTQ==/base.apk
      (dart_opaque_rust2dart_decode+12)
      #05 pc 00000000000081d4  [anon:dart-code]

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:

decodeDartOpaque toInt call 12970367442122235088 => 9223372036854775807

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

An issue to be fixed by this PR is listed above.
New tests are added to ensure new features are working. Please refer to this page to see how to add a test.
./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
If this PR adds/changes features, documentations (in the ./website folder) are updated.
CI is passing. Please refer to this page to see how to solve a failed CI.

Remark for PR creator

./frb_internal --help shows utilities for development.
If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

welcome · 2024-08-28T02:05:32Z

Hi! Thanks for opening this pull request! 😄

fzyzcjy · 2024-08-28T03:38:42Z

Great catch!

~~Btw, I wonder what platform are you using? Dart int is 64bit (Web is 32bit; but the pointers are also 32bit on web), so in non-web platforms the conversion seems should not convert to u32::max.~~

EDIT: I see the problem: It is truncated to 2^63 - 1. i.e. the raw pointer exceeds i64 (but within range of u64), and since Dart int is indeed i64, it is saturated.

(If you need I can provide some suggestions for e.g. what precommit subcommand to run to make CI happy)

alexlapa · 2024-09-02T08:12:25Z

(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 - BigInt.toInt() is not a problem and it works as expected (0xffffffff.toInt() == -1), its this toUnsigned(64) that breaks pointers. 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.

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 usize is a pretty common thing, and all usizes are treated with toUnsigned(64) on the Dart side it seems. Maybe there is a same issue with the dco codec?

Other than that i believe its ready for review.

fzyzcjy · 2024-09-02T14:44:29Z

Great job!

Yes, I agree it is the case when numbers is bigger than isize (still within usize range).

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 usize is a pretty common thing, and all usizes are treated with toUnsigned(64) on the Dart side it seems. Maybe there is a same issue with the dco codec?

That looks reasonable. To test cst & dco codec, you can (temporarily) set full_dep: true in config.

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 int (i64 on non-web). Thus curious what will happen when Dart sees an address like your case which is larger than maximum of i64...

Too busy and a bit tired now, thus will try to review this PR within a day :)

alexlapa · 2024-09-03T10:18:18Z

To test cst & dco codec, you can (temporarily) set full_dep: true in config.

So i've run some tests and returning DartOpaque from Rust to Dart works fine using dco even without this PR, so its not affected.

Btw, is it possible we somehow add some tests for this scenario?

Meh, i dont think there is an easy way to do this.

Running test on CI on a arm64-v8a Android will indirectly check for this issue, but thats not really a specific test. And that might be problematic since AFAIK there are some limitations of android emulator, i believe you cant run arm Android on x86 host since 27(?) API. Running that on a MacOS with Apple silicone might do the job but i never checked that.

Another option is implementing allocator(wrapping default one?) that will always return pointers that are bigger then i64::MAX. But im not really sure how to do that correctly.

Another option is not using MirTypePrimitive::Isize or MirTypePrimitive::Usize for this but maybe adding MirTypePrimitive::Ptr so we might add some tests for this type that will just check the hardcoded values. Like return u64::MAX from Rust and assert that its -1 in Dart. This way it would also make *const () a correct return for the Rust exported functions, which would be pretty cool.

Thus curious what will happen when Dart sees an address like your case which is larger than maximum of i64

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 u64::MAX, it will be translated to -1 in Dart, and translated back to the correct value when its back in Rust. As i previously mentioned it is toUnsigned(64).toInt() that is the problem:

0xffffffffffffffff.toInt() = -1
0xffffffffffffffff.b.toUnsigned(64) = 18446744073709551615
0xffffffffffffffff.b.toUnsigned(64).toInt() = 9223372036854775807

fzyzcjy · 2024-09-03T10:56:35Z

So i've run some tests and returning DartOpaque from Rust to Dart works fine using dco even without this PR, so its not affected.

Great!

Meh, i dont think there is an easy way to do this.

I also think so... The environment is not trivial to create, and the unit tests may not cover wide enough.

it will be translated to -1 in Dart, and translated back to the correct value when its back in Rust

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)

fzyzcjy

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!

fzyzcjy · 2024-09-09T11:59:01Z

Btw, feel free to ping me (or use request a review button) when ready!

fzyzcjy · 2024-09-10T01:56:05Z

Great! I will probably review within 24hours

fzyzcjy

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/ir/mir/ty/dart_opaque.rs

frb_codegen/src/library/codegen/generator/codec/sse/ty/dart_opaque.rs

welcome · 2024-09-11T00:41:36Z

Hi! Congrats on merging your first pull request! 🎉

fzyzcjy · 2024-09-11T00:41:43Z

Great job!

fzyzcjy · 2024-09-11T00:41:58Z

@all-contributors please add @alexlapa for code

allcontributors · 2024-09-11T00:42:08Z

@fzyzcjy

I've put up a pull request to add @alexlapa! 🎉

pass dart opaque pointers as isize to fit in dart int

d609441

alexlapa and others added 3 commits August 29, 2024 10:04

run precommit slow

7210c85

refactor

edb35ea

Merge branch 'master' into fix-ptr-arithmetics

7909b89

alexlapa marked this pull request as ready for review September 2, 2024 08:12

fzyzcjy reviewed Sep 3, 2024

View reviewed changes

alexlapa added 2 commits September 9, 2024 13:22

Merge branch 'refs/heads/master' into fix-ptr-arithmetics

11107c8

continue using usize on Rust side

1fd0ace

alexlapa requested a review from fzyzcjy September 9, 2024 13:50

fzyzcjy approved these changes Sep 10, 2024

View reviewed changes

fix review notes

5047ec7

fzyzcjy merged commit 6fa3179 into fzyzcjy:master Sep 11, 2024
135 checks passed

allcontributors bot mentioned this pull request Sep 11, 2024

docs: add alexlapa as a contributor for code #2286

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault when passing DartOpaque through ffi boundaries #2259

Fix segfault when passing DartOpaque through ffi boundaries #2259

alexlapa commented Aug 28, 2024 •

edited

Loading

welcome bot commented Aug 28, 2024

fzyzcjy commented Aug 28, 2024 •

edited

Loading

alexlapa commented Sep 2, 2024

fzyzcjy commented Sep 2, 2024 •

edited

Loading

alexlapa commented Sep 3, 2024

fzyzcjy commented Sep 3, 2024

fzyzcjy left a comment •

edited

Loading

fzyzcjy commented Sep 9, 2024

fzyzcjy commented Sep 10, 2024

fzyzcjy left a comment

welcome bot commented Sep 11, 2024

fzyzcjy commented Sep 11, 2024

fzyzcjy commented Sep 11, 2024

allcontributors bot commented Sep 11, 2024

Fix segfault when passing DartOpaque through ffi boundaries #2259

Fix segfault when passing DartOpaque through ffi boundaries #2259

Conversation

alexlapa commented Aug 28, 2024 • edited Loading

Changes

Checklist

Remark for PR creator

welcome bot commented Aug 28, 2024

fzyzcjy commented Aug 28, 2024 • edited Loading

alexlapa commented Sep 2, 2024

fzyzcjy commented Sep 2, 2024 • edited Loading

alexlapa commented Sep 3, 2024

fzyzcjy commented Sep 3, 2024

fzyzcjy left a comment • edited Loading

Choose a reason for hiding this comment

fzyzcjy commented Sep 9, 2024

fzyzcjy commented Sep 10, 2024

fzyzcjy left a comment

Choose a reason for hiding this comment

welcome bot commented Sep 11, 2024

fzyzcjy commented Sep 11, 2024

fzyzcjy commented Sep 11, 2024

allcontributors bot commented Sep 11, 2024

alexlapa commented Aug 28, 2024 •

edited

Loading

fzyzcjy commented Aug 28, 2024 •

edited

Loading

fzyzcjy commented Sep 2, 2024 •

edited

Loading

fzyzcjy left a comment •

edited

Loading