Skip to content

Commit

Permalink
Fix unterminated string causing flaky behavior.
Browse files Browse the repository at this point in the history
Fixes shorebirdtech/shorebird#235
The root cause was we were not terminating the strings passed
for original_libapp_paths.  We used a Vector<char> but didn't
append a '\0' to the end before calling data(), so when the c/rust
side walked to find the end of the char* it sometimes found extra
characters at the end of the string unexpectedly.

This fixes to use a shorter (but probably equally dangerous?)
method of using c_str() which we had tried before but previously
had used c_str() on a temporary.  This attempt carefully uses
a reference to the string so the c_str()'s lifetime is tied to
the string in application_library_paths rather than a temporary
copy.

This is still slightly dangerous in that c_str() lives as long
as a std::string does, so long as it's not modified.
  • Loading branch information
eseidel committed Apr 7, 2023
1 parent 978a56f commit 48c1ea6
Showing 1 changed file with 18 additions and 24 deletions.
42 changes: 18 additions & 24 deletions shell/platform/android/flutter_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,33 +104,27 @@ void ConfigureShorebird(std::string android_cache_path,
fml::CreateDirectory(fml::paths::GetCachesDirectory(), {"shorebird_updater"},
fml::FilePermission::kReadWrite);

AppParameters app_parameters;
app_parameters.release_version = version.c_str();
app_parameters.cache_dir = cache_dir.c_str();

// Converting the strings in `settings.application_library_path` to `char*`.
// We maintain a separate vector so that the lifetime of the `char*` is clear.
// If we used `c_str` then we would have to be very careful not to modify the
// string or have taken a copy of the string as `c_str` is only valid for the
// lifetime of the string.
std::vector<std::vector<char>> libapp_paths_storage;
std::vector<const char*> original_libapp_paths;
for (auto& path : settings.application_library_path) {
libapp_paths_storage.push_back(std::vector<char>(path.begin(), path.end()));
original_libapp_paths.push_back(libapp_paths_storage.back().data());
}
// Putting initialization into a block to make AppParameters lifetime clear.
{
AppParameters app_parameters;
app_parameters.release_version = version.c_str();
app_parameters.cache_dir = cache_dir.c_str();

// https://stackoverflow.com/questions/26032039/convert-vectorstring-into-char-c
std::vector<const char*> c_paths{};
for (const auto& string : settings.application_library_path) {
c_paths.push_back(string.c_str());
}
// Do not modify application_library_path or cstrings will be invalid.

app_parameters.original_libapp_paths = original_libapp_paths.data();
app_parameters.original_libapp_paths_size = original_libapp_paths.size();
app_parameters.original_libapp_paths = c_paths.data();
app_parameters.original_libapp_paths_size = c_paths.size();

// TODO: How do can we get the path to libflutter.so?
// The Rust side doesn't actually use this yet. The intent is so
// that Rust could hash it, or otherwise know that it matches the
// version the patch is intended for, but currently that's not
// implemented.
app_parameters.vm_path = "libflutter.so";
app_parameters.vm_path = "libflutter.so"; // Unused.

shorebird_init(&app_parameters, shorebirdYaml.c_str());
// shorebird_init copies from app_parameters and shorebirdYaml.
shorebird_init(&app_parameters, shorebirdYaml.c_str());
}

FML_LOG(INFO) << "Starting Shorebird update";
shorebird_update();
Expand Down

0 comments on commit 48c1ea6

Please sign in to comment.