-
Notifications
You must be signed in to change notification settings - Fork 137
Fix: Check flatbuffer integrity before parsing #1864
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
base: main
Are you sure you want to change the base?
Changes from all commits
7193a18
c50d721
dde589e
1dde56a
0202676
51f3395
8938c3b
9850a4c
aa64115
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -250,6 +250,10 @@ Variant FlexbufferToVariant(const flexbuffers::Reference& ref) { | |||||||||||||||||
| case flexbuffers::FBT_BLOB: | ||||||||||||||||||
| LogError("Flexbuffers containing blobs are not supported."); | ||||||||||||||||||
| break; | ||||||||||||||||||
| case flexbuffers::FBT_MAX_TYPE: | ||||||||||||||||||
| LogError("Unknown or unsupported flexbuffer type: %d", | ||||||||||||||||||
| static_cast<int>(ref.GetType())); | ||||||||||||||||||
| break; | ||||||||||||||||||
|
Comment on lines
+253
to
+256
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using Using a
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| return Variant::Null(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -103,6 +103,10 @@ Variant FlexbufferToVariant(const flexbuffers::Reference& ref) { | |||||||||||||||||
| case flexbuffers::FBT_BLOB: | ||||||||||||||||||
| LogError("Flexbuffers containing blobs are not supported."); | ||||||||||||||||||
| break; | ||||||||||||||||||
| case flexbuffers::FBT_MAX_TYPE: | ||||||||||||||||||
| LogError("Unknown or unsupported flexbuffer type: %d", | ||||||||||||||||||
| static_cast<int>(ref.GetType())); | ||||||||||||||||||
| break; | ||||||||||||||||||
|
Comment on lines
+106
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using Using a
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| return Variant::Null(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,11 +36,16 @@ namespace internal { | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| RemoteConfigFileManager::RemoteConfigFileManager(const std::string& filename, | ||||||||||||||||||||||||||
| const firebase::App& app) { | ||||||||||||||||||||||||||
| const char* package_name = app.options().package_name(); | ||||||||||||||||||||||||||
| 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(package_name ? package_name : "") + "/remote_config"; | ||||||||||||||||||||||||||
|
Comment on lines
+39
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When This inconsistency can cause path resolution issues in
Suggested change
|
||||||||||||||||||||||||||
| std::string error; | ||||||||||||||||||||||||||
| std::string app_dir = | ||||||||||||||||||||||||||
| AppDataDir(app_data_prefix.c_str(), /*should_create=*/true, &error); | ||||||||||||||||||||||||||
| std::string file_path; | ||||||||||||||||||||||||||
| if (error.empty() && !app_dir.empty()) { | ||||||||||||||||||||||||||
| file_path = app_dir + "/" + app.name() + "_" + filename; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| #if FIREBASE_PLATFORM_WINDOWS | ||||||||||||||||||||||||||
| std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> utf8_to_wstring; | ||||||||||||||||||||||||||
| file_path_ = utf8_to_wstring.from_bytes(file_path); | ||||||||||||||||||||||||||
|
|
@@ -50,16 +55,28 @@ RemoteConfigFileManager::RemoteConfigFileManager(const std::string& filename, | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| bool RemoteConfigFileManager::Load(LayeredConfigs* configs) const { | ||||||||||||||||||||||||||
| if (file_path_.empty()) { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| std::fstream input(file_path_, std::ios::in | std::ios::binary); | ||||||||||||||||||||||||||
| if (!input) { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| std::stringstream ss; | ||||||||||||||||||||||||||
| ss << input.rdbuf(); | ||||||||||||||||||||||||||
| configs->Deserialize(ss.str()); | ||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| bool RemoteConfigFileManager::Save(const LayeredConfigs& configs) const { | ||||||||||||||||||||||||||
| if (file_path_.empty()) { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| std::string buffer = configs.Serialize(); | ||||||||||||||||||||||||||
| std::fstream output(file_path_, std::ios::out | std::ios::binary); | ||||||||||||||||||||||||||
| if (!output) { | ||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| output.write(buffer.c_str(), buffer.size()); | ||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -81,6 +81,10 @@ Variant FlexbufferToVariant(const flexbuffers::Reference& ref) { | |||||||||||||||||
| case flexbuffers::FBT_BLOB: | ||||||||||||||||||
| LogError("Flexbuffers containing blobs are not supported."); | ||||||||||||||||||
| break; | ||||||||||||||||||
| case flexbuffers::FBT_MAX_TYPE: | ||||||||||||||||||
| LogError("Unknown or unsupported flexbuffer type: %d", | ||||||||||||||||||
| static_cast<int>(ref.GetType())); | ||||||||||||||||||
| break; | ||||||||||||||||||
|
Comment on lines
+84
to
+87
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using Using a
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| return Variant::Null(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| From: Antigravity <antigravity@google.com> | ||
| Subject: Patch flatbuffers to resolve ERROR macro conflict on Windows | ||
|
|
||
| Workaround for conflict between ProtoIdGapAction::ERROR in idl.h and the | ||
| Windows global ERROR macro defined in wingdi.h/windows.h. | ||
|
|
||
| See Flatbuffers Issue: https://github.com/google/flatbuffers/issues/8483 | ||
|
|
||
| diff --git a/include/flatbuffers/idl.h b/include/flatbuffers/idl.h | ||
| index 95fda8c2..6da1c6e3 100644 | ||
| --- a/include/flatbuffers/idl.h | ||
| +++ b/include/flatbuffers/idl.h | ||
| @@ -17,6 +17,12 @@ | ||
| #ifndef FLATBUFFERS_IDL_H_ | ||
| #define FLATBUFFERS_IDL_H_ | ||
|
|
||
| +#ifdef ERROR | ||
| +#pragma push_macro("ERROR") | ||
| +#undef ERROR | ||
| +#define FLATBUFFERS_POP_ERROR_MACRO | ||
| +#endif | ||
| + | ||
| #include <algorithm> | ||
| #include <functional> | ||
| #include <map> | ||
| @@ -1325,4 +1331,9 @@ extern bool GenerateTSGRPC(const Parser& parser, const std::string& path, | ||
| const std::string& file_name); | ||
| } // namespace flatbuffers | ||
|
|
||
| +#ifdef FLATBUFFERS_POP_ERROR_MACRO | ||
| +#pragma pop_macro("ERROR") | ||
| +#undef FLATBUFFERS_POP_ERROR_MACRO | ||
| +#endif | ||
| + | ||
| #endif // FLATBUFFERS_IDL_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing the result of
GenerateTexttonullptris incorrect.GenerateTextreturns abool(indicating success or failure), not a pointer.Comparing a
booltonullptrwill either fail to compile in modern C++ or result in inverted logic (wheregenerate_statusbecomesfalseon success andtrueon failure), causing the subsequentFIREBASE_ASSERT_RETURN_VOID(generate_status)to trigger on successful execution.We should revert this to simply assign the
boolresult ofGenerateTextdirectly.