Skip to content

Reject truncated values and non-string map keys #463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oschwald wants to merge 5 commits into
base: main
Choose a base branch
from
+373 −68
Open

Reject truncated values and non-string map keys #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc24faa
Reject values past the end of the data section
oschwald Oct 20, 2026
0684158
Reject non-string map keys in the pure Python reader
oschwald Oct 20, 2026
8288fab
Test non-string map keys in every reader mode
oschwald Oct 20, 2026
1afe10e
Decode map keys without the generic dispatch
oschwald Oct 20, 2026
22b26b2
Read to the end of the file in FileBuffer.rfind
oschwald Oct 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,16 @@ History
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.
* For a record map with a key that cannot be hashed, such as a list, the
pure Python reader raises ``InvalidDatabaseError`` instead of ``TypeError``,
as the C extension does.
* The pure Python reader now raises ``InvalidDatabaseError`` when a value,
search tree record or pointer goes past the end of the data section, when
the search tree overlaps the metadata, or when the file shrinks after open in
``MODE_FILE``. Previously, it could return a shorter or wrong value, or
decode bytes from the metadata section.
* The pure Python reader now raises ``InvalidDatabaseError`` when a record map
key is not a string, as the C extension does. Previously, it returned the
non-string key or raised ``TypeError``.
* An empty database file now raises ``InvalidDatabaseError`` in ``MODE_FILE``,
as in ``MODE_MEMORY``. Previously, it raised ``OSError``.

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, it raised OSError is true only where os.pread exists. On Windows, the old _read(-1, 0) called handle.read(-1), which returned b''. rfind then returned -1, so MODE_FILE already raised InvalidDatabaseError there.

🤖 Comment by Claude Opus 5.5.

* C extension:

* Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and
Expand Down
94 changes: 84 additions & 10 deletions maxminddb/decoder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
mmap = None # type: ignore[assignment]

from maxminddb.errors import InvalidDatabaseError
from maxminddb.file import FileBuffer

if TYPE_CHECKING:
from maxminddb.file import FileBuffer
from maxminddb.types import Record


Expand Down Expand Up @@ -49,6 +49,8 @@
"The MaxMind DB file's data section contains bad data "
"(unknown data type or corrupt data)"
)
# The same message as the C extension.
_NON_STRING_KEY = "Invalid map key: the key is not a string."


@dataclass
Expand All @@ -68,18 +70,31 @@ def __init__(
database_buffer: FileBuffer | mmap.mmap | bytes | bytearray,
pointer_base: int = 0,
pointer_test: bool = False, # noqa: FBT001, FBT002
data_end: int | None = None,
) -> None:
"""Create a Decoder for a MaxMind DB.
Arguments:
database_buffer: an mmap'd MaxMind DB file.
pointer_base: the base number to use when decoding a pointer
pointer_test: used for internal unit testing of pointer code
data_end: the offset where the decoded section ends. No payload
may extend past it, and no pointer may target it or a later
offset. Control and size bytes are not checked. The default

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data_end does not bound control bytes. A value with no payload decodes with no error when its control byte is at or after data_end. This applies to an empty map, an empty array, a boolean, and a size-0 int32. _decode_boolean (line 127) is one example.

Example: Decoder(b'\x02\x04\xa0' + tail, data_end=3).decode(0) returns [0, {}], [0, []], [0, True] or [0, 0] for each of those tails. All of them read past data_end.

The Reader is safe only because the first byte of the metadata marker (0xab, a uint16 with an 11-byte payload) always fails. A caller whose data_end is not at a marker does not get this protection. Also, _decode_key (line 245) checks the control byte, so Control and size bytes are not checked is only partly correct.

🤖 Comment by Claude Opus 5.5.

is the end of the buffer.
"""
self._pointer_test = pointer_test
self._buffer = database_buffer
self._pointer_base = pointer_base
if data_end is None:
# FileBuffer has no __len__, because len() fails for a size past
# sys.maxsize, as on 32-bit Python.
if isinstance(database_buffer, FileBuffer):

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isinstance check adds a runtime import from decoder to file, so the decoder must know about one buffer class. The Reader already has _buffer_size. It can pass data_end=self._buffer_size to the metadata Decoder, and the tests can pass file_buffer.size(). The default can then stay len(buffer), with no type check and no import.

🤖 Comment by Claude Opus 5.5.

data_end = database_buffer.size()
else:
data_end = len(database_buffer)
self._data_end = data_end

