From 16a36a08d327d5c446815c5a1e3bd23a82847b97 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Fri, 2 Oct 2026 22:31:47 +0000 Subject: [PATCH 01/26] Fix segmentation faults in the C extension Metadata_dealloc called Py_DECREF on each field. A Metadata object that init did not fill has NULL fields, so freeing it crashed. That happened after Metadata.__new__ and after a failed init. Reader.metadata() passes every metadata key to init, so a database with an unknown key crashed the process when code called metadata(). The spec allows new keys in a minor version of the format. Use Py_XDECREF. Metadata_init made every argument optional but increfed all nine locals. A missing argument increfed an uninitialized pointer. Make the arguments required. Reader_iter and ReaderIter_next checked closed == Py_True. A Reader that init did not open has closed == NULL and mmdb == NULL, so iteration dereferenced NULL. Check mmdb, as get() and metadata() do. The ReaderIter type allowed direct instantiation, and its dealloc then decrefed a NULL reader. Disallow instantiation. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 8 ++++++ extension/maxminddb.c | 28 ++++++++++----------- maxminddb/extension.pyi | 18 ++++++++++--- tests/reader_test.py | 56 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 17 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 50839c9a..ddd53d2f 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -3,6 +3,14 @@ History ------- +3.3.0 +++++++++++++++++++ + +* C extension: + + * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and + the internal iterator type. + 3.2.0 (2026-09-10) ++++++++++++++++++ diff --git a/extension/maxminddb.c b/extension/maxminddb.c index c2dd73e7..59833aa8 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -686,7 +686,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."); @@ -727,7 +727,7 @@ 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."); @@ -777,7 +777,7 @@ 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."); @@ -972,7 +972,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, @@ -1013,15 +1013,15 @@ static int Metadata_init(PyObject *self, PyObject *args, PyObject *kwds) { static void Metadata_dealloc(PyObject *self) { Metadata_obj *obj = (Metadata_obj *)self; - Py_DECREF(obj->binary_format_major_version); - Py_DECREF(obj->binary_format_minor_version); - Py_DECREF(obj->build_epoch); - Py_DECREF(obj->database_type); - Py_DECREF(obj->description); - Py_DECREF(obj->ip_version); - Py_DECREF(obj->languages); - Py_DECREF(obj->node_count); - Py_DECREF(obj->record_size); + Py_XDECREF(obj->binary_format_major_version); + Py_XDECREF(obj->binary_format_minor_version); + Py_XDECREF(obj->build_epoch); + Py_XDECREF(obj->database_type); + Py_XDECREF(obj->description); + Py_XDECREF(obj->ip_version); + Py_XDECREF(obj->languages); + Py_XDECREF(obj->node_count); + Py_XDECREF(obj->record_size); PyObject_Del(self); } @@ -1305,7 +1305,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/tests/reader_test.py b/tests/reader_test.py index ce95714e..38a000f2 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -38,6 +38,19 @@ # 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$" ) @@ -942,6 +955,49 @@ 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_uninitialized_metadata(self) -> None: + metadata_class = maxminddb.extension.Metadata + metadata = metadata_class.__new__(metadata_class) + self.assertIsNone(metadata.languages) + del metadata + + 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_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 From 108843d0933660869b97e58ef10e3dfd55fc3c91 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Mon, 5 Oct 2026 16:08:18 +0000 Subject: [PATCH 02/26] Initialize the reader lock in tp_new On free-threaded builds, the read-write lock did not follow the object lifetime. Reader_init created the lock, destroyed it again when the open failed, and Reader_dealloc destroyed it once more, so a failed open destroyed the lock twice. A reader made with __new__ alone, with no init, used and then destroyed a lock that was never created. A second __init__ initialized the lock again, while another thread could hold it. glibc treats an all-zero pthread_rwlock_t as a valid unlocked lock and ignores a second destroy, so this was harmless on Linux. Other platforms, such as macOS, reject both. Create the lock once in tp_new and destroy it once in Reader_dealloc. Reader_init no longer creates or destroys the lock. Every allocated reader, including one from a bare __new__ or a failed init, then has exactly one valid lock for its whole lifetime. If the lock fails to initialize, tp_new frees the object directly, because Reader_dealloc would destroy the failed lock. Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 2 ++ extension/maxminddb.c | 30 +++++++++++++++++++++++------- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index ddd53d2f..01531392 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -10,6 +10,8 @@ History * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and the internal iterator type. + * 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 59833aa8..5eeb5323 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -308,6 +308,28 @@ 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; + } + + return self; +} + static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { maxminddb_state *state = get_maxminddb_state_from_self(self); if (state == NULL) { @@ -364,16 +386,9 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { return -1; } - if (reader_lock_init(&mmdb_obj->rwlock) != 0) { - free(mmdb); - Py_XDECREF(filepath); - return -1; - } - 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 " @@ -1263,6 +1278,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}, From 435b58dd2d67f106e3cefa36a7ce3e5bd12d0b1c Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sat, 3 Oct 2026 16:11:23 +0000 Subject: [PATCH 03/26] Fix memory leaks and a use-after-free in the C extension The Reader, Metadata and iterator types are heap types, so each instance holds a reference to its type. The dealloc functions did not release that reference, so each object leaked one reference to its type, and the types were never freed. A second Reader_init did not close the open database, so it leaked. It also left existing iterators with records that point into the old database, so the next step of such an iterator read freed memory. A second init now reopens the reader, as in the pure Python reader. It closes the old database under the write lock before it opens the new one, so a failed open leaves the reader closed. Each open increments a generation count, and an iterator from an older generation raises ValueError. Init releases the path only after the lock, because a bytes subclass from __fspath__ can run code that uses the reader when it is freed. Metadata_init also accepted a second init, which leaked the old field values. Refuse it with a ValueError. Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 4 ++ extension/maxminddb.c | 103 +++++++++++++++++++++++++++++------------- tests/reader_test.py | 103 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 178 insertions(+), 32 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 01531392..76f21268 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -12,6 +12,10 @@ History the internal iterator type. * Fixed a ``RuntimeWarning`` or ``RuntimeError`` on free-threaded Python on macOS when a ``Reader`` failed to open or was used without ``__init__``. + * 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``. Reinitializing a ``Metadata`` now raises + ``ValueError``. 3.2.0 (2026-09-10) ++++++++++++++++++ diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 5eeb5323..6e986a52 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -67,6 +67,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 +85,7 @@ typedef struct { PyObject_HEAD /* no semicolon */ Reader_obj *reader; struct record *next; + uint64_t generation; } ReaderIter_obj; typedef struct { @@ -308,6 +311,16 @@ static void reader_release_write_lock(Reader_obj *reader) { // Reader implementation // ============================================================================= +// 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_new(PyTypeObject *type, PyObject *UNUSED(args), PyObject *UNUSED(kwds)) { PyObject *self = type->tp_alloc(type, 0); @@ -379,16 +392,20 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { } Reader_obj *mmdb_obj = (Reader_obj *)self; - if (!mmdb_obj) { + if (reader_acquire_write_lock(mmdb_obj) != 0) { Py_XDECREF(filepath); free(mmdb); - PyErr_NoMemory(); return -1; } + // A second init reopens the reader, as in the pure Python reader. Close + // the old database first. A failed open then leaves the reader closed. + reader_close_database(mmdb_obj); + int const status = MMDB_open(filename, MMDB_MODE_MMAP, mmdb); if (status != MMDB_SUCCESS) { + reader_release_write_lock(mmdb_obj); free(mmdb); PyErr_Format(state->MaxMindDB_error, "Error opening database file (%s). Is this a valid " @@ -398,10 +415,15 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { return -1; } - Py_XDECREF(filepath); - 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; } @@ -681,13 +703,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); @@ -727,7 +743,10 @@ static void Reader_dealloc(PyObject *self) { 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) { @@ -749,6 +768,7 @@ static PyObject *Reader_iter(PyObject *obj) { return NULL; } + uint64_t const generation = reader->generation; reader_release_read_lock(reader); ReaderIter_obj *ri = (ReaderIter_obj *)PyType_GenericAlloc( @@ -759,6 +779,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)); @@ -799,6 +820,14 @@ static PyObject *ReaderIter_next(PyObject *self) { 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; @@ -965,7 +994,9 @@ 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) { @@ -1003,27 +1034,35 @@ static int Metadata_init(PyObject *self, PyObject *args, PyObject *kwds) { 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); + // Refuse a second init, as Reader_init does. Replacing a field would leak + // the old value or free it while a getter uses it. On free-threaded + // builds, the critical section makes the check and the stores atomic. + int status = 0; +#ifdef Py_GIL_DISABLED + Py_BEGIN_CRITICAL_SECTION(self); +#endif + if (obj->binary_format_major_version != NULL) { + PyErr_SetString(PyExc_ValueError, + "Attempt to reinitialize a MaxMind DB Metadata."); + status = -1; + } else { + 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); + } +#ifdef Py_GIL_DISABLED + Py_END_CRITICAL_SECTION(); +#endif - return 0; + return status; } static void Metadata_dealloc(PyObject *self) { @@ -1037,7 +1076,9 @@ static void Metadata_dealloc(PyObject *self) { Py_XDECREF(obj->languages); Py_XDECREF(obj->node_count); Py_XDECREF(obj->record_size); + PyTypeObject *type = Py_TYPE(self); PyObject_Del(self); + Py_DECREF(type); } static PyObject * diff --git a/tests/reader_test.py b/tests/reader_test.py index 38a000f2..dc91af7e 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -6,11 +6,14 @@ import multiprocessing import os import pathlib +import subprocess import sys +import sysconfig import tempfile +import textwrap import threading import unittest -from typing import TYPE_CHECKING, cast +from typing import TYPE_CHECKING, Any, cast from unittest import mock import maxminddb @@ -988,6 +991,104 @@ def test_enter_uninitialized_reader(self) -> None: with self.assertRaisesRegex(ValueError, "closed MaxMind DB"): reader.__enter__() + def test_reinitialize_reader_reopens_it(self) -> None: + reader = maxminddb.extension.Reader( + f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb", + ) + self.addCleanup(reader.close) + iterator = iter(reader) + next(iterator) + reader.__init__(_DECODER_DB) # type: ignore[misc] + self.assertEqual(reader.metadata().database_type, "MaxMind DB Decoder Test") + # The records of the iterator point into the old database. + with self.assertRaisesRegex(ValueError, "reopened MaxMind DB"): + next(iterator) + + reader.close() + reader.__init__(_DECODER_DB) # type: ignore[misc] + self.assertFalse(reader.closed) + self.assertIsNotNone(reader.get("::1.1.1.0")) + + 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") + """, + ) + # 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") + + 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_is_refused(self) -> None: + metadata = maxminddb.extension.Metadata(**_METADATA_FIELDS) + with self.assertRaisesRegex(ValueError, "reinitialize"): + 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)) From 64ca4e36aa011cc1ed77e7a73b15a4f1ec1dc4bb Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sat, 3 Oct 2026 05:35:14 +0000 Subject: [PATCH 04/26] Pass a Py_ssize_t length to Py_BuildValue The module defines PY_SSIZE_T_CLEAN, so the y# format reads the length as a Py_ssize_t. ReaderIter_next passed an int, which is undefined behavior on 64-bit platforms: Py_BuildValue reads 8 bytes from a 4-byte argument. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 6e986a52..e296bc69 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -921,7 +921,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 From af912dde1695009dda41e983ee55a7252d80eb46 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Fri, 2 Oct 2026 23:21:47 +0000 Subject: [PATCH 05/26] Fix a reference leak when a map key fails to decode from_map built the dict, then returned NULL without releasing it when a key failed to decode, for example a map key that is not valid UTF-8. The sibling path for a failed value already released the dict. The cyclic garbage collector does not free an object with a leaked reference, so each failed lookup kept one empty dict, about 64 bytes. The new test makes 2,000 failed lookups and checks the memory that tracemalloc reports. Before the fix, the lookups kept about 128 KB. Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 1 + tests/reader_test.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index e296bc69..36a62a12 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -1152,6 +1152,7 @@ static PyObject *from_map(maxminddb_state *state, if (!key) { // PyUnicode_FromStringAndSize will set an appropriate exception // in this case. + Py_DECREF(py_obj); return NULL; } diff --git a/tests/reader_test.py b/tests/reader_test.py index dc91af7e..59cb3f40 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1,6 +1,7 @@ from __future__ import annotations import contextlib +import gc import io import ipaddress import multiprocessing @@ -12,6 +13,7 @@ import tempfile import textwrap import threading +import tracemalloc import unittest from typing import TYPE_CHECKING, Any, cast from unittest import mock @@ -945,6 +947,28 @@ class TestExtensionReader(BaseTestReader): if has_maxminddb_extension(): reader_class = maxminddb.extension.Reader + 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"), From 571f5eaf3c04e9f8eba0f94205a474821877006c Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sat, 3 Oct 2026 03:24:41 +0000 Subject: [PATCH 06/26] Check the PyDict_SetItem result in from_map from_map ignored a failure from PyDict_SetItem, such as a MemoryError while the dict resizes. It then returned the dict with the exception still set, and the caller raised SystemError. Now it releases the dict and returns NULL, so the original exception reaches the caller. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 36a62a12..9e2d02fa 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -1164,9 +1164,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; From ff1b0ef9cb7529cc7b0b3ea4b5d9d9ae70f766d1 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sat, 3 Oct 2026 06:45:10 +0000 Subject: [PATCH 07/26] Reject a map key that is not a string from_map read every map key from the utf8_string member of the entry union. libmaxminddb does not check the key type, so a database with a key of another type, such as a uint16, made from_map read an integer as a pointer. The process crashed with a segmentation fault. Raise InvalidDatabaseError for a key that is not a UTF-8 string. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 6 ++++-- extension/maxminddb.c | 8 ++++++++ tests/reader_test.py | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 76f21268..8917aaa4 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -10,12 +10,14 @@ History * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and the internal iterator type. - * Fixed a ``RuntimeWarning`` or ``RuntimeError`` on free-threaded Python on - macOS when a ``Reader`` failed to open or was used without ``__init__``. + * Fixed a segmentation fault on a database with a map key that is not a + string. Such a database now raises ``InvalidDatabaseError``. * 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``. Reinitializing a ``Metadata`` now raises ``ValueError``. + * 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 9e2d02fa..cad7aba4 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -1146,6 +1146,14 @@ static PyObject *from_map(maxminddb_state *state, for (i = 0; i < map_size && *entry_data_list; i++) { *entry_data_list = (*entry_data_list)->next; + // 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); diff --git a/tests/reader_test.py b/tests/reader_test.py index 59cb3f40..da62c853 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -947,6 +947,21 @@ 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): From 51791b8b5d970f92dc7c5fa645bde2c7bd15475d Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sat, 3 Oct 2026 06:45:27 +0000 Subject: [PATCH 08/26] Decode uint32 values with PyLong_FromUnsignedLong from_entry_data_list passed uint32 values to PyLong_FromLong. On platforms where a C long has 32 bits, such as Windows and 32-bit Linux, a value of 2**31 or more became negative, for example an ASN of 4200000000. The pure Python reader returns the correct value. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 2 ++ extension/maxminddb.c | 3 ++- tests/reader_test.py | 9 +++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/HISTORY.rst b/HISTORY.rst index 8917aaa4..d9686872 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -12,6 +12,8 @@ History 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``. Reinitializing a ``Metadata`` now raises diff --git a/extension/maxminddb.c b/extension/maxminddb.c index cad7aba4..cfe37cf5 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -1113,7 +1113,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: diff --git a/tests/reader_test.py b/tests/reader_test.py index da62c853..171fca92 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -523,6 +523,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", From ed76a5ce7926a988a263d9a922849483511f0af6 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Fri, 2 Oct 2026 23:21:59 +0000 Subject: [PATCH 09/26] Fix a reference leak on a corrupt metadata type Reader.metadata() returned NULL without releasing the decoded object when it was not a dict. libmaxminddb validates the metadata when it opens the database, so an opened database reaches this path only through a bug, but the refcount handling was still wrong. Co-Authored-By: Claude Opus 4.8 --- extension/maxminddb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index cfe37cf5..7f813483 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -677,6 +677,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; } From aad2b72fd5aa130ff5f5681c9814304a24d81b52 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Sat, 3 Oct 2026 04:30:08 +0000 Subject: [PATCH 10/26] Check the result of Reader_close Reader__exit__ and Reader_dealloc called Reader_close and ignored the result. Each successful close leaked a reference to None, which matters before Python 3.12, where None is not immortal. On free-threaded builds, a failure to take the write lock left an exception set: __exit__ hid it behind a successful return, and dealloc left it pending for unrelated code to find. __exit__ now returns the result of Reader_close. dealloc no longer calls Reader_close. It closes the database directly, without the lock. At a reference count of 0 no other thread can use the reader, because each iterator holds a reference to it. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 7f813483..5ba32226 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -732,15 +732,14 @@ 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); From 7870349829c053937f778d7a2a96e0f6e7d0e66c Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Mon, 5 Oct 2026 15:22:44 +0000 Subject: [PATCH 11/26] Build Metadata in tp_new Metadata is an immutable value object, but Metadata_init set its fields. That allowed a Metadata with NULL fields, after Metadata.__new__ or a failed init, and a second init. Each state needed its own guard: a reinit check, a critical section on free-threaded builds, and Py_XDECREF in dealloc. Metadata_new now parses the arguments and sets every field, and the type has no tp_init. Each Metadata then has all fields set, and a second __init__ call changes nothing, because object.__init__ ignores the arguments when a type overrides tp_new. Metadata.__new__ with no arguments now raises TypeError. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 3 +- extension/maxminddb.c | 73 +++++++++++++++++-------------------------- tests/reader_test.py | 12 +++---- 3 files changed, 35 insertions(+), 53 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index d9686872..8362bef9 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -16,8 +16,7 @@ History 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``. Reinitializing a ``Metadata`` now raises - ``ValueError``. + raises ``ValueError``. 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__``. diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 5ba32226..d28d3521 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -999,8 +999,10 @@ static void ReaderIter_dealloc(PyObject *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; @@ -1029,53 +1031,36 @@ 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; - - // Refuse a second init, as Reader_init does. Replacing a field would leak - // the old value or free it while a getter uses it. On free-threaded - // builds, the critical section makes the check and the stores atomic. - int status = 0; -#ifdef Py_GIL_DISABLED - Py_BEGIN_CRITICAL_SECTION(self); -#endif - if (obj->binary_format_major_version != NULL) { - PyErr_SetString(PyExc_ValueError, - "Attempt to reinitialize a MaxMind DB Metadata."); - status = -1; - } else { - 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); - } -#ifdef Py_GIL_DISABLED - Py_END_CRITICAL_SECTION(); -#endif - - return status; + 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) { Metadata_obj *obj = (Metadata_obj *)self; - Py_XDECREF(obj->binary_format_major_version); - Py_XDECREF(obj->binary_format_minor_version); - Py_XDECREF(obj->build_epoch); - Py_XDECREF(obj->database_type); - Py_XDECREF(obj->description); - Py_XDECREF(obj->ip_version); - Py_XDECREF(obj->languages); - Py_XDECREF(obj->node_count); - Py_XDECREF(obj->record_size); + Py_DECREF(obj->binary_format_major_version); + Py_DECREF(obj->binary_format_minor_version); + Py_DECREF(obj->build_epoch); + Py_DECREF(obj->database_type); + Py_DECREF(obj->description); + Py_DECREF(obj->ip_version); + 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); @@ -1351,7 +1336,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}, diff --git a/tests/reader_test.py b/tests/reader_test.py index 171fca92..8666695e 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1013,11 +1013,10 @@ class TestExtensionReaderWithIPObjects(BaseTestReader): class TestExtensionObjects(unittest.TestCase): """Objects in states that crashed the extension.""" - def test_uninitialized_metadata(self) -> None: + def test_new_metadata_requires_arguments(self) -> None: metadata_class = maxminddb.extension.Metadata - metadata = metadata_class.__new__(metadata_class) - self.assertIsNone(metadata.languages) - del 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"): @@ -1117,10 +1116,9 @@ def test_initialize_after_close_on_uninitialized_reader(self) -> None: with reader: self.assertIsNotNone(reader.get("::1.1.1.0")) - def test_reinitialize_metadata_is_refused(self) -> None: + def test_reinitialize_metadata_changes_nothing(self) -> None: metadata = maxminddb.extension.Metadata(**_METADATA_FIELDS) - with self.assertRaisesRegex(ValueError, "reinitialize"): - metadata.__init__(**{**_METADATA_FIELDS, "record_size": 28}) # type: ignore[misc] + metadata.__init__(**{**_METADATA_FIELDS, "record_size": 28}) # type: ignore[misc] self.assertEqual(metadata.record_size, 24) @unittest.skipUnless( From 9a25a090b26f4fbdc05489d845ce16ce15481c2d Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Mon, 5 Oct 2026 16:13:11 +0000 Subject: [PATCH 12/26] Reopen the pure Python reader like the C extension A second __init__ on the pure Python Reader replaced the buffer, but it did not close the old one. An iterator from before the second init then walked the new database with node numbers from the old one, and returned networks that are not in either database. The C extension now closes the old database and stops such an iterator. Do the same here: __init__ closes the old buffer once the new one is loaded and increments a generation count. An iterator from an older generation raises the same ValueError. A count is needed, because a source can return the same buffer object again: BytesIO.read() does after seek(0). The check is one integer comparison for each node during iteration. Lookups do not change. The reinitialization tests now run for every mode of both readers, so the two keep the same behavior. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 4 ++ maxminddb/reader.py | 39 +++++++++++++--- tests/reader_test.py | 104 +++++++++++++++++++++++++++++++++++-------- 3 files changed, 123 insertions(+), 24 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 8362bef9..33a9872a 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -6,6 +6,10 @@ 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. * C extension: * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and diff --git a/maxminddb/reader.py b/maxminddb/reader.py index b4def914..cb814c2a 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -27,6 +27,7 @@ from maxminddb.types import Record _IPV4_MAX_NUM = 2**32 +_REOPENED = "Attempt to iterate over a reopened MaxMind DB. Create a new iterator." class Reader: @@ -45,6 +46,8 @@ class Reader: _metadata: Metadata _record_size: int _ipv4_start: int + # Incremented on each open, so an iterator can detect a reopen. + _generation: int def __init__( self, @@ -65,7 +68,13 @@ def __init__( a path. This mode implies MODE_MEMORY. """ + old_buffer = getattr(self, "_buffer", None) filename = self._load_buffer(database, mode) + # A second __init__ reopens the reader, as in the C extension. + _close_buffer(old_buffer) + # A source can return the same buffer object again, such as BytesIO, + # so count the opens instead of comparing buffers. + self._generation = getattr(self, "_generation", 0) + 1 # Include validation errors in this cleanup scope. TRY301 is suppressed # because the handler only closes the buffer and re-raises the error. @@ -191,9 +200,19 @@ def get_with_prefix_len( return None, prefix_len def __iter__(self) -> Iterator: - return self._generate_children(0, 0, 0) + return self._generate_children(0, 0, 0, self._generation) - def _generate_children(self, node: int, depth: int, ip_acc: int) -> Iterator: + def _generate_children( + self, + node: int, + depth: int, + ip_acc: int, + generation: int, + ) -> Iterator: + # The node numbers come from the database of this generation. After a + # second __init__, stop, as the C extension does. + if self._generation != generation: + raise ValueError(_REOPENED) if ip_acc != 0 and node == self._ipv4_start: # Skip nodes aliased to IPv4 return @@ -214,9 +233,11 @@ def _generate_children(self, node: int, depth: int, ip_acc: int) -> Iterator: left = self._read_node(node, 0) ip_acc <<= 1 depth += 1 - yield from self._generate_children(left, depth, ip_acc) + yield from self._generate_children(left, depth, ip_acc, generation) + if self._generation != generation: + raise ValueError(_REOPENED) right = self._read_node(node, 1) - yield from self._generate_children(right, depth, ip_acc | 1) + yield from self._generate_children(right, depth, ip_acc | 1, generation) def _find_address_in_tree(self, packed: bytearray) -> tuple[int, int]: bit_count = len(packed) * 8 @@ -320,8 +341,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 +406,9 @@ 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) -> None: + # 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 8666695e..c4b858e7 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -785,6 +785,55 @@ def test_closed(self) -> None: reader.close() self.assertEqual(reader.closed, True) + def _reinitialize(self, reader: Any, path: str, mode: int) -> None: # noqa: ANN401 + if mode == MODE_FD: + with open(path, "rb") as database: + reader.__init__(database, mode) + else: + reader.__init__(path, mode) + + def test_close_uninitialized_reader(self) -> None: + reader = self.reader_class.__new__(self.reader_class) + 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_failed_reinitialize(self) -> None: + reader = open_database(_DECODER_DB, self.mode) + self.addCleanup(reader.close) + + # Argument and file errors leave 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) + self.assertFalse(reader.closed) + self.assertIsNotNone(reader.get("::1.1.1.0")) + + # A file that is not a database closes the reader. + with self.assertRaises(InvalidDatabaseError): + self._reinitialize(reader, "README.rst", self.mode) + self.assertTrue(reader.closed) + def test_closed_metadata(self) -> None: reader = open_database( "tests/data/test-data/MaxMind-DB-test-decoder.mmdb", @@ -1038,24 +1087,6 @@ def test_enter_uninitialized_reader(self) -> None: with self.assertRaisesRegex(ValueError, "closed MaxMind DB"): reader.__enter__() - def test_reinitialize_reader_reopens_it(self) -> None: - reader = maxminddb.extension.Reader( - f"{_TEST_DATA_DIR}/MaxMind-DB-test-ipv4-24.mmdb", - ) - self.addCleanup(reader.close) - iterator = iter(reader) - next(iterator) - reader.__init__(_DECODER_DB) # type: ignore[misc] - self.assertEqual(reader.metadata().database_type, "MaxMind DB Decoder Test") - # The records of the iterator point into the old database. - with self.assertRaisesRegex(ValueError, "reopened MaxMind DB"): - next(iterator) - - reader.close() - reader.__init__(_DECODER_DB) # type: ignore[misc] - self.assertFalse(reader.closed) - self.assertIsNotNone(reader.get("::1.1.1.0")) - 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 @@ -1190,6 +1221,43 @@ def setUp(self) -> None: class TestReaderInitialization(unittest.TestCase): + 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" From 88b7e076960a3997d024c81cbfc4fbc2ee7c7fab Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 14:51:25 +0000 Subject: [PATCH 13/26] Report closed as True before init A reader made with Reader.__new__ alone has no open database. The C reader reported closed as None, and the pure Python reader raised AttributeError, so code such as "if not reader.closed" treated the reader as open. Reader_new now sets closed to True, and the pure Python Reader has a class default of True. Both stay True until init succeeds. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 2 ++ maxminddb/reader.py | 3 ++- tests/reader_test.py | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index d28d3521..c8566d65 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -340,6 +340,8 @@ Reader_new(PyTypeObject *type, PyObject *UNUSED(args), PyObject *UNUSED(kwds)) { return NULL; } + // No database is open until Reader_init succeeds. + ((Reader_obj *)self)->closed = Py_True; return self; } diff --git a/maxminddb/reader.py b/maxminddb/reader.py index cb814c2a..26b5e1b3 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -41,7 +41,8 @@ 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 diff --git a/tests/reader_test.py b/tests/reader_test.py index c4b858e7..9830d5a6 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -794,6 +794,7 @@ def _reinitialize(self, reader: Any, path: str, mode: int) -> None: # noqa: ANN 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) From 610c387af0a14294125cb7e9a9812e41cedf4d37 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 14:51:52 +0000 Subject: [PATCH 14/26] Count a reopen before closing the old buffer A second __init__ on the pure Python Reader closed the old buffer before it incremented the generation. If the close raised, for example BufferError from an mmap with an exported memoryview, the generation stayed the same, and an old iterator walked the new buffer with node numbers from the old one. It also closed the old buffer when the source returned the same object again, such as a MODE_FD source whose read() returns one mmap. The new reader then used a closed buffer. Increment the generation first, and skip the close when the old buffer is the one now in use. Co-Authored-By: Claude Opus 5.5 --- maxminddb/reader.py | 14 +++++++++----- tests/reader_test.py | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/maxminddb/reader.py b/maxminddb/reader.py index 26b5e1b3..e3c40579 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -71,11 +71,12 @@ def __init__( """ old_buffer = getattr(self, "_buffer", None) filename = self._load_buffer(database, mode) - # A second __init__ reopens the reader, as in the C extension. - _close_buffer(old_buffer) - # A source can return the same buffer object again, such as BytesIO, - # so count the opens instead of comparing buffers. + # A second __init__ reopens the reader, as in the C extension. A source + # can return the same buffer object again, such as BytesIO, so count + # the opens instead of comparing buffers. Count first, so an old + # iterator stops even if closing the old buffer fails. self._generation = getattr(self, "_generation", 0) + 1 + _close_buffer(old_buffer, keep=self._buffer) # Include validation errors in this cleanup scope. TRY301 is suppressed # because the handler only closes the buffer and re-raises the error. @@ -409,7 +410,10 @@ def search_tree_size(self) -> int: return self.node_count * self.node_byte_size -def _close_buffer(buffer: object) -> None: +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 9830d5a6..5b81e3ff 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -4,6 +4,7 @@ import gc import io import ipaddress +import mmap import multiprocessing import os import pathlib @@ -1222,6 +1223,20 @@ def setUp(self) -> None: class TestReaderInitialization(unittest.TestCase): + 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") From 8b91704f2f06ff07fb02103bc8a61754da713dd4 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 14:53:11 +0000 Subject: [PATCH 15/26] Keep the old database when a reinit fails A failed second __init__ left the two readers in different states. The C reader closed its old database before it opened the new one, so a file that is not a database left it closed. The pure Python reader replaced the buffer, but it kept the metadata, decoder and tree layout of the old database. After the error, it reported closed, but metadata() and get() still used the old layout on the new, closed buffer. Both readers now keep the old database when a reinit fails. The C reader opens the new database before it takes the write lock, and swaps it in under the lock. This also stops it from holding the write lock while it opens and maps the file. The pure Python reader saves its state first, and restores it when the new database fails to load. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 5 +++-- extension/maxminddb.c | 24 ++++++++++++------------ maxminddb/reader.py | 27 +++++++++++++++------------ tests/reader_test.py | 28 ++++++++++++---------------- 4 files changed, 42 insertions(+), 42 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 33a9872a..c1c8e9eb 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -9,7 +9,7 @@ History * 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. + numbers from the old one. A failed ``__init__`` keeps the old database. * C extension: * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and @@ -20,7 +20,8 @@ History 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``. Reinitializing a ``Metadata`` changes nothing. + 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__``. diff --git a/extension/maxminddb.c b/extension/maxminddb.c index c8566d65..9ca5ae06 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -393,21 +393,11 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { return -1; } - Reader_obj *mmdb_obj = (Reader_obj *)self; - if (reader_acquire_write_lock(mmdb_obj) != 0) { - Py_XDECREF(filepath); - free(mmdb); - return -1; - } - - // A second init reopens the reader, as in the pure Python reader. Close - // the old database first. A failed open then leaves the reader closed. - reader_close_database(mmdb_obj); - + // 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_release_write_lock(mmdb_obj); free(mmdb); PyErr_Format(state->MaxMindDB_error, "Error opening database file (%s). Is this a valid " @@ -417,6 +407,16 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { return -1; } + 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. diff --git a/maxminddb/reader.py b/maxminddb/reader.py index e3c40579..4c437378 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -69,18 +69,14 @@ def __init__( a path. This mode implies MODE_MEMORY. """ - old_buffer = getattr(self, "_buffer", None) - filename = self._load_buffer(database, mode) - # A second __init__ reopens the reader, as in the C extension. A source - # can return the same buffer object again, such as BytesIO, so count - # the opens instead of comparing buffers. Count first, so an old - # iterator stops even if closing the old buffer fails. - self._generation = getattr(self, "_generation", 0) + 1 - _close_buffer(old_buffer, keep=self._buffer) + # A failed __init__ keeps the old database, if any, as in the C + # extension. + old_state = self.__dict__.copy() - # Include validation errors in this cleanup scope. TRY301 is suppressed - # because the handler only closes the buffer and re-raises the error. + # TRY301 is suppressed because the handler only restores the old state + # 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), @@ -147,10 +143,17 @@ 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"), keep=old_state.get("_buffer")) + self.__dict__ = old_state raise + # A second __init__ reopens the reader, as in the C extension. A source + # can return the same buffer object again, such as BytesIO, so count + # the opens instead of comparing buffers. Count first, so an old + # iterator stops even if closing the old buffer fails. + self._generation = old_state.get("_generation", 0) + 1 + _close_buffer(old_state.get("_buffer"), keep=self._buffer) + def metadata(self) -> Metadata: """Return the metadata associated with the MaxMind DB file.""" return self._metadata diff --git a/tests/reader_test.py b/tests/reader_test.py index 5b81e3ff..b429bd8f 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -822,19 +822,17 @@ def test_failed_reinitialize(self) -> None: reader = open_database(_DECODER_DB, self.mode) self.addCleanup(reader.close) - # Argument and file errors leave the old database open. + # 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) - self.assertFalse(reader.closed) - self.assertIsNotNone(reader.get("::1.1.1.0")) - - # A file that is not a database closes the reader. with self.assertRaises(InvalidDatabaseError): self._reinitialize(reader, "README.rst", self.mode) - self.assertTrue(reader.closed) + self.assertFalse(reader.closed) + self.assertEqual(reader.metadata().database_type, "MaxMind DB Decoder Test") + self.assertIsNotNone(reader.get("::1.1.1.0")) def test_closed_metadata(self) -> None: reader = open_database( @@ -1333,21 +1331,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): From 5f37e14b22b8ff2d84205d83bfc9d499fa2cecb0 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 14:54:22 +0000 Subject: [PATCH 16/26] Check a pure Python iterator only between records The pure Python iterator checked the generation when it entered each node and after each left subtree. That missed cases, and it cost a check for each node: - After a reinit while the iterator was at the last record of a walk that went right at every node, the generator ended without raising. - close() did not stop it. In MODE_MEMORY and MODE_FD, the iterator kept returning records. In the other modes, it raised the error of the closed mmap or file. __iter__ now returns a small generator that checks once per record, before the walk resumes. After a reinit, it raises the existing "reopened" ValueError. After close(), it raises the same "closed" ValueError as the C extension. _generate_children no longer checks anything. Co-Authored-By: Claude Opus 5.5 --- HISTORY.rst | 1 + maxminddb/reader.py | 38 +++++++++++++++++++++----------------- tests/reader_test.py | 24 ++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index c1c8e9eb..ae86d464 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -10,6 +10,7 @@ History 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 diff --git a/maxminddb/reader.py b/maxminddb/reader.py index 4c437378..e927ddcb 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -28,6 +28,7 @@ _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: @@ -205,19 +206,24 @@ def get_with_prefix_len( return None, prefix_len def __iter__(self) -> Iterator: - return self._generate_children(0, 0, 0, self._generation) - - def _generate_children( - self, - node: int, - depth: int, - ip_acc: int, - generation: int, - ) -> Iterator: - # The node numbers come from the database of this generation. After a - # second __init__, stop, as the C extension does. - if self._generation != generation: - raise ValueError(_REOPENED) + 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: # Skip nodes aliased to IPv4 return @@ -238,11 +244,9 @@ def _generate_children( left = self._read_node(node, 0) ip_acc <<= 1 depth += 1 - yield from self._generate_children(left, depth, ip_acc, generation) - if self._generation != generation: - raise ValueError(_REOPENED) + yield from self._generate_children(left, depth, ip_acc) right = self._read_node(node, 1) - yield from self._generate_children(right, depth, ip_acc | 1, generation) + yield from self._generate_children(right, depth, ip_acc | 1) def _find_address_in_tree(self, packed: bytearray) -> tuple[int, int]: bit_count = len(packed) * 8 diff --git a/tests/reader_test.py b/tests/reader_test.py index b429bd8f..259031a7 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -818,6 +818,30 @@ def test_reinitialize_reopens_the_reader(self) -> None: 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) From b470da73ffd5dc11d4ccce624ce936aa63b17567 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 14:54:40 +0000 Subject: [PATCH 17/26] Check for a short entry list in from_map from_map tested *entry_data_list before it moved to the next entry, then read the key from the new entry without a test. If libmaxminddb returned a list shorter than the map size, for example a system libmaxminddb of another version, the process crashed on a NULL pointer. Raise InvalidDatabaseError with the message that from_entry_data_list uses for the same case. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 9ca5ae06..ca82ef3e 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -1134,6 +1134,16 @@ 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, + "Error while looking up data. Your database may be " + "corrupt or you have found a bug in libmaxminddb."); + 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) { From b4694482a5029f962022de9e884feaccd224e196 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 14:54:59 +0000 Subject: [PATCH 18/26] Move reader_close_database below its first caller Functions in this file come after the functions that call them. Move reader_close_database below Reader_init and declare it with the other forward declarations. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index ca82ef3e..7e650a88 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -130,6 +130,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); @@ -311,16 +312,6 @@ static void reader_release_write_lock(Reader_obj *reader) { // Reader implementation // ============================================================================= -// 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_new(PyTypeObject *type, PyObject *UNUSED(args), PyObject *UNUSED(kwds)) { PyObject *self = type->tp_alloc(type, 0); @@ -429,6 +420,16 @@ static int Reader_init(PyObject *self, PyObject *args, PyObject *kwds) { 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) { From c060c9763cdf581edf85002504530ff4d37b7d91 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:23:00 +0000 Subject: [PATCH 19/26] Refuse a close or reinit during a read on GIL builds With the GIL, the reader lock did nothing. Python code can still run during a decode. For example, on Python 3.10 and 3.11, an allocation in from_map can start a GC, which runs a finalizer. If that finalizer called close() or __init__, the database was unmapped under the decoder, and the process crashed. close() already crashed this way, and the reinit change added a second path to the same crash. In GIL-only builds, the lock now counts the reads in progress, and taking the write lock fails with RuntimeError while the count is above 0. The GIL serializes the count, and a write section runs no Python code. A refused reinit keeps the old database, because Reader_init closes the new one when it cannot take the lock. The new test runs in a subprocess. It moves the setup of the existing finalizer test into a shared helper. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 26 +++++++++++++++----------- tests/reader_test.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 7e650a88..00386d20 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 @@ -169,8 +171,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 } @@ -226,8 +227,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 } @@ -247,8 +247,7 @@ static void reader_release_read_lock(Reader_obj *reader) { } #else - // GIL-only mode - no-op - (void)reader; + reader->rwlock.readers--; #endif } @@ -282,8 +281,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 } diff --git a/tests/reader_test.py b/tests/reader_test.py index 259031a7..162a1749 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1141,6 +1141,48 @@ def __fspath__(self): 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"): From bd400924bcb4b44801d7d941e143c7b38d2d1992 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:23:45 +0000 Subject: [PATCH 20/26] Give _generation a class default The pure Python Reader set _generation only in __init__, and __iter__ reads it. iter() on a reader with no database, from Reader.__new__ or a failed first __init__, raised AttributeError. The C reader raises the "closed" ValueError. Default _generation to 0, as closed defaults to True. Iteration then raises the "closed" ValueError in both readers. Co-Authored-By: Claude Opus 5.5 --- maxminddb/reader.py | 2 +- tests/reader_test.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/maxminddb/reader.py b/maxminddb/reader.py index e927ddcb..05248147 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -49,7 +49,7 @@ class Reader: _record_size: int _ipv4_start: int # Incremented on each open, so an iterator can detect a reopen. - _generation: int + _generation: int = 0 def __init__( self, diff --git a/tests/reader_test.py b/tests/reader_test.py index 162a1749..b6ddd799 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -793,6 +793,12 @@ def _reinitialize(self, reader: Any, path: str, mode: int) -> None: # noqa: ANN else: reader.__init__(path, mode) + 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) From 6c7389f13961a04523d2733ee0d1261904009796 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:24:42 +0000 Subject: [PATCH 21/26] Switch to a new pure Python database in one step A second __init__ on the pure Python Reader set _buffer, _metadata, _decoder and closed one at a time, and incremented the generation only at the end. Another thread that read from the reader, or stepped an old iterator, during the load could use the new buffer with the old metadata and decoder, and return wrong records with no error. __init__ now loads the database into a new object with _load, and then switches to it with one assignment to __dict__. A failed load leaves the reader as it was, so the code that saved and restored the old state is gone. A read that is already in progress when the switch happens can still see both databases, because it reads several attributes. Preventing that would need a lock on every lookup, so the docstring now says that a second __init__, like close(), can disrupt reads on other threads. Co-Authored-By: Claude Opus 5.5 --- maxminddb/reader.py | 39 ++++++++++++++++++++++++--------------- tests/reader_test.py | 25 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/maxminddb/reader.py b/maxminddb/reader.py index 05248147..d59f5b19 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -69,13 +69,30 @@ def __init__( * MODE_FD - the param passed via database is a file descriptor, not a path. This mode implies MODE_MEMORY. - """ - # A failed __init__ keeps the old database, if any, as in the C - # extension. - old_state = self.__dict__.copy() + 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. - # TRY301 is suppressed because the handler only restores the old state - # and re-raises the error. + """ + # Load into a new object, then switch to it 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. + new = Reader.__new__(type(self)) + 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__ = 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( @@ -144,17 +161,9 @@ def __init__( ipv4_start = node self._ipv4_start = ipv4_start except BaseException: - _close_buffer(self.__dict__.get("_buffer"), keep=old_state.get("_buffer")) - self.__dict__ = old_state + _close_buffer(self.__dict__.get("_buffer")) raise - # A second __init__ reopens the reader, as in the C extension. A source - # can return the same buffer object again, such as BytesIO, so count - # the opens instead of comparing buffers. Count first, so an old - # iterator stops even if closing the old buffer fails. - self._generation = old_state.get("_generation", 0) + 1 - _close_buffer(old_state.get("_buffer"), keep=self._buffer) - def metadata(self) -> Metadata: """Return the metadata associated with the MaxMind DB file.""" return self._metadata diff --git a/tests/reader_test.py b/tests/reader_test.py index b6ddd799..848dd084 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1293,6 +1293,31 @@ def setUp(self) -> None: class TestReaderInitialization(unittest.TestCase): + 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) From d51d684695f4f5bb93ea791bc554ea5319307e41 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:25:12 +0000 Subject: [PATCH 22/26] Share the corrupt-data error message from_entry_data_list and from_map raised the same error text when an entry data list ended too early, as two copies of the string. Define the message once, so the two cannot drift apart. Co-Authored-By: Claude Opus 5.5 --- extension/maxminddb.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 00386d20..96679f17 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -145,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 @@ -1077,9 +1082,7 @@ 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; } @@ -1142,9 +1145,7 @@ static PyObject *from_map(maxminddb_state *state, // 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, - "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); Py_DECREF(py_obj); return NULL; } From 71ee436d5c1fa200829dc34456dbfc3a23502213 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:25:45 +0000 Subject: [PATCH 23/26] Share how the tests pass a database in MODE_FD get_reader_from_file_descriptor and _reinitialize both opened the path as a binary file for MODE_FD and passed the path in the other modes. Add _database_source, which yields the argument for a mode, and use it in both, so the two cannot drift apart. Co-Authored-By: Claude Opus 5.5 --- tests/reader_test.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/reader_test.py b/tests/reader_test.py index 848dd084..5a279016 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -38,6 +38,7 @@ if TYPE_CHECKING: from collections.abc import Iterator + from typing import IO from maxminddb.reader import Reader @@ -115,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): @@ -787,11 +794,8 @@ def test_closed(self) -> None: self.assertEqual(reader.closed, True) def _reinitialize(self, reader: Any, path: str, mode: int) -> None: # noqa: ANN401 - if mode == MODE_FD: - with open(path, "rb") as database: - reader.__init__(database, mode) - else: - reader.__init__(path, mode) + with _database_source(path, mode) as database: + reader.__init__(database, mode) def test_iterate_uninitialized_reader(self) -> None: reader = self.reader_class.__new__(self.reader_class) From 444aad4ea2a12df0dc0c96be59cf0a38b85da5e3 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:26:06 +0000 Subject: [PATCH 24/26] Move _reinitialize below the tests that call it Functions in this project come after their callers. Move the test helper below test_failed_reinitialize, the last test that uses it. Co-Authored-By: Claude Opus 5.5 --- tests/reader_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/reader_test.py b/tests/reader_test.py index 5a279016..ff1ccda3 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -793,10 +793,6 @@ def test_closed(self) -> None: reader.close() self.assertEqual(reader.closed, True) - 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_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(). @@ -868,6 +864,10 @@ def test_failed_reinitialize(self) -> None: 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", From 435b4f2a389fb7a4ab8087f74aa6c598c2154a5d Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:53:20 +0000 Subject: [PATCH 25/26] Keep subclass state when a pure Python reader opens __init__ loaded the database into a new object of the caller's type, then replaced self.__dict__ with the new object's dictionary. That broke subclasses of Reader in two ways: - The new object shared its dictionary with the reader. When it was freed, a subclass __del__ that calls close() closed the reader, so its first lookup raised ValueError. - Replacing the dictionary dropped the attributes that a subclass set before it called super().__init__(). Load into a base Reader, which has no __del__, and copy its state into the reader with one dict.update() call. That still switches all of the reader's attributes in one step, and it keeps the subclass's own attributes. Co-Authored-By: Claude Opus 5.5 --- maxminddb/reader.py | 12 +++++++----- tests/reader_test.py | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/maxminddb/reader.py b/maxminddb/reader.py index d59f5b19..c0ca346b 100644 --- a/maxminddb/reader.py +++ b/maxminddb/reader.py @@ -74,16 +74,18 @@ def __init__( progress on other threads fail or return wrong results. """ - # Load into a new object, then switch to it 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. - new = Reader.__new__(type(self)) + # 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__ = new.__dict__ + self.__dict__.update(new.__dict__) _close_buffer(old_buffer, keep=self._buffer) def _load( diff --git a/tests/reader_test.py b/tests/reader_test.py index ff1ccda3..017ffd68 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1297,6 +1297,30 @@ def setUp(self) -> None: class TestReaderInitialization(unittest.TestCase): + 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", From 013ca3d6185ea19d4ec2bdaa539c213b6b908601 Mon Sep 17 00:00:00 2001 From: Gregory Oschwald Date: Tue, 6 Oct 2026 16:54:23 +0000 Subject: [PATCH 26/26] Test that a reinit releases the old database The reinit tests checked reads from the new database and the stop of old iterators, but not that the old database is released. On main, a reinit leaked it in both readers. Check that the pure Python reader closes the old mmap or file buffer. For the C reader, reinit a reader on a private copy of a database 10 times, then check /proc/self/maps and /proc/self/fd: one mapping and no open descriptor remain, and close() removes the mapping. On main, the same steps leave 11 mappings. The C test runs only where /proc exists. Co-Authored-By: Claude Opus 5.5 --- tests/reader_test.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/reader_test.py b/tests/reader_test.py index 017ffd68..da91173f 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1214,6 +1214,35 @@ def _run_program(self, program: str) -> None: 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) @@ -1297,6 +1326,18 @@ 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: