diff --git a/HISTORY.rst b/HISTORY.rst index 50839c9a..ae86d464 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -3,6 +3,29 @@ History ------- +3.3.0 +++++++++++++++++++ + +* A second ``__init__`` on a pure Python ``Reader`` now closes the old + database, and an iterator from before it raises ``ValueError``, as in the + C extension. Before, the iterator walked the new database with node + numbers from the old one. A failed ``__init__`` keeps the old database. + After ``close()``, an iterator raises ``ValueError`` in every mode. +* C extension: + + * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and + the internal iterator type. + * Fixed a segmentation fault on a database with a map key that is not a + string. Such a database now raises ``InvalidDatabaseError``. + * Fixed large ``uint32`` values, which came back negative on platforms + with a 32-bit C ``long``, such as Windows. + * Fixed memory leaks and a use-after-free. A second ``__init__`` on a + ``Reader`` now closes the old database, and an iterator from before it + raises ``ValueError``. A failed ``__init__`` keeps the old database. + Reinitializing a ``Metadata`` changes nothing. + * Fixed a ``RuntimeWarning`` or ``RuntimeError`` on free-threaded Python on + macOS when a ``Reader`` failed to open or was used without ``__init__``. + 3.2.0 (2026-09-10) ++++++++++++++++++ diff --git a/extension/maxminddb.c b/extension/maxminddb.c index c2dd73e7..96679f17 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -50,10 +50,12 @@ typedef SRWLOCK reader_rwlock_t; #elif defined(MAXMINDDB_USE_PTHREAD_LOCKS) typedef pthread_rwlock_t reader_rwlock_t; #else -// Dummy lock type for GIL-only mode +// GIL-only mode. The GIL serializes all access, but Python code, such as a +// finalizer that a GC runs, can still run during a read and close the reader. +// Count the reads so that close() and a reinit can refuse to unmap the +// database under one. typedef struct { - // Dummy member to satisfy MSVC, which doesn't allow empty structs. - char dummy; + int readers; } reader_rwlock_t; #endif @@ -67,6 +69,8 @@ typedef struct Reader_obj_struct { MMDB_s *mmdb; PyObject *closed; reader_rwlock_t rwlock; + // Incremented on each open, so an iterator can detect a reopen. + uint64_t generation; } Reader_obj; typedef struct record record; @@ -83,6 +87,7 @@ typedef struct { PyObject_HEAD /* no semicolon */ Reader_obj *reader; struct record *next; + uint64_t generation; } ReaderIter_obj; typedef struct { @@ -127,6 +132,7 @@ static inline maxminddb_state *get_maxminddb_state_from_self(PyObject *self) { return get_maxminddb_state(module); } +static void reader_close_database(Reader_obj *reader); static bool can_read(const char *path); static int get_record(PyObject *self, PyObject *args, PyObject **record); static bool format_sockaddr(struct sockaddr *addr, char *dst); @@ -139,6 +145,11 @@ static PyObject *from_array(maxminddb_state *state, static PyObject *from_uint128(const MMDB_entry_data_list_s *entry_data_list); static int ip_converter(PyObject *obj, struct sockaddr_storage *ip_address); +// The error for an entry data list that ends too early. +#define CORRUPT_DATA_MESSAGE \ + "Error while looking up data. Your database may be corrupt or you have " \ + "found a bug in libmaxminddb." + #ifdef __GNUC__ #define UNUSED(x) UNUSED_##x __attribute__((__unused__)) #else @@ -165,8 +176,7 @@ static int reader_lock_init(reader_rwlock_t *lock) { return 0; #else - // GIL-only mode - no-op - (void)lock; + lock->readers = 0; return 0; #endif } @@ -222,8 +232,7 @@ static int reader_acquire_read_lock(Reader_obj *reader) { return 0; #else - // GIL-only mode - no-op - (void)reader; + reader->rwlock.readers++; return 0; #endif } @@ -243,8 +252,7 @@ static void reader_release_read_lock(Reader_obj *reader) { } #else - // GIL-only mode - no-op - (void)reader; + reader->rwlock.readers--; #endif } @@ -278,8 +286,13 @@ static int reader_acquire_write_lock(Reader_obj *reader) { return 0; #else - // GIL-only mode - no-op - (void)reader; + // A write section runs no Python code, so only a read can be in progress. + if (reader->rwlock.readers > 0) { + PyErr_SetString(PyExc_RuntimeError, + "Cannot close or reopen a MaxMind DB while a read is " + "in progress."); + return -1; + } return 0; #endif } @@ -308,6 +321,30 @@ static void reader_release_write_lock(Reader_obj *reader) { // Reader implementation // ============================================================================= +static PyObject * +Reader_new(PyTypeObject *type, PyObject *UNUSED(args), PyObject *UNUSED(kwds)) { + PyObject *self = type->tp_alloc(type, 0); + if (self == NULL) { + return NULL; + } + + // Initialize the lock once, for the whole lifetime of the object. + // Reader_dealloc destroys it. A bare __new__ or a failed Reader_init then + // still leaves a valid, unlocked lock, so no path uses or destroys an + // uninitialized lock. + if (reader_lock_init(&((Reader_obj *)self)->rwlock) != 0) { + // Skip Reader_dealloc, which would destroy the failed lock. An + // instance of a heap type holds a reference to its type. + PyObject_Del(self); + Py_DECREF(type); + return NULL; + } + + // No database is open until Reader_init succeeds. + ((Reader_obj *)self)->closed = Py_True; + return self; +} + static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { maxminddb_state *state = get_maxminddb_state_from_self(self); if (state == NULL) { @@ -356,24 +393,11 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { return -1; } - Reader_obj *mmdb_obj = (Reader_obj *)self; - if (!mmdb_obj) { - Py_XDECREF(filepath); - free(mmdb); - PyErr_NoMemory(); - return -1; - } - - if (reader_lock_init(&mmdb_obj->rwlock) != 0) { - free(mmdb); - Py_XDECREF(filepath); - return -1; - } - + // Open the new database before taking the lock, so a failed open leaves + // the reader as it was, as in the pure Python reader. int const status = MMDB_open(filename, MMDB_MODE_MMAP, mmdb); if (status != MMDB_SUCCESS) { - reader_lock_destroy(&mmdb_obj->rwlock); free(mmdb); PyErr_Format(state->MaxMindDB_error, "Error opening database file (%s). Is this a valid " @@ -383,13 +407,38 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { return -1; } - Py_XDECREF(filepath); + Reader_obj *mmdb_obj = (Reader_obj *)self; + if (reader_acquire_write_lock(mmdb_obj) != 0) { + MMDB_close(mmdb); + free(mmdb); + Py_XDECREF(filepath); + return -1; + } + // A second init reopens the reader. Close the old database. + reader_close_database(mmdb_obj); mmdb_obj->mmdb = mmdb; mmdb_obj->closed = Py_False; + // Stop the iterators of the old database. Their records point into it. + mmdb_obj->generation++; + reader_release_write_lock(mmdb_obj); + + // Release the path only after the lock. filepath can be a bytes subclass + // from __fspath__, so its finalizer can run code that uses the reader. + Py_XDECREF(filepath); return 0; } +// The caller holds the write lock, or is the only user of the reader. +static void reader_close_database(Reader_obj *reader) { + if (reader->mmdb != NULL) { + MMDB_close(reader->mmdb); + free(reader->mmdb); + reader->mmdb = NULL; + } + reader->closed = Py_True; +} + static PyObject *Reader_get(PyObject *self, PyObject *args) { PyObject *record = NULL; if (get_record(self, args, &record) == -1) { @@ -640,6 +689,7 @@ static PyObject *Reader_metadata(PyObject *self, PyObject *UNUSED(args)) { if (metadata_dict == NULL || !PyDict_Check(metadata_dict)) { reader_release_read_lock(mmdb_obj); PyErr_SetString(state->MaxMindDB_error, "Error decoding metadata."); + Py_XDECREF(metadata_dict); return NULL; } @@ -666,13 +716,7 @@ static PyObject *Reader_close(PyObject *self, PyObject *UNUSED(args)) { return NULL; } - if (mmdb_obj->mmdb != NULL) { - MMDB_close(mmdb_obj->mmdb); - free(mmdb_obj->mmdb); - mmdb_obj->mmdb = NULL; - } - - mmdb_obj->closed = Py_True; + reader_close_database(mmdb_obj); reader_release_write_lock(mmdb_obj); @@ -686,7 +730,7 @@ static PyObject *Reader__enter__(PyObject *self, PyObject *UNUSED(args)) { return NULL; } - if (mmdb_obj->closed == Py_True) { + if (mmdb_obj->mmdb == NULL) { reader_release_read_lock(mmdb_obj); PyErr_SetString(PyExc_ValueError, "Attempt to reopen a closed MaxMind DB."); @@ -700,19 +744,21 @@ static PyObject *Reader__enter__(PyObject *self, PyObject *UNUSED(args)) { } static PyObject *Reader__exit__(PyObject *self, PyObject *UNUSED(args)) { - Reader_close(self, NULL); - Py_RETURN_NONE; + return Reader_close(self, NULL); } static void Reader_dealloc(PyObject *self) { Reader_obj *obj = (Reader_obj *)self; - if (obj->mmdb != NULL) { - Reader_close(self, NULL); - } + // No lock is needed. At a count of 0 no other thread can use the reader, + // because each iterator holds a reference to it. + reader_close_database(obj); reader_lock_destroy(&obj->rwlock); + // An instance of a heap type holds a reference to its type. + PyTypeObject *type = Py_TYPE(self); PyObject_Del(self); + Py_DECREF(type); } static PyObject *Reader_iter(PyObject *obj) { @@ -727,13 +773,14 @@ static PyObject *Reader_iter(PyObject *obj) { return NULL; } - if (reader->closed == Py_True) { + if (reader->mmdb == NULL) { reader_release_read_lock(reader); PyErr_SetString(PyExc_ValueError, "Attempt to iterate over a closed MaxMind DB."); return NULL; } + uint64_t const generation = reader->generation; reader_release_read_lock(reader); ReaderIter_obj *ri = (ReaderIter_obj *)PyType_GenericAlloc( @@ -744,6 +791,7 @@ static PyObject *Reader_iter(PyObject *obj) { ri->reader = reader; Py_INCREF(reader); + ri->generation = generation; // Currently, we are always starting from the 0 node with the 0 IP ri->next = calloc(1, sizeof(record)); @@ -777,13 +825,21 @@ static PyObject *ReaderIter_next(PyObject *self) { return NULL; } - if (ri->reader->closed == Py_True) { + if (ri->reader->mmdb == NULL) { reader_release_read_lock(ri->reader); PyErr_SetString(PyExc_ValueError, "Attempt to iterate over a closed MaxMind DB."); return NULL; } + if (ri->generation != ri->reader->generation) { + reader_release_read_lock(ri->reader); + PyErr_SetString(PyExc_ValueError, + "Attempt to iterate over a reopened MaxMind DB. " + "Create a new iterator."); + return NULL; + } + while (ri->next != NULL) { record *cur = ri->next; ri->next = cur->next; @@ -877,7 +933,7 @@ static PyObject *ReaderIter_next(PyObject *self) { } int ip_start = 0; - int ip_length = 4; + Py_ssize_t ip_length = 4; if (ri->reader->mmdb->depth == 128) { if (is_ipv6(cur->ip_packed)) { // IPv6 address @@ -950,11 +1006,15 @@ static void ReaderIter_dealloc(PyObject *self) { next = cur->next; free(cur); } + PyTypeObject *type = Py_TYPE(self); PyObject_Del(self); + Py_DECREF(type); } -static int Metadata_init(PyObject *self, PyObject *args, PyObject *kwds) { - +// Metadata is immutable, so tp_new sets every field and there is no tp_init. +// No object can then have a NULL field or be initialized twice. +static PyObject * +Metadata_new(PyTypeObject *type, PyObject *args, PyObject *kwds) { PyObject *binary_format_major_version, *binary_format_minor_version, *build_epoch, *database_type, *description, *ip_version, *languages, *node_count, *record_size; @@ -972,7 +1032,7 @@ static int Metadata_init(PyObject *self, PyObject *args, PyObject *kwds) { if (!PyArg_ParseTupleAndKeywords(args, kwds, - "|OOOOOOOOO", + "OOOOOOOOO:Metadata", kwlist, &binary_format_major_version, &binary_format_minor_version, @@ -983,32 +1043,23 @@ static int Metadata_init(PyObject *self, PyObject *args, PyObject *kwds) { &languages, &node_count, &record_size)) { - return -1; + return NULL; } - Metadata_obj *obj = (Metadata_obj *)self; - - obj->binary_format_major_version = binary_format_major_version; - obj->binary_format_minor_version = binary_format_minor_version; - obj->build_epoch = build_epoch; - obj->database_type = database_type; - obj->description = description; - obj->ip_version = ip_version; - obj->languages = languages; - obj->node_count = node_count; - obj->record_size = record_size; - - Py_INCREF(obj->binary_format_major_version); - Py_INCREF(obj->binary_format_minor_version); - Py_INCREF(obj->build_epoch); - Py_INCREF(obj->database_type); - Py_INCREF(obj->description); - Py_INCREF(obj->ip_version); - Py_INCREF(obj->languages); - Py_INCREF(obj->node_count); - Py_INCREF(obj->record_size); - - return 0; + Metadata_obj *obj = (Metadata_obj *)type->tp_alloc(type, 0); + if (obj == NULL) { + return NULL; + } + obj->binary_format_major_version = Py_NewRef(binary_format_major_version); + obj->binary_format_minor_version = Py_NewRef(binary_format_minor_version); + obj->build_epoch = Py_NewRef(build_epoch); + obj->database_type = Py_NewRef(database_type); + obj->description = Py_NewRef(description); + obj->ip_version = Py_NewRef(ip_version); + obj->languages = Py_NewRef(languages); + obj->node_count = Py_NewRef(node_count); + obj->record_size = Py_NewRef(record_size); + return (PyObject *)obj; } static void Metadata_dealloc(PyObject *self) { @@ -1022,16 +1073,16 @@ static void Metadata_dealloc(PyObject *self) { Py_DECREF(obj->languages); Py_DECREF(obj->node_count); Py_DECREF(obj->record_size); + PyTypeObject *type = Py_TYPE(self); PyObject_Del(self); + Py_DECREF(type); } static PyObject * from_entry_data_list(maxminddb_state *state, MMDB_entry_data_list_s **entry_data_list) { if (entry_data_list == NULL || *entry_data_list == NULL) { - PyErr_SetString(state->MaxMindDB_error, - "Error while looking up data. Your database may be " - "corrupt or you have found a bug in libmaxminddb."); + PyErr_SetString(state->MaxMindDB_error, CORRUPT_DATA_MESSAGE); return NULL; } @@ -1057,7 +1108,8 @@ from_entry_data_list(maxminddb_state *state, case MMDB_DATA_TYPE_UINT16: return PyLong_FromLong((*entry_data_list)->entry_data.uint16); case MMDB_DATA_TYPE_UINT32: - return PyLong_FromLong((*entry_data_list)->entry_data.uint32); + return PyLong_FromUnsignedLong( + (*entry_data_list)->entry_data.uint32); case MMDB_DATA_TYPE_BOOLEAN: return PyBool_FromLong((*entry_data_list)->entry_data.boolean); case MMDB_DATA_TYPE_UINT64: @@ -1090,12 +1142,29 @@ static PyObject *from_map(maxminddb_state *state, for (i = 0; i < map_size && *entry_data_list; i++) { *entry_data_list = (*entry_data_list)->next; + // A list that ends before the key is corrupt, as in + // from_entry_data_list. + if (*entry_data_list == NULL) { + PyErr_SetString(state->MaxMindDB_error, CORRUPT_DATA_MESSAGE); + Py_DECREF(py_obj); + return NULL; + } + + // libmaxminddb does not check the key type, and the union holds a + // string only for a string key. + if ((*entry_data_list)->entry_data.type != MMDB_DATA_TYPE_UTF8_STRING) { + PyErr_SetString(state->MaxMindDB_error, + "Invalid map key: the key is not a string."); + Py_DECREF(py_obj); + return NULL; + } PyObject *key = PyUnicode_FromStringAndSize( (*entry_data_list)->entry_data.utf8_string, (*entry_data_list)->entry_data.data_size); if (!key) { // PyUnicode_FromStringAndSize will set an appropriate exception // in this case. + Py_DECREF(py_obj); return NULL; } @@ -1107,9 +1176,13 @@ static PyObject *from_map(maxminddb_state *state, Py_DECREF(py_obj); return NULL; } - PyDict_SetItem(py_obj, key, value); + int const status = PyDict_SetItem(py_obj, key, value); Py_DECREF(value); Py_DECREF(key); + if (status < 0) { + Py_DECREF(py_obj); + return NULL; + } } return py_obj; @@ -1263,6 +1336,7 @@ static PyMemberDef Metadata_members[] = { static PyType_Slot Reader_Type_slots[] = { {Py_tp_doc, "Reader object"}, {Py_tp_dealloc, Reader_dealloc}, + {Py_tp_new, Reader_new}, {Py_tp_init, Reader_init}, {Py_tp_iter, Reader_iter}, {Py_tp_methods, Reader_methods}, @@ -1280,7 +1354,7 @@ static PyType_Spec Reader_Type_spec = { static PyType_Slot Metadata_Type_slots[] = { {Py_tp_doc, "Metadata object"}, {Py_tp_dealloc, Metadata_dealloc}, - {Py_tp_init, Metadata_init}, + {Py_tp_new, Metadata_new}, {Py_tp_methods, Metadata_methods}, {Py_tp_members, Metadata_members}, {0, NULL}, @@ -1305,7 +1379,7 @@ static PyType_Slot ReaderIter_Type_slots[] = { static PyType_Spec ReaderIter_Type_spec = { .name = "maxminddb.extension.ReaderIter", .basicsize = sizeof(ReaderIter_obj), - .flags = Py_TPFLAGS_DEFAULT, + .flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_DISALLOW_INSTANTIATION, .slots = ReaderIter_Type_slots, }; diff --git a/maxminddb/extension.pyi b/maxminddb/extension.pyi index 1a5e88b0..a694ab40 100644 --- a/maxminddb/extension.pyi +++ b/maxminddb/extension.pyi @@ -3,7 +3,7 @@ from collections.abc import Iterator from ipaddress import IPv4Address, IPv4Network, IPv6Address, IPv6Network from os import PathLike -from typing import IO, Any +from typing import IO from typing_extensions import Self @@ -113,5 +113,17 @@ class Metadata: The bit size of a record in the search tree. """ - def __init__(self, **kwargs: Any) -> None: # noqa: ANN401 - """Create new Metadata object. kwargs are key/value pairs from spec.""" + def __init__( + self, + *, + binary_format_major_version: int, + binary_format_minor_version: int, + build_epoch: int, + database_type: str, + description: dict[str, str], + ip_version: int, + languages: list[str], + node_count: int, + record_size: int, + ) -> None: + """Create new Metadata object from the metadata fields in the spec.""" diff --git a/maxminddb/reader.py b/maxminddb/reader.py index b4def914..c0ca346b 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -27,6 +27,8 @@ from maxminddb.types import Record _IPV4_MAX_NUM = 2**32 +_REOPENED = "Attempt to iterate over a reopened MaxMind DB. Create a new iterator." +_CLOSED = "Attempt to iterate over a closed MaxMind DB." class Reader: @@ -40,11 +42,14 @@ class Reader: _buffer: bytes | FileBuffer | "mmap.mmap" # noqa: UP037 _buffer_size: int - closed: bool + # No database is open until __init__ succeeds. + closed: bool = True _decoder: Decoder _metadata: Metadata _record_size: int _ipv4_start: int + # Incremented on each open, so an iterator can detect a reopen. + _generation: int = 0 def __init__( self, @@ -64,12 +69,34 @@ def __init__( * MODE_FD - the param passed via database is a file descriptor, not a path. This mode implies MODE_MEMORY. - """ - filename = self._load_buffer(database, mode) + A second call reopens the reader with the new database. A failed call + keeps the old one. Like close(), a second call can make reads in + progress on other threads fail or return wrong results. - # Include validation errors in this cleanup scope. TRY301 is suppressed - # because the handler only closes the buffer and re-raises the error. + """ + # Load into a new object, then copy its state in one step, so that + # other threads never see a mix of the old and the new database. A + # failed load leaves this reader as it was, as in the C extension. The + # new object is a base Reader, so freeing it runs no __del__ of a + # subclass, and the update keeps the attributes that a subclass set. + new = Reader.__new__(Reader) + new._load(database, mode) # noqa: SLF001 + # A source can return the same buffer object again, such as BytesIO, + # so count the opens instead of comparing buffers. + new._generation = self._generation + 1 # noqa: SLF001 + old_buffer = self.__dict__.get("_buffer") + self.__dict__.update(new.__dict__) + _close_buffer(old_buffer, keep=self._buffer) + + def _load( + self, + database: str | bytes | int | PathLike[str] | PathLike[bytes] | IO[bytes], + mode: int, + ) -> None: + # TRY301 is suppressed because the handler only closes the buffer and + # re-raises the error. try: + filename = self._load_buffer(database, mode) metadata_start = self._buffer.rfind( self._METADATA_START_MARKER, max(0, self._buffer_size - 128 * 1024), @@ -136,8 +163,7 @@ def __init__( ipv4_start = node self._ipv4_start = ipv4_start except BaseException: - # Release the buffer on any initialization failure. - self.close() + _close_buffer(self.__dict__.get("_buffer")) raise def metadata(self) -> Metadata: @@ -191,7 +217,22 @@ def get_with_prefix_len( return None, prefix_len def __iter__(self) -> Iterator: - return self._generate_children(0, 0, 0) + return self._iterate(self._generation) + + def _iterate(self, generation: int) -> Iterator: + children = self._generate_children(0, 0, 0) + while True: + # Check before the walk resumes and reads more nodes, as the C + # extension does. After a second __init__ or close(), the node + # numbers of the walk no longer match the buffer. + if self._generation != generation: + raise ValueError(_REOPENED) + if self.closed: + raise ValueError(_CLOSED) + record = next(children, None) + if record is None: + return + yield record def _generate_children(self, node: int, depth: int, ip_acc: int) -> Iterator: if ip_acc != 0 and node == self._ipv4_start: @@ -320,8 +361,8 @@ def close(self) -> None: Calling this method while reads are in progress may cause exceptions. """ - with contextlib.suppress(AttributeError): - self._buffer.close() # type: ignore[union-attr] + # A reader made with __new__ alone has no buffer. + _close_buffer(getattr(self, "_buffer", None)) self.closed = True @@ -385,3 +426,12 @@ def node_byte_size(self) -> int: def search_tree_size(self) -> int: """The size of the search tree.""" return self.node_count * self.node_byte_size + + +def _close_buffer(buffer: object, keep: object = None) -> None: + # A source can return the same buffer again. Keep the one in use open. + if buffer is keep: + return + # bytes, bytearray and None have no close(). + with contextlib.suppress(AttributeError): + buffer.close() # type: ignore[attr-defined] diff --git a/tests/reader_test.py b/tests/reader_test.py index ce95714e..da91173f 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1,16 +1,22 @@ from __future__ import annotations import contextlib +import gc import io import ipaddress +import mmap import multiprocessing import os import pathlib +import subprocess import sys +import sysconfig import tempfile +import textwrap import threading +import tracemalloc import unittest -from typing import TYPE_CHECKING, cast +from typing import TYPE_CHECKING, Any, cast from unittest import mock import maxminddb @@ -32,12 +38,26 @@ if TYPE_CHECKING: from collections.abc import Iterator + from typing import IO from maxminddb.reader import Reader # Directory holding the shared MaxMind DB test fixtures. _TEST_DATA_DIR = "tests/data/test-data" +_DECODER_DB = f"{_TEST_DATA_DIR}/MaxMind-DB-test-decoder.mmdb" +# Valid arguments for the C extension Metadata. +_METADATA_FIELDS: dict[str, Any] = { + "binary_format_major_version": 2, + "binary_format_minor_version": 0, + "build_epoch": 1, + "database_type": "db", + "description": {}, + "ip_version": 4, + "languages": [], + "node_count": 1, + "record_size": 24, +} _PAYLOAD_TOO_LARGE = ( "^The MaxMind DB file's data section exceeds the maximum payload size$" ) @@ -96,14 +116,20 @@ def address_space_in_use() -> int: def get_reader_from_file_descriptor(filepath: str, mode: int) -> Reader: """Patches open_database() for class TestFDReader().""" + # There are a few cases where mode is statically defined in + # BaseTestReader(). In those cases, this opens the string path. + with _database_source(filepath, mode) as database: + return maxminddb.open_database(database, mode) + + +@contextlib.contextmanager +def _database_source(path: str, mode: int) -> Iterator[str | IO[bytes]]: + """Yield the database argument for path: a binary file for MODE_FD.""" if mode == MODE_FD: - with open(filepath, "rb") as mmdb_fh: - return maxminddb.open_database(mmdb_fh, mode) + with open(path, "rb") as database: + yield database else: - # There are a few cases where mode is statically defined in - # BaseTestReader(). In those cases just call an unpatched - # open_database() with a string path. - return maxminddb.open_database(filepath, mode) + yield path class BaseTestReader(unittest.TestCase): @@ -505,6 +531,15 @@ def test_decoder(self) -> None: self.assertEqual(1329227995784915872903807060280344576, record["uint128"]) reader.close() + def test_decoder_maximum_values(self) -> None: + with open_database(_DECODER_DB, self.mode) as reader: + record = cast("dict", reader.get(self.ipf("::255.255.255.255"))) + # A C long has 32 bits on Windows, where a signed conversion would make + # the uint32 negative. + self.assertEqual(record["uint32"], 2**32 - 1) + self.assertEqual(record["uint64"], 2**64 - 1) + self.assertEqual(record["uint128"], 2**128 - 1) + def test_metadata_pointers(self) -> None: with open_database( "tests/data/test-data/MaxMind-DB-test-metadata-pointers.mmdb", @@ -758,6 +793,81 @@ def test_closed(self) -> None: reader.close() self.assertEqual(reader.closed, True) + def test_iterate_uninitialized_reader(self) -> None: + reader = self.reader_class.__new__(self.reader_class) + # The C reader raises in iter(), the pure Python reader in next(). + with self.assertRaisesRegex(ValueError, "closed MaxMind DB"): + next(iter(reader)) + + def test_close_uninitialized_reader(self) -> None: + reader = self.reader_class.__new__(self.reader_class) + self.assertTrue(reader.closed) + reader.close() + self.assertTrue(reader.closed) + + def test_reinitialize_reopens_the_reader(self) -> None: + reader = open_database( + f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb", + self.mode, + ) + self.addCleanup(reader.close) + iterator = iter(reader) + next(iterator) + self._reinitialize(reader, _DECODER_DB, self.mode) + self.assertEqual(reader.metadata().database_type, "MaxMind DB Decoder Test") + # The iterator walked the old database, so it must stop. + with self.assertRaisesRegex(ValueError, "reopened MaxMind DB"): + next(iterator) + + reader.close() + self._reinitialize(reader, _DECODER_DB, self.mode) + self.assertFalse(reader.closed) + self.assertIsNotNone(reader.get("::1.1.1.0")) + + def test_reinitialize_at_the_last_record(self) -> None: + # The last record of this database is the right child of the root, so + # no node of the walk remains after it. + reader = open_database( + f"{_TEST_DATA_DIR}/MaxMind-DB-test-decoder-value-limit.mmdb", + self.mode, + ) + self.addCleanup(reader.close) + count = sum(1 for _ in reader) + iterator = iter(reader) + for _ in range(count): + next(iterator) + self._reinitialize(reader, _DECODER_DB, self.mode) + with self.assertRaisesRegex(ValueError, "reopened MaxMind DB"): + next(iterator) + + def test_iterate_after_close(self) -> None: + reader = open_database(_DECODER_DB, self.mode) + iterator = iter(reader) + next(iterator) + reader.close() + with self.assertRaisesRegex(ValueError, "closed MaxMind DB"): + next(iterator) + + def test_failed_reinitialize(self) -> None: + reader = open_database(_DECODER_DB, self.mode) + self.addCleanup(reader.close) + + # A failed reinit leaves the old database open. + with self.assertRaisesRegex(ValueError, "Unsupported open mode"): + self._reinitialize(reader, _DECODER_DB, 100) + if self.mode != MODE_FD: + with self.assertRaises(FileNotFoundError): + self._reinitialize(reader, "missing.mmdb", self.mode) + with self.assertRaises(InvalidDatabaseError): + self._reinitialize(reader, "README.rst", self.mode) + self.assertFalse(reader.closed) + self.assertEqual(reader.metadata().database_type, "MaxMind DB Decoder Test") + self.assertIsNotNone(reader.get("::1.1.1.0")) + + def _reinitialize(self, reader: Any, path: str, mode: int) -> None: # noqa: ANN401 + with _database_source(path, mode) as database: + reader.__init__(database, mode) + def test_closed_metadata(self) -> None: reader = open_database( "tests/data/test-data/MaxMind-DB-test-decoder.mmdb", @@ -929,6 +1039,43 @@ class TestExtensionReader(BaseTestReader): if has_maxminddb_extension(): reader_class = maxminddb.extension.Reader + def test_map_key_that_is_not_a_string_is_rejected(self) -> None: + data = bytearray( + pathlib.Path(f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb").read_bytes(), + ) + # Change the type of the "ip" key from a string to a uint16. + data[data.index(b"\x42ip")] = 0xA2 + with tempfile.TemporaryDirectory() as directory: + path = pathlib.Path(directory) / "int-key.mmdb" + path.write_bytes(data) + with ( + maxminddb.extension.Reader(path) as reader, + self.assertRaisesRegex(InvalidDatabaseError, "not a string"), + ): + reader.get("1.1.1.1") + + def test_invalid_utf8_key_does_not_leak(self) -> None: + def fail_to_decode(count: int) -> None: + for _ in range(count): + with contextlib.suppress(UnicodeDecodeError): + reader.get("163.254.149.39") + + with maxminddb.extension.Reader( + "tests/data/bad-data/maxminddb-python/bad-unicode-in-map-key.mmdb", + ) as reader: + fail_to_decode(100) + gc.collect() + tracemalloc.start() + try: + before, _ = tracemalloc.get_traced_memory() + fail_to_decode(2000) + gc.collect() + after, _ = tracemalloc.get_traced_memory() + finally: + tracemalloc.stop() + # A leaked dict on each failure keeps about 128 KB. + self.assertLess(after - before, 16_000) + @unittest.skipIf( not has_maxminddb_extension() and not os.environ.get("MM_FORCE_EXT_TESTS"), @@ -942,6 +1089,198 @@ class TestExtensionReaderWithIPObjects(BaseTestReader): reader_class = maxminddb.extension.Reader +@unittest.skipIf( + not has_maxminddb_extension() and not os.environ.get("MM_FORCE_EXT_TESTS"), + "No C extension module found. Skipping tests", +) +class TestExtensionObjects(unittest.TestCase): + """Objects in states that crashed the extension.""" + + def test_new_metadata_requires_arguments(self) -> None: + metadata_class = maxminddb.extension.Metadata + with self.assertRaisesRegex(TypeError, "missing required argument"): + metadata_class.__new__(metadata_class) + + def test_metadata_missing_argument(self) -> None: + with self.assertRaisesRegex(TypeError, "missing required argument"): + maxminddb.extension.Metadata(binary_format_major_version=2) # type: ignore[call-arg] + + def test_metadata_too_many_arguments(self) -> None: + with self.assertRaisesRegex(TypeError, "at most 9"): + maxminddb.extension.Metadata(**_METADATA_FIELDS, unknown=1) # type: ignore[call-arg] + + def test_iterate_uninitialized_reader(self) -> None: + reader_class = maxminddb.extension.Reader + reader = reader_class.__new__(reader_class) + with self.assertRaisesRegex(ValueError, "closed MaxMind DB"): + iter(reader) + + def test_enter_uninitialized_reader(self) -> None: + reader_class = maxminddb.extension.Reader + reader = reader_class.__new__(reader_class) + with self.assertRaisesRegex(ValueError, "closed MaxMind DB"): + reader.__enter__() + + def test_path_finalizer_can_close_the_reader(self) -> None: + # A bytes subclass from __fspath__ can run code when init releases + # it. If init still held the write lock, a close() on another thread + # would wait for it forever on free-threaded Python. Run in a + # subprocess with a timeout. + program = textwrap.dedent( + """ + import sys + import threading + + from maxminddb.extension import Reader + + reader = Reader.__new__(Reader) + + class FinalizingBytes(bytes): + def __del__(self): + worker = threading.Thread(target=reader.close) + worker.start() + worker.join() + + class Path: + def __fspath__(self): + return FinalizingBytes(sys.argv[1].encode()) + + reader.__init__(Path()) + if not reader.closed: + sys.exit("the finalizer did not close the reader") + print("ok") + """, + ) + self._run_program(program) + + def test_finalizer_during_a_read_cannot_reopen_the_reader(self) -> None: + # With the GIL, a GC can run a finalizer during a decode, on Python + # 3.10 and 3.11. If the finalizer reopened the reader there, the + # decode would read the unmapped database and crash. + program = textwrap.dedent( + """ + import gc + import sys + + from maxminddb.extension import Reader + + reader = Reader(sys.argv[1]) + + class Reopen: + def __init__(self): + self.cycle = self + + def __del__(self): + try: + reader.__init__(sys.argv[1]) + except RuntimeError: + pass + + gc.set_threshold(1) + for _ in range(2000): + Reopen() + if reader.get("::1.1.1.0") is None: + sys.exit("get() lost the record") + Reopen() + try: + next(iter(reader)) + except ValueError: + # A finalizer between iter() and next() reopened it. + pass + print("ok") + """, + ) + self._run_program(program) + + def _run_program(self, program: str) -> None: + # Put this process's maxminddb first, and keep the harness's paths. + paths = [str(pathlib.Path(maxminddb.__file__).parent.parent)] + if os.environ.get("PYTHONPATH"): + paths.append(os.environ["PYTHONPATH"]) + env = {**os.environ, "PYTHONPATH": os.pathsep.join(paths)} + path = pathlib.Path(_DECODER_DB).resolve() + with tempfile.TemporaryDirectory() as directory: + # Run from an empty directory so the child imports the same + # maxminddb as this process, not a source tree in the cwd. + result = subprocess.run( # noqa: S603 + [sys.executable, "-c", program, str(path)], + capture_output=True, + text=True, + check=False, + cwd=directory, + env=env, + timeout=60, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip(), "ok") + + @unittest.skipUnless( + pathlib.Path("/proc/self/maps").exists(), + "needs /proc/self/maps and /proc/self/fd", + ) + def test_reinitialize_releases_the_old_database(self) -> None: + with tempfile.TemporaryDirectory() as directory: + # A copy that no other test has open, so only this reader counts. + path = pathlib.Path(directory) / "decoder.mmdb" + path.write_bytes(pathlib.Path(_DECODER_DB).read_bytes()) + real_path = path.resolve() + + def mappings() -> int: + maps = pathlib.Path("/proc/self/maps").read_text() + return maps.count(str(real_path)) + + def descriptors() -> int: + return sum( + fd.resolve() == real_path + for fd in pathlib.Path("/proc/self/fd").iterdir() + ) + + reader = maxminddb.extension.Reader(path) + for _ in range(10): + reader.__init__(path) # type: ignore[misc] + self.assertEqual(mappings(), 1) + self.assertEqual(descriptors(), 0) + reader.close() + self.assertEqual(mappings(), 0) + + def test_initialize_after_close_on_uninitialized_reader(self) -> None: + reader_class = maxminddb.extension.Reader + reader = reader_class.__new__(reader_class) + reader.close() + self.assertTrue(reader.closed) + reader.__init__(_DECODER_DB) # type: ignore[misc] + with reader: + self.assertIsNotNone(reader.get("::1.1.1.0")) + + def test_reinitialize_metadata_changes_nothing(self) -> None: + metadata = maxminddb.extension.Metadata(**_METADATA_FIELDS) + metadata.__init__(**{**_METADATA_FIELDS, "record_size": 28}) # type: ignore[misc] + self.assertEqual(metadata.record_size, 24) + + @unittest.skipUnless( + hasattr(sys, "getrefcount") and not sysconfig.get_config_var("Py_GIL_DISABLED"), + "needs CPython reference counts on a build with the GIL", + ) + def test_freed_objects_release_their_type(self) -> None: + with maxminddb.extension.Reader(_DECODER_DB) as reader: + classes = [type(reader), type(reader.metadata()), type(iter(reader))] + before = [sys.getrefcount(c) for c in classes] + for _ in range(10): + with maxminddb.extension.Reader(_DECODER_DB) as reader: + reader.metadata() + iter(reader) + self.assertEqual([sys.getrefcount(c) for c in classes], before) + + def test_iterator_type_is_not_instantiable(self) -> None: + with maxminddb.extension.Reader(_DECODER_DB) as reader: + iterator_class = type(iter(reader)) + # The message differs across Python versions, so check only the type. + with self.assertRaises(TypeError): + iterator_class() + with self.assertRaises(TypeError): + iterator_class.__new__(iterator_class) + + class TestAutoReader(BaseTestReader): mode = MODE_AUTO @@ -987,6 +1326,118 @@ def setUp(self) -> None: class TestReaderInitialization(unittest.TestCase): + def test_reinitialize_closes_the_old_buffer(self) -> None: + for mode in (MODE_MMAP, MODE_FILE): + with self.subTest(mode=mode): + reader = maxminddb.reader.Reader(_DECODER_DB, mode) + self.addCleanup(reader.close) + old: Any = reader._buffer # noqa: SLF001 + reader.__init__(_DECODER_DB, mode) # type: ignore[misc] + if mode == MODE_FILE: + self.assertTrue(old._handle.closed) # noqa: SLF001 + else: + self.assertTrue(old.closed) + + def test_subclass_with_a_closing_finalizer_stays_open(self) -> None: + class ClosingReader(maxminddb.reader.Reader): + def __del__(self) -> None: + self.close() + + reader = ClosingReader(_DECODER_DB, MODE_MMAP) + self.addCleanup(reader.close) + # Freeing an object that init used must not close this reader. + gc.collect() + self.assertFalse(reader.closed) + self.assertIsNotNone(reader.get("::1.1.1.0")) + + def test_subclass_attributes_survive_init(self) -> None: + class TaggedReader(maxminddb.reader.Reader): + def __init__(self, database: str, mode: int) -> None: + self.tag = "kept" + super().__init__(database, mode) + + reader = TaggedReader(_DECODER_DB, MODE_MMAP) + self.addCleanup(reader.close) + self.assertEqual(reader.tag, "kept") + reader.__init__(f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb", MODE_MMAP) # type: ignore[misc] + self.assertEqual(reader.tag, "kept") + + def test_reinitialize_switches_to_the_new_database_at_the_end(self) -> None: + reader = maxminddb.reader.Reader( + f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb", + MODE_MEMORY, + ) + self.addCleanup(reader.close) + load_buffer = maxminddb.reader.Reader._load_buffer # noqa: SLF001 + records_during_load: list[object] = [] + + def load_and_read(new: Reader, database: str, mode: int) -> object: + filename = load_buffer(new, database, mode) + # Another thread could read here. It must see the old database. + records_during_load.append(reader.get("1.1.1.1")) + return filename + + with mock.patch.object( + maxminddb.reader.Reader, + "_load_buffer", + autospec=True, + side_effect=load_and_read, + ): + reader.__init__(_DECODER_DB, MODE_MEMORY) # type: ignore[misc] + self.assertEqual(records_during_load, [{"ip": "1.1.1.1"}]) + self.assertEqual(reader.metadata().database_type, "MaxMind DB Decoder Test") + + def test_reinitialize_from_a_source_that_returns_the_same_mmap(self) -> None: + with open(_DECODER_DB, "rb") as database: + buffer = mmap.mmap(database.fileno(), 0, access=mmap.ACCESS_READ) + self.addCleanup(buffer.close) + + class Source: + def read(self) -> mmap.mmap: + return buffer + + reader = maxminddb.reader.Reader(Source(), MODE_FD) # type: ignore[arg-type] + # A reinit must not close the buffer that it then uses. + reader.__init__(Source(), MODE_FD) # type: ignore[misc] + self.assertIsNotNone(reader.get("::1.1.1.0")) + + def test_reinitialize_from_the_same_source(self) -> None: + ipv4 = pathlib.Path(f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb") + ipv6 = pathlib.Path(f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv6-24.mmdb") + + class OneBuffer: + """Return the same bytearray from each read().""" + + def __init__(self) -> None: + self.buffer = bytearray(ipv4.read_bytes()) + + def read(self) -> bytearray: + return self.buffer + + # BytesIO.read() returns the same bytes object after seek(0). + bytes_io = io.BytesIO(ipv4.read_bytes()) + one_buffer = OneBuffer() + + def reopen_bytes_io() -> None: + bytes_io.seek(0) + + def reopen_one_buffer() -> None: + one_buffer.buffer[:] = ipv6.read_bytes() + + for source, change in ( + (bytes_io, reopen_bytes_io), + (one_buffer, reopen_one_buffer), + ): + with self.subTest(source=type(source).__name__): + reader = maxminddb.reader.Reader(source, MODE_FD) # type: ignore[arg-type] + self.addCleanup(reader.close) + iterator = iter(reader) + next(iterator) + change() + reader.__init__(source, MODE_FD) # type: ignore[misc] + with self.assertRaisesRegex(ValueError, "reopened MaxMind DB"): + next(iterator) + def test_empty_search_tree_is_accepted(self) -> None: data = pathlib.Path( f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb" @@ -1046,21 +1497,19 @@ def test_failed_initialization_closes_buffer(self) -> None: with ( _bounded(), mock.patch.object( - reader_class, - "close", - autospec=True, - side_effect=reader_class.close, - ) as close, + maxminddb.reader, + "_close_buffer", + wraps=maxminddb.reader._close_buffer, # noqa: SLF001 + ) as close_buffer, self.assertRaisesRegex(error, message), ): reader_class(path, mode) - close.assert_called_once() - reader = close.call_args.args[0] - self.assertTrue(reader.closed) + close_buffer.assert_called_once() + buffer = close_buffer.call_args.args[0] if mode == MODE_FILE: - self.assertTrue(reader._buffer._handle.closed) # noqa: SLF001 + self.assertTrue(buffer._handle.closed) # noqa: SLF001 else: - self.assertTrue(reader._buffer.closed) # noqa: SLF001 + self.assertTrue(buffer.closed) class TestSearchTreeNodes(unittest.TestCase):