def _decode_array(
self,
Expand Down Expand Up @@ -124,6 +139,8 @@ def _decode_bytes(
raise InvalidDatabaseError(_TOO_LARGE)
budget.payload_left = remaining
new_offset = offset + size
if new_offset > self._data_end:
raise InvalidDatabaseError(_BAD_DATA)
return self._buffer[offset:new_offset], new_offset

def _decode_double(
Expand All @@ -134,6 +151,8 @@ def _decode_double(
) -> tuple[float, int]:
self._verify_size(size, 8)
new_offset = offset + size
if new_offset > self._data_end:
raise InvalidDatabaseError(_BAD_DATA)
packed_bytes = self._buffer[offset:new_offset]
(value,) = struct.unpack(b"!d", packed_bytes)
return value, new_offset
Expand All @@ -146,6 +165,8 @@ def _decode_float(
) -> tuple[float, int]:
self._verify_size(size, 4)
new_offset = offset + size
if new_offset > self._data_end:
raise InvalidDatabaseError(_BAD_DATA)
packed_bytes = self._buffer[offset:new_offset]
(value,) = struct.unpack(b"!f", packed_bytes)
return value, new_offset
Expand All @@ -161,6 +182,8 @@ def _decode_int32(
if size == 0:
return 0, offset
new_offset = offset + size
if new_offset > self._data_end:
raise InvalidDatabaseError(_BAD_DATA)
packed_bytes = self._buffer[offset:new_offset]

if size != 4:
Expand All @@ -185,17 +208,60 @@ def _decode_map(
budget.depth = depth
container: dict[str, Record] = {}
decode = self._decode
decode_key = self._decode_key
for _ in range(size):
(key, offset) = decode(offset, budget, False) # noqa: FBT003
(key, offset) = decode_key(offset, budget)
(value, offset) = decode(offset, budget, False) # noqa: FBT003
try:
container[key] = value # type: ignore[index]
except TypeError as ex:
# The key cannot be hashed, such as a list.
raise InvalidDatabaseError(_BAD_DATA) from ex
container[key] = value
budget.depth -= 1
return container, offset

def _decode_key(self, offset: int, budget: _DecodeBudget) -> tuple[str, int]: # noqa: C901

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_decode_key copies the pointer arithmetic and the target bound check from _decode_pointer. It also copies the size and budget checks from the string branch of _decode. A future fix to pointer or string bounds must then go in 3 places. If one place is missed, map keys and values behave differently.

A small shared helper that returns (target, end) for a pointer would remove the pointer copy. It costs one call per key, against the gain of about 8% that the commit message reports.

🤖 Comment by Claude Opus 5.5.

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was intentional.

# A key is a string or a pointer to one. Real databases share most keys
# through pointers, so decode both here without the generic dispatch.
# A string cannot nest, so a pointer key does not count toward the
# depth. This repeats the pointer arithmetic of _decode_pointer to save
# a call per key. pointer_test applies to pointer values only.
buffer = self._buffer
data_end = self._data_end
ctrl_byte = buffer[offset]
new_offset = offset + 1
is_pointer = ctrl_byte >> 5 == 1
if is_pointer:
size = ctrl_byte & 0x1F
pointer_size = (size >> 3) + 1
pointer_end = new_offset + pointer_size
if pointer_end > data_end:
raise InvalidDatabaseError(_BAD_DATA)
pointer = int.from_bytes(buffer[new_offset:pointer_end], "big")
if pointer_size < 4:
pointer |= (size & 0x7) << (pointer_size << 3)
pointer += _POINTER_VALUE_OFFSETS[pointer_size]
pointer += self._pointer_base
if pointer >= data_end:
raise InvalidDatabaseError(_BAD_DATA)
ctrl_byte = buffer[pointer]
new_offset = pointer + 1
elif offset >= data_end:
raise InvalidDatabaseError(_BAD_DATA)
if ctrl_byte >> 5 != 2:
raise InvalidDatabaseError(_NON_STRING_KEY)

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a map key that is a pointer to another pointer, this raises Invalid map key: the key is not a string. The C extension raises ... data section contains bad data .... Before this PR, the pure Python reader raised that message too.

Example data section: 41 61 | 20 00 | e1 20 02 a0. This is a map whose key points to a pointer to "a", with a record that points to the map. Reader.get() in MODE_MEMORY and extension.Reader.get() give different messages.

The decoder test case e12004a02000 expects the new message, so the tests do not show the difference. Should a pointer key whose target is a pointer raise _BAD_DATA, to match the C extension?

🤖 Comment by Claude Opus 5.5.

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem important.

size = ctrl_byte & 0x1F
if size >= 29:
(size, new_offset) = self._size_from_ctrl_byte(size, new_offset)
remaining = budget.payload_left - size
if remaining < 0:
raise InvalidDatabaseError(_TOO_LARGE)
budget.payload_left = remaining
end = new_offset + size
if end > data_end:
raise InvalidDatabaseError(_BAD_DATA)
key = buffer[new_offset:end].decode("utf-8")
if is_pointer:
# The map continues after the pointer, not after its target.
return key, pointer_end
return key, end

def _decode_pointer(
self,
size: int,
Expand All @@ -204,10 +270,9 @@ def _decode_pointer(
) -> tuple[Record, int]:
pointer_size = (size >> 3) + 1
new_offset = offset + pointer_size
pointer_bytes = self._buffer[offset:new_offset]
if len(pointer_bytes) != pointer_size:
if new_offset > self._data_end:

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes the len(pointer_bytes) != pointer_size guard. The bound check now relies on data_end <= len(buffer), but __init__ does not enforce that. If data_end is past the end of the buffer, the slice returns fewer bytes, and int.from_bytes gives a wrong pointer with no error.

Example: Decoder(b'\x28\x00', data_end=10).decode(0). The slice returns 1 byte instead of 2. Before this PR, this raised a bad data error. The same thing can happen if the caller passes a bytearray and later shrinks it.

_decode_key (line 236) has the same gap. A check in __init__ that data_end is not past the end of the buffer would fix the first case.

🤖 Comment by Claude Opus 5.5.

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure.

raise InvalidDatabaseError(_BAD_DATA)
pointer = int.from_bytes(pointer_bytes, "big")
pointer = int.from_bytes(self._buffer[offset:new_offset], "big")
if pointer_size < 4:
# The low three bits of the ctrl byte are the high bits of the
# pointer, and sizes 2 and 3 add a fixed offset.
Expand All @@ -218,6 +283,11 @@ def _decode_pointer(
if self._pointer_test:
return pointer, new_offset

# The payload checks do not cover a target with no payload, such as
# an empty map.
if pointer >= self._data_end:
raise InvalidDatabaseError(_BAD_DATA)

# The value at the pointer's position was charged by its containing
# array or map, so the target costs nothing more. Only the depth changes.
depth = budget.depth + 1
Expand All @@ -239,6 +309,8 @@ def _decode_uint(
if size > _MAX_UINT_BYTES:
raise InvalidDatabaseError(_BAD_DATA)
new_offset = offset + size
if new_offset > self._data_end:
raise InvalidDatabaseError(_BAD_DATA)
uint_bytes = self._buffer[offset:new_offset]
return int.from_bytes(uint_bytes, "big"), new_offset

Expand Down Expand Up @@ -297,6 +369,8 @@ def _decode( # noqa: C901, PLR0911, PLR0912
raise InvalidDatabaseError(_TOO_LARGE)
budget.payload_left = remaining
end = new_offset + size
if end > self._data_end:
raise InvalidDatabaseError(_BAD_DATA)
return self._buffer[new_offset:end].decode("utf-8"), end
case 1:
if pointer_target:
Expand Down
23 changes: 20 additions & 3 deletions maxminddb/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import os
from typing import TYPE_CHECKING, overload

from maxminddb.errors import InvalidDatabaseError

try:
from multiprocessing import Lock
except ImportError:
Expand All @@ -13,6 +15,9 @@
if TYPE_CHECKING:
from maxminddb.types import StrOrBytesPath

# A read inside the size at open came back short, so the file shrank.
_SHORT_READ = "The database file changed after the reader opened it."


class FileBuffer:
"""A slice-able file reader."""
Expand All @@ -32,16 +37,28 @@ def __getitem__(self, index: slice) -> bytes: ...

def __getitem__(self, index: slice | int) -> bytes | int:
"""Get item by index."""
# Past the size at open, a read acts as on bytes: a slice returns
# fewer bytes, and an index raises IndexError.
if isinstance(index, slice):
return self._read(index.stop - index.start, index.start)
size = index.stop - index.start
data = self._read(size, index.start)
if len(data) != size and index.stop <= self._size:
raise InvalidDatabaseError(_SHORT_READ)

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short-read check does not work on platforms without os.pread, such as the Windows CI job. On that path, _read (line 92) calls seek() and read() on a BufferedReader. A seek inside the cached 8 KiB buffer does not go to the OS.

Example: read fb[0] (this fills the buffer), truncate a 100-byte file to 10 bytes, then read fb[50] and fb[60:64]. Both reads return the old bytes, and no error occurs. If a process rewrites the file in place, reads can mix old cached bytes with new bytes.

HISTORY.rst says that a file that shrinks after open now raises InvalidDatabaseError. Opening the file with buffering=0 on this path makes the check work.

🤖 Comment by Claude Opus 5.5.

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure about this one.

return data
if isinstance(index, int):
return self._read(1, index)[0]
data = self._read(1, index)
if data:
return data[0]
if index < self._size:
raise InvalidDatabaseError(_SHORT_READ)
msg = "FileBuffer index out of range"
raise IndexError(msg)
msg = "Invalid argument type."
raise TypeError(msg)

def rfind(self, needle: bytes, start: int) -> int:
"""Reverse find needle from start."""
pos = self._read(self._size - start - 1, start).rfind(needle)
pos = self._read(self._size - start, start).rfind(needle)
if pos == -1:
return pos
return start + pos
Expand Down
45 changes: 30 additions & 15 deletions maxminddb/reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ class Reader:

_buffer: bytes | bytearray | FileBuffer | "mmap.mmap" # noqa: UP037
_buffer_size: int
# The offset of the metadata marker, where the data section ends.
_data_end: int
# No database is open until __init__ succeeds.
closed: bool = True
_decoder: Decoder
Expand Down Expand Up @@ -137,6 +139,7 @@ def _load(
msg,
)

self._data_end = metadata_start
metadata_start += len(self._METADATA_START_MARKER)
metadata_decoder = Decoder(self._buffer, metadata_start)
# For a repeated key, the decoder keeps the last value, but
Expand Down Expand Up @@ -167,14 +170,13 @@ def _load(

# Traversal reads nodes below node_count. Once the tree fits, those
# reads need no length checks of their own.
if self._data_start > self._buffer_size:
msg = (
f"Error opening database file ({filename}). The search tree "
"extends past the end of the file."
)
raise InvalidDatabaseError(msg) # noqa: TRY301
_check_search_tree_fits(
self._data_start, self._data_end, self._buffer_size, filename
)

self._decoder = Decoder(self._buffer, self._data_start)
self._decoder = Decoder(
self._buffer, self._data_start, data_end=self._data_end
)
self.closed = False

ipv4_start = 0
Expand Down Expand Up @@ -362,9 +364,9 @@ def _read_node(self, node_number: int, index: int) -> int:
def _resolve_data_pointer(self, pointer: int) -> Record:
resolved = pointer + self._pointer_base

# A pointer into the separator between the tree and the data section
# is as corrupt as one past the end, as libmaxminddb checks.
if resolved < self._data_start or resolved >= self._buffer_size:
# A record must point into the data section. libmaxminddb also rejects
# a pointer into the separator between the tree and the data section.
if resolved < self._data_start or resolved >= self._data_end:
raise InvalidDatabaseError(_CORRUPT_TREE)

(data, _) = self._decoder.decode(resolved)
Expand Down Expand Up @@ -501,10 +503,6 @@ def _metadata_fields(metadata: RecordDict, filename: object) -> dict[str, Any]:
A new minor version of the format can add keys. This ignores them.
"""
prefix = f"Error reading metadata in database file ({filename})."
# The C extension also rejects a key that is not a string.
if not all(type(k) is str for k in metadata):
msg = f"{prefix} A metadata key is not a string."
raise InvalidDatabaseError(msg)
fields: dict[str, Any] = {}
for key, value_type in _METADATA_TYPES.items():
value = metadata.get(key)
Expand All @@ -513,7 +511,7 @@ def _metadata_fields(metadata: RecordDict, filename: object) -> dict[str, Any]:
if valid and isinstance(value, list):
valid = all(type(v) is str for v in value)
elif valid and isinstance(value, dict):
valid = all(type(k) is str and type(v) is str for k, v in value.items())
valid = all(type(v) is str for v in value.values())
if not valid:
msg = f"{prefix} The {key} value is missing or has the wrong type."
raise InvalidDatabaseError(msg)
Expand Down Expand Up @@ -549,6 +547,23 @@ def _check_metadata_ranges(fields: dict[str, Any], prefix: str) -> None:
raise InvalidDatabaseError(msg)


def _check_search_tree_fits(
data_start: int,
data_end: int,
file_size: int,
filename: object,
) -> None:
"""Raise InvalidDatabaseError unless the search tree ends before the metadata."""
if data_start <= data_end:

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now rejects an empty search tree (node_count 0) when the separator before the metadata marker is shorter than 16 bytes. On main, such a file opens in MODE_MEMORY and MODE_FILE, and get() returns None. On this branch, it raises The search tree extends into the metadata section.

The message is misleading for this case. The tree is empty, and the separator is the part that overlaps. The docstring does not mention the separator, and HISTORY.rst does not mention this change. _check_metadata_ranges says that this reader accepts an empty search tree, so this change might not be intended.

🤖 Comment by Claude Opus 5.5.

@horgh horgh Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems not important

return
if data_start > file_size:
problem = "extends past the end of the file"
else:
problem = "extends into the metadata section"
msg = f"Error opening database file ({filename}). The search tree {problem}."
raise InvalidDatabaseError(msg)


@dataclass(kw_only=True, frozen=True)
class Metadata:
"""Metadata for the MaxMind DB reader."""
Expand Down
Loading
Loading