Fix: Check flatbuffer integrity before parsing#1864
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Flatbuffers dependency to a newer version, removes an obsolete patch file, and improves the robustness of the Remote Config desktop implementation. Specifically, it adds buffer verification before deserializing flexbuffers, enhances file path handling and error checking in the file manager, and replaces std::stoi with safer string-to-integer parsing in the metadata deserialization. The review feedback highlights critical improvements: ensuring robust overflow detection for std::strtol on LLP64 platforms (like Windows) by checking errno, preventing potential undefined behavior from a null package_name(), and explicitly including the necessary and headers.
| char* endptr = nullptr; | ||
| long raw_key = std::strtol(key_str, &endptr, 10); | ||
| if (endptr == key_str || *endptr != '\0') { | ||
| continue; | ||
| } | ||
| if (raw_key < std::numeric_limits<int>::min() || | ||
| raw_key > std::numeric_limits<int>::max()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
On platforms where sizeof(long) == sizeof(int) (such as Windows, which uses the LLP64 model where long is 32-bit), std::strtol will overflow and return LONG_MAX or LONG_MIN for values outside the 32-bit signed integer range. In these cases, raw_key will be equal to INT_MAX or INT_MIN, and the range checks raw_key < std::numeric_limits<int>::min() and raw_key > std::numeric_limits<int>::max() will fail to detect the overflow.
To correctly detect overflow on all platforms, you should clear errno before calling std::strtol and check if errno == ERANGE afterwards.
| char* endptr = nullptr; | |
| long raw_key = std::strtol(key_str, &endptr, 10); | |
| if (endptr == key_str || *endptr != '\0') { | |
| continue; | |
| } | |
| if (raw_key < std::numeric_limits<int>::min() || | |
| raw_key > std::numeric_limits<int>::max()) { | |
| continue; | |
| } | |
| char* endptr = nullptr; | |
| errno = 0; | |
| long raw_key = std::strtol(key_str, &endptr, 10); | |
| if (endptr == key_str || *endptr != '\\0' || errno == ERANGE) { | |
| continue; | |
| } | |
| if (raw_key < std::numeric_limits<int>::min() || | |
| raw_key > std::numeric_limits<int>::max()) { | |
| continue; | |
| } |
| std::string app_data_prefix = | ||
| std::string(app.options().package_name()) + "/" + app.name(); | ||
| std::string file_path = | ||
| AppDataDir(app_data_prefix.c_str(), /*should_create=*/true) + "/" + | ||
| filename; | ||
| std::string(app.options().package_name()) + "/remote_config"; |
There was a problem hiding this comment.
If app.options().package_name() returns nullptr, constructing a std::string directly from it will result in undefined behavior (typically a crash). It is safer to perform a null check before constructing the string.
| std::string app_data_prefix = | |
| std::string(app.options().package_name()) + "/" + app.name(); | |
| std::string file_path = | |
| AppDataDir(app_data_prefix.c_str(), /*should_create=*/true) + "/" + | |
| filename; | |
| std::string(app.options().package_name()) + "/remote_config"; | |
| const char* package_name = app.options().package_name(); | |
| std::string app_data_prefix = | |
| std::string(package_name ? package_name : "") + "/remote_config"; |
Updated flatbuffer to latest version to get verify buffer Use strol for key parsing to ensure exceptions do not result in a crash.
AustinBenoit
added
the
tests-requested: full
github-actions
Bot
added
tests: in-progress
❌ Integration test FAILEDRequested by @AustinBenoit on commit acf24f3
Add flaky tests to go/fpl-cpp-flake-tracker |
firebase-workflow-trigger
Bot
removed
the
tests: in-progress
1 participant
Description
Testing
Integration test in github
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.