Fix: Check flatbuffer integrity before parsing#1864
Conversation
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.
❌ Integration test FAILEDRequested by @AustinBenoit on commit 818d66e
Add flaky tests to go/fpl-cpp-flake-tracker |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates Flatbuffers to support buffer verification, adds integrity checks to prevent crashes on invalid data, and improves path resolution and error handling in Remote Config. The reviewer feedback highlights a critical bug where a boolean return value is incorrectly compared to nullptr, an issue with an unintended leading slash when the package name is empty, and suggests using default cases instead of case flexbuffers::FBT_MAX_TYPE to robustly handle corrupted or unknown flexbuffer types.
Updated flatbuffer to latest version to get verify buffer Use strol for key parsing to ensure exceptions do not result in a crash.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates Flatbuffers to a newer version, introduces buffer verification checks before deserializing flexbuffers, and hardens Remote Config desktop code with robust error and bounds checking. Feedback on the changes includes addressing a potential absolute path issue when package_name is empty or null, and adding a null pointer check for the configs parameter in RemoteConfigFileManager::Load to prevent crashes.
Wiz Scan Summary
|
| Scanner | Findings |
|---|---|
| 25 |
|
| - | |
| - | |
| - | |
| - | |
| - | |
| Total | 25 |
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.
Description
Testing
Integration test in github
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.