Return an error for invalid search nodes

Author: oschwald

Changes.md

@@ -11,6 +11,9 @@
 - Fixed search-tree validation for records that point into the 16-byte separator
   before the data section. These records are now rejected as corrupt instead of
   being exposed as apparent data entries with underflowed offsets.
+- `MMDB_read_node()` now returns `MMDB_CORRUPT_SEARCH_TREE_ERROR` instead of
+  `MMDB_SUCCESS` with `MMDB_RECORD_TYPE_INVALID` record types when a node's
+  child record is invalid.
 
 ## 1.13.3 - 2026-03-05
 

doc/libmaxminddb.md

@@ -773,7 +773,11 @@ to an `MMDB_search_node_s` structure that will be populated by this function.
 
 The return value is a status code. If you pass a `node_number` that is greater
 than or equal to the number of nodes in the database, this function will return
-`MMDB_INVALID_NODE_NUMBER_ERROR`, otherwise it will return `MMDB_SUCCESS`.
+`MMDB_INVALID_NODE_NUMBER_ERROR`. If the node's child records are invalid or
+point outside the search tree or data section, it will return
+`MMDB_CORRUPT_SEARCH_TREE_ERROR`. Otherwise it will return `MMDB_SUCCESS`. On
+any non-`MMDB_SUCCESS` return the contents of `*node` are unspecified and must
+not be read.
 
 The first node in the search tree is always node 0. If you wanted to iterate
 over the whole search tree, you would start by reading node 0 and then following

src/maxminddb.c

@@ -1143,6 +1143,11 @@ int MMDB_read_node(const MMDB_s *const mmdb,
     node->left_record_type = record_type(mmdb, node->left_record);
     node->right_record_type = record_type(mmdb, node->right_record);
 
+    if (node->left_record_type == MMDB_RECORD_TYPE_INVALID ||
+        node->right_record_type == MMDB_RECORD_TYPE_INVALID) {
+        return MMDB_CORRUPT_SEARCH_TREE_ERROR;
+    }
+
     // Note that offset will be invalid if the record type is not
     // MMDB_RECORD_TYPE_DATA, but that's ok. Any use of the record entry
     // for other data types is a programming error.

t/bad_search_tree_t.c

@@ -92,7 +92,16 @@ static int copy_file(FILE *dest, const char *source_path) {
     return result;
 }
 
-void test_separator_record_rejected(void) {
+// Mutate node 0 of a copy of the IPv4 test database so that one of its
+// 24-bit records holds `record_value`, then assert that all corruption-
+// rejection paths fire. `record_offset` is 0 for the left record or 3
+// for the right record. `lookup_ip` must traverse the corrupted record
+// from node 0 (high bit 0 for left, 1 for right). `label` identifies
+// the case in TAP output.
+static void check_corrupt_record(const char *label,
+                                 long record_offset,
+                                 unsigned char record_value_be[3],
+                                 const char *lookup_ip) {
     char *db_file = test_database_path("MaxMind-DB-test-ipv4-24.mmdb");
     char temp_path[64];
     FILE *temp = open_temp_file(temp_path, sizeof(temp_path));
@@ -108,16 +117,13 @@ void test_separator_record_rejected(void) {
         BAIL_OUT("could not copy test database: %s", strerror(errno));
     }
 
-    // Overwrite node 0's left record with node_count + 1, which points into
-    // the 16-byte separator between the search tree and data section.
-    if (fseek(temp, 0, SEEK_SET) != 0) {
+    if (fseek(temp, record_offset, SEEK_SET) != 0) {
         fclose(temp);
         unlink(temp_path);
         free(db_file);
         BAIL_OUT("fseek failed: %s", strerror(errno));
     }
-    unsigned char bad_record[3] = {0x00, 0x00, 0xA4};
-    if (fwrite(bad_record, 1, sizeof(bad_record), temp) != sizeof(bad_record)) {
+    if (fwrite(record_value_be, 1, 3, temp) != 3) {
         fclose(temp);
         unlink(temp_path);
         free(db_file);
@@ -131,7 +137,7 @@ void test_separator_record_rejected(void) {
 
     MMDB_s mmdb;
     int status = MMDB_open(temp_path, MMDB_MODE_MMAP, &mmdb);
-    cmp_ok(status, "==", MMDB_SUCCESS, "opened crafted bad search tree MMDB");
+    cmp_ok(status, "==", MMDB_SUCCESS, "%s: opened crafted bad MMDB", label);
     if (status != MMDB_SUCCESS) {
         unlink(temp_path);
         free(db_file);
@@ -140,29 +146,48 @@ void test_separator_record_rejected(void) {
 
     MMDB_search_node_s node;
     status = MMDB_read_node(&mmdb, 0, &node);
-    cmp_ok(
-        status, "==", MMDB_SUCCESS, "MMDB_read_node succeeds for crafted node");
-    cmp_ok(node.left_record_type,
+    cmp_ok(status,
            "==",
-           MMDB_RECORD_TYPE_INVALID,
-           "MMDB_read_node marks separator record as invalid");
+           MMDB_CORRUPT_SEARCH_TREE_ERROR,
+           "%s: MMDB_read_node rejects record as corrupt",
+           label);
 
     int gai_error = 0;
     int mmdb_error = 0;
     MMDB_lookup_result_s result =
-        MMDB_lookup_string(&mmdb, "1.1.1.1", &gai_error, &mmdb_error);
-    cmp_ok(gai_error, "==", 0, "lookup string parse succeeds");
+        MMDB_lookup_string(&mmdb, lookup_ip, &gai_error, &mmdb_error);
+    cmp_ok(gai_error, "==", 0, "%s: lookup string parse succeeds", label);
     cmp_ok(mmdb_error,
            "==",
            MMDB_CORRUPT_SEARCH_TREE_ERROR,
-           "MMDB_lookup_string rejects records pointing into the separator");
-    ok(!result.found_entry, "lookup does not report an entry for corrupt tree");
+           "%s: MMDB_lookup_string rejects record",
+           label);
+    ok(!result.found_entry, "%s: lookup reports no entry", label);
 
     MMDB_close(&mmdb);
     unlink(temp_path);
     free(db_file);
 }
 
+void test_separator_record_rejected(void) {
+    // The IPv4 test database has node_count == 163 (0xA3). Records in the
+    // half-open range [node_count + 1, node_count + 16) point into the
+    // 16-byte data section separator and must be rejected as corrupt.
+    unsigned char first_separator_byte[3] = {0x00, 0x00, 0xA4};
+    unsigned char last_separator_byte[3] = {0x00, 0x00, 0xB2};
+
+    check_corrupt_record("left record, first separator byte",
+                         0,
+                         first_separator_byte,
+                         "1.1.1.1");
+    check_corrupt_record("right record, first separator byte",
+                         3,
+                         first_separator_byte,
+                         "128.0.0.0");
+    check_corrupt_record(
+        "left record, last separator byte", 0, last_separator_byte, "1.1.1.1");
+}
+
 int main(void) {
     plan(NO_PLAN);
     test_read_node_bounds();