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

@@ -1,19 +1,5 @@
-// Required for mkstemp visibility under -D_POSIX_C_SOURCE=200112L on glibc.
-#define _XOPEN_SOURCE 500
-
 #include "maxminddb_test_helper.h"
 
-#include <errno.h>
-#include <string.h>
-
-#ifdef _WIN32
-    #include <io.h>
-    #include <stdio.h>
-    #define unlink _unlink
-#else
-    #include <unistd.h>
-#endif
-
 /*
  * Test the off-by-one fix in MMDB_read_node: node_number >= node_count
  * must return MMDB_INVALID_NODE_NUMBER_ERROR. Previously the check used
@@ -55,114 +41,60 @@ void test_read_node_bounds(void) {
     free(mmdb);
 }
 
-static FILE *open_temp_file(char *path, size_t path_size) {
-#ifdef _WIN32
-    errno_t err = tmpnam_s(path, path_size);
-    if (err != 0) {
-        return NULL;
-    }
-    return fopen(path, "wb");
-#else
-    snprintf(path, path_size, "./bad-search-tree-XXXXXX");
-    int fd = mkstemp(path);
-    if (fd < 0) {
-        return NULL;
-    }
-    return fdopen(fd, "wb");
-#endif
-}
-
-static int copy_file(FILE *dest, const char *source_path) {
-    FILE *source = fopen(source_path, "rb");
-    if (!source) {
-        return -1;
-    }
-
-    unsigned char buffer[4096];
-    size_t bytes_read;
-    while ((bytes_read = fread(buffer, 1, sizeof(buffer), source)) > 0) {
-        if (fwrite(buffer, 1, bytes_read, dest) != bytes_read) {
-            fclose(source);
-            return -1;
-        }
-    }
-
-    int result = ferror(source) ? -1 : 0;
-    fclose(source);
-    return result;
-}
-
-void test_separator_record_rejected(void) {
-    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));
-    if (!temp) {
-        free(db_file);
-        BAIL_OUT("could not create temp file: %s", strerror(errno));
-    }
-
-    if (copy_file(temp, db_file) != 0) {
-        fclose(temp);
-        unlink(temp_path);
-        free(db_file);
-        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) {
-        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)) {
-        fclose(temp);
-        unlink(temp_path);
-        free(db_file);
-        BAIL_OUT("fwrite failed: %s", strerror(errno));
-    }
-    if (fclose(temp) != 0) {
-        unlink(temp_path);
-        free(db_file);
-        BAIL_OUT("fclose failed: %s", strerror(errno));
-    }
-
+// Open a pre-built corruption fixture and assert that all rejection paths
+// fire. `lookup_ip` must traverse the corrupted record from node 0 — high
+// bit 0 for a corrupted left record, 1 for a corrupted right record.
+static void check_corrupt_record(const char *label,
+                                 const char *fixture,
+                                 const char *lookup_ip) {
+    char *db_file = bad_database_path(fixture);
     MMDB_s mmdb;
-    int status = MMDB_open(temp_path, MMDB_MODE_MMAP, &mmdb);
-    cmp_ok(status, "==", MMDB_SUCCESS, "opened crafted bad search tree MMDB");
+    int status = MMDB_open(db_file, MMDB_MODE_MMAP, &mmdb);
+    cmp_ok(status, "==", MMDB_SUCCESS, "%s: opened crafted bad MMDB", label);
     if (status != MMDB_SUCCESS) {
-        unlink(temp_path);
         free(db_file);
         return;
     }
 
     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) {
+    // Records in the half-open range [node_count + 1, node_count + 16) point
+    // into the 16-byte separator between the search tree and data section
+    // and must be rejected as corrupt.
+    check_corrupt_record("left record, first separator byte",
+                         "libmaxminddb-separator-record-min-left.mmdb",
+                         "1.1.1.1");
+    check_corrupt_record("right record, first separator byte",
+                         "libmaxminddb-separator-record-min-right.mmdb",
+                         "128.0.0.0");
+    check_corrupt_record("left record, last separator byte",
+                         "libmaxminddb-separator-record-max-left.mmdb",
+                         "1.1.1.1");
+}
+
 int main(void) {
     plan(NO_PLAN);
     test_read_node_bounds();