Fix validation of empty maps/arrays at end of metadata#422
Fix validation of empty maps/arrays at end of metadata#422
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the MaxMind DB library where databases containing empty maps or arrays as the final metadata fields were erroneously marked as invalid. The fix corrects an off-by-one error in the data section size validation, ensuring that such valid databases can now be opened and processed successfully. This improves the robustness and compatibility of the library with a broader range of MMDB files. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 fixes an off-by-one error in map/array size validation that incorrectly rejected valid databases with empty containers at the end of the metadata. The change correctly adjusts the boundary checks from >= to > to allow for this case. New tests are added to verify the fix for both empty maps and arrays. The changes look correct and effectively address the issue. I've added one suggestion to refactor the new test code to reduce duplication.
t/empty_container_metadata_t.c
Outdated
| void test_empty_map_last_in_metadata(void) { | ||
| char *db_file = | ||
| bad_database_path("libmaxminddb-empty-map-last-in-metadata.mmdb"); | ||
|
|
||
| MMDB_s mmdb; | ||
| int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); | ||
| cmp_ok(status, | ||
| "==", | ||
| MMDB_SUCCESS, | ||
| "opened MMDB with empty map at end of metadata"); | ||
|
|
||
| if (status != MMDB_SUCCESS) { | ||
| diag("MMDB_open failed: %s", MMDB_strerror(status)); | ||
| free(db_file); | ||
| return; | ||
| } | ||
|
|
||
| int gai_error, mmdb_error; | ||
| MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error); | ||
| cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); | ||
|
|
||
| MMDB_close(&mmdb); | ||
| free(db_file); | ||
| } | ||
|
|
||
| void test_empty_array_last_in_metadata(void) { | ||
| char *db_file = | ||
| bad_database_path("libmaxminddb-empty-array-last-in-metadata.mmdb"); | ||
|
|
||
| MMDB_s mmdb; | ||
| int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb); | ||
| cmp_ok(status, | ||
| "==", | ||
| MMDB_SUCCESS, | ||
| "opened MMDB with empty array at end of metadata"); | ||
|
|
||
| if (status != MMDB_SUCCESS) { | ||
| diag("MMDB_open failed: %s", MMDB_strerror(status)); | ||
| free(db_file); | ||
| return; | ||
| } | ||
|
|
||
| int gai_error, mmdb_error; | ||
| MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error); | ||
| cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded"); | ||
|
|
||
| MMDB_close(&mmdb); | ||
| free(db_file); | ||
| } |
There was a problem hiding this comment.
The two test functions test_empty_map_last_in_metadata and test_empty_array_last_in_metadata are nearly identical. To improve maintainability and reduce code duplication, you could extract the common logic into a helper function. This would make the tests cleaner and easier to manage in the future.
static void test_db_opens_and_lookups(const char *filename, const char *description) {
char *db_file = bad_database_path(filename);
MMDB_s mmdb;
int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb);
cmp_ok(status, "==", MMDB_SUCCESS, description);
if (status != MMDB_SUCCESS) {
diag("MMDB_open failed: %s", MMDB_strerror(status));
free(db_file);
return;
}
int gai_error, mmdb_error;
MMDB_lookup_string(&mmdb, "1.2.3.4", &gai_error, &mmdb_error);
cmp_ok(mmdb_error, "==", MMDB_SUCCESS, "lookup succeeded");
MMDB_close(&mmdb);
free(db_file);
}
void test_empty_map_last_in_metadata(void) {
test_db_opens_and_lookups("libmaxminddb-empty-map-last-in-metadata.mmdb",
"opened MMDB with empty map at end of metadata");
}
void test_empty_array_last_in_metadata(void) {
test_db_opens_and_lookups("libmaxminddb-empty-array-last-in-metadata.mmdb",
"opened MMDB with empty array at end of metadata");
}
The size validation added in c1d353a to prevent DoS via exaggerated map/array sizes used >= when checking offset against data_section_size. This rejected valid 0-length containers when they were the last field in metadata (offset_to_next == data_section_size). Change to > since the existing second condition (size > remaining) already correctly handles the boundary case for non-empty containers. Reported in vimt/MaxMind-DB-Writer-python#16. ENG-4322 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This too, though I don't know that it especially matters (Codex this time): |
2 participants
Summary
get_entry_data_list()(added in c1d353a) that rejected valid 0-length containers at the end of the metadata sectionMMDB_openwould returnMMDB_INVALID_DATA_ERRORfor databases wheredescription(empty map) orlanguages(empty array) was the last metadata field>=to>in both the array and map bounds checks — the existing second condition already correctly handles non-empty containers at the boundaryTest plan
empty_container_metadata_ttest confirms both empty-map-last and empty-array-last databases open and look up successfullybad_data_size_ttest still rejects oversized arrays/maps (DoS protection preserved)Fixes ENG-4322. Reported in vimt/MaxMind-DB-Writer-python#16.
Submodule PR: maxmind/MaxMind-DB#222
🤖 Generated with Claude Code