Skip to content
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

ffi: linux support. ci: add build + integration test to CI. #144

Merged
merged 14 commits into from
Jun 3, 2024

Conversation

phlip9
Copy link
Contributor

@phlip9 phlip9 commented Jun 1, 2024

Hey again! Back with another pass at fixing up the native glue code. I recently needed to run our app on a Linux machine and encountered a crash with flutter_zxing whenever it called encodeBarcode. With this PR, I can now confidently run flutter_zxing on Linux w/o issues!

Overview

This PR:

  1. Adds support for Linux (minus camera support, of course). Had to fix some SEGFAULTs in the native code to get this working.
  2. Adds an integration test exercising the FFI code.
  3. Adds flutter build and flutter test integration_test Github CI workflows running across android, iOS, macOS, and linux. Windows build fails to compile though.
  4. Enforces strict allocator discipline in the native code. Memory freed in C++ from Dart must use dart_free. Memory freed in Dart from C++ must be allocated with dart_malloc.
  5. Tries hard to ensure we don't throw/unwind exceptions across the FFI boundary, which is unsafe. Public FFI functions are marked noexcept.

I recommend reviewing by commit.

phlip9 added 13 commits May 29, 2024 19:01
Without this fix, the Linux build segfaults when calling `readBarcode(s)`
or `encodeBarcode`.

I did some investigation in gdb and it's not totally clear what's going on.
The `param` arg usually looks corrupted -- maybe there's some calling
convention mismatch though that seems unlikely.

More likely, the pointer backing the value returned from e.g.
`toEncodeBarcodeParams` is getting freed immediately; maybe dart has some
new short-liveness scope rule.

Anyway passing the params as a basic owned pointer works.
Adds a `unique_dart_ptr` smart pointer that can safely take ownership of
and free Dart-allocated pointers.
The backing memory behind `EncodeResult::text` is free'd when
`EncodeBarcodeParams` is dropped, causing use-after-free when Dart tries
to UTF-8 encode the text later on.
`dart_allocator` is a `std::allocator` impl that can:

(1) safely allocate memory in C++ that is later freed inside the Dart VM
(2) safely free memory in C++ that was earlier allocated inside the Dart VM

Returning allocated memory to Dart or freeing memory allocated from within
Dart in C++ must be done _very carefully_. For this to be safe, both C++ and
Dart must use the same allocator on each side. Since C++ may use a different
allocator for `new`/`free`/other std types:

* Manage all memory from/to dart using `dart_allocator` (in `dart_alloc.h`).
* Avoid returning memory from `new` to Dart.
* Avoid freeing memory from Dart with `delete`.
@phlip9
Copy link
Contributor Author

phlip9 commented Jun 1, 2024

cc @khoren93

@phlip9 phlip9 force-pushed the phlip9/fix-linux branch from 4376983 to 76566fe Compare June 1, 2024 22:05
@khoren93 khoren93 merged commit 5581911 into khoren93:main Jun 3, 2024
9 checks passed
@khoren93
Copy link
Owner

khoren93 commented Jun 3, 2024

Hey @phlip9

Thanks for the great work on fixing up the native glue code and adding Linux support. The PR has been merged!

@phlip9 phlip9 deleted the phlip9/fix-linux branch June 4, 2024 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants