khoren93 / flutter_zxing

Flutter plugin for scanning and generating QR codes using the ZXing library, supporting Android, iOS, and desktop platforms
https://pub.dev/packages/flutter_zxing
MIT License
97 stars 56 forks source link

ffi: fix crashes from use-after-free. fix memory leaks. correctness cleanups. #126

Closed phlip9 closed 7 months ago

phlip9 commented 8 months ago

Overview

We were seeing occasional crashes/SEGFAULTs from inside this ffi code (esp. on iOS/macOS). To combat this, I took a pass at fixing up the use-after-free bugs and memory leaks, along with some other correctness-focused cleanups.

Bug fixes:

Correctness-focused cleanups:

Note for reviewer

This PR is easiest to review by-commit.

Example use-after-free

Use-after-free's are nasty as they only sometimes crash the app.

For an example of this bug, let's check out the use-after-free in native_zxing.cpp::encodeBarcode:

//                                       vvvvvvv
result.data = ToMatrix<int8_t>(bitMatrix).data();

Here we're returning a pointer to the data backing a std::vector<int8_t>, which is dropped before the function scope returns. Across the FFI boundary, when dart tries to access the data behind this pointer, the runtime may SEGFAULT since this memory has already been freed and is no longer valid.

The fix here is to copy the data out of the std::vector for Dart to free once it's done with it. Sadly there doesn't seem to be any easy way to intentionally stop a std::vector from running its destructor, so we have to copy.

auto matrix = ToMatrix<int8_t>(bitMatrix);
result.data = new int8_t[matrix.size()];
std::copy(matrix.begin(), matrix.end(), result.data);

Example memory leak

Memory leaks are definitely less problematic since it won't crash the app, but leaking image buffers definitely adds up.

In this example, we're allocating an owned char* text but nobody ever frees it.

void resultToCodeResult(struct CodeResult *code, Result result)
{
  string text = result.text();
  code->text = new char[text.length() + 1]; // <<<<<<< allocation
  strcpy(code->text, text.c_str());
  // ...
}
extension CodeExt on CodeResult {
  Code toCode() {
    return Code(
      //                                       but no free
      //                             vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
      text: text == nullptr ? null : text.cast<Utf8>().toDartString(),
      // ...
    );
  }
}
phlip9 commented 7 months ago

gentle cc @khoren93

khoren93 commented 7 months ago

Hey @phlip9,

I wanted to take a moment to say thanks for your recent pull request. Your contribution is awesome and we're grateful for it.

Thanks a bunch!

Best, Khoren