From 3273ffcaaed7c633c2c84275eae2c16f6f685fd1 Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Mon, 28 Sep 2026 11:01:08 -0400 Subject: [PATCH 1/4] Stop the C accelerator leaking a bytes object per column _PyUnicode_AsUTF8 took a new reference to the encoded bytes and never dropped it, so every query leaked one block per column plus one for the encoding errors string -- around 50 MB per 3000 wide queries, which is enough to OOM a long-running service. Fixes #135. The per-column encodings were leaking their C copies too: State_clear_fields freed the array and not the entries in it. The struct sequence field names cannot be freed the same way. The type stores the name pointers rather than copying the strings, and reads them again when a row is repr'd, so they have to outlive every row rather than the State that built them -- freeing them here would trade the leak for a use-after-free. They now belong to a capsule in the type's own dict, which frees them when the last reference to the type goes away. Co-Authored-By: Claude Opus 5 --- accel.c | 77 ++++++++++++++++-- singlestoredb/tests/test_accel_leaks.py | 101 ++++++++++++++++++++++++ 2 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 singlestoredb/tests/test_accel_leaks.py diff --git a/accel.c b/accel.c index db6341c8b..52971fb1c 100644 --- a/accel.c +++ b/accel.c @@ -369,18 +369,28 @@ inline int IMIN(int a, int b) { return((a) < (b) ? a : b); } static PyObject *create_numpy_array(PyObject *py_memview, char *data_format, int data_type, PyObject *py_objs); +// Returns a newly allocated UTF-8 copy of `unicode`; the caller owns it +// and must free() it. char *_PyUnicode_AsUTF8(PyObject *unicode) { PyObject *bytes = PyUnicode_AsEncodedString(unicode, "utf-8", "strict"); if (!bytes) return NULL; char *str = NULL; Py_ssize_t str_l = 0; - if (PyBytes_AsStringAndSize(bytes, &str, &str_l) < 0) { - return NULL; - } + char *out = NULL; + + if (PyBytes_AsStringAndSize(bytes, &str, &str_l) < 0) goto exit; - char *out = calloc(str_l + 1, 1); + out = calloc(str_l + 1, 1); + if (!out) { + PyErr_NoMemory(); + goto exit; + } memcpy(out, str, str_l); + +exit: + Py_DECREF(bytes); + return out; } @@ -924,14 +934,45 @@ int ensure_bson() { } +// Name of the capsule, stored in the struct sequence type's dict, that owns +// the type's field name storage. +#define STRUCTSEQUENCE_FIELDS_CAPSULE "singlestoredb.Row.fields" + + +// Frees a NULL name terminated array of struct sequence fields and the +// names in it. +static void free_structsequence_fields(PyStructSequence_Field *fields) { + if (!fields) return; + for (PyStructSequence_Field *field = fields; field->name; field++) { + free((void*)field->name); + } + free(fields); +} + + +static void structsequence_fields_capsule_destructor(PyObject *py_capsule) { + PyStructSequence_Field *fields = (PyStructSequence_Field*) + PyCapsule_GetPointer(py_capsule, STRUCTSEQUENCE_FIELDS_CAPSULE); + if (!fields) { + PyErr_Clear(); + return; + } + free_structsequence_fields(fields); +} + + static void State_clear_fields(StateObject *self) { if (!self) return; DESTROY(self->offsets); DESTROY(self->scales); DESTROY(self->flags); DESTROY(self->type_codes); - DESTROY(self->encodings); - DESTROY(self->structsequence_desc.fields); + if (self->encodings) { + for (unsigned long i = 0; i < self->n_cols; i++) { + DESTROY(self->encodings[i]); + } + DESTROY(self->encodings); + } DESTROY(self->encoding_errors); if (self->py_converters) { for (unsigned long i = 0; i < self->n_cols; i++) { @@ -958,6 +999,11 @@ static void State_clear_fields(StateObject *self) { DESTROY(self->py_invalid_values); } Py_CLEAR(self->structsequence); + // Only reached if the type was never built, or building it failed before + // the capsule took ownership. Once the capsule holds the fields, this is + // NULL and the names outlive us along with the type. + free_structsequence_fields(self->structsequence_desc.fields); + self->structsequence_desc.fields = NULL; Py_CLEAR(self->py_namedtuple); Py_CLEAR(self->py_namedtuple_args); Py_CLEAR(self->py_names_list); @@ -1201,10 +1247,29 @@ static int State_init(StateObject *self, PyObject *args, PyObject *kwds) { if (!self->structsequence_desc.fields) goto error; for (unsigned long i = 0; i < self->n_cols; i++) { self->structsequence_desc.fields[i].name = _PyUnicode_AsUTF8(self->py_names[i]); + if (!self->structsequence_desc.fields[i].name) goto error; self->structsequence_desc.fields[i].doc = NULL; } self->structsequence = PyStructSequence_NewType(&self->structsequence_desc); if (!self->structsequence) goto error; + + // The type stores the field name pointers rather than copying the + // strings, and reads them again when a row is repr'd. Rows can + // outlive this State, so the storage is handed to a capsule in the + // type's dict, which frees it when the type itself goes away. + PyObject *py_fields_capsule = PyCapsule_New( + self->structsequence_desc.fields, + STRUCTSEQUENCE_FIELDS_CAPSULE, + &structsequence_fields_capsule_destructor + ); + if (!py_fields_capsule) goto error; + self->structsequence_desc.fields = NULL; + + rc = PyObject_SetAttrString((PyObject*)self->structsequence, + "__singlestoredb_fields__", + py_fields_capsule); + Py_DECREF(py_fields_capsule); + if (rc != 0) goto error; } // Fall through diff --git a/singlestoredb/tests/test_accel_leaks.py b/singlestoredb/tests/test_accel_leaks.py new file mode 100644 index 000000000..4e4a91d06 --- /dev/null +++ b/singlestoredb/tests/test_accel_leaks.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python +# type: ignore +"""Test that the C accelerator does not leak memory per query.""" +import gc +import os +import sys +import unittest + +import singlestoredb as s2 +from singlestoredb.mysql import connection as mysql_connection + +# The leak in issue #135 was one allocation per column per query, so a wide +# result makes it unmistakable: it shows up as ~N_COLS blocks per query, three +# orders of magnitude above the noise floor of a few blocks over the run. +N_COLS = 100 +WIDE_QUERY = 'SELECT ' + ', '.join(f'{i} AS c{i}' for i in range(N_COLS)) + +WARMUP = 50 +ITERATIONS = 200 + +# Per-query budget, in allocated blocks. Zero is what a fixed accelerator +# actually measures; this leaves room for caches that fill on the first few +# queries while staying far below the N_COLS a per-column leak would cost. +MAX_BLOCKS_PER_QUERY = 5.0 + +has_accel = mysql_connection._singlestoredb_accel is not None +pure_python = bool(int(os.environ.get('SINGLESTOREDB_PURE_PYTHON', '0'))) + + +@unittest.skipIf(not has_accel, 'C extension is not available') +@unittest.skipIf(pure_python, 'C extension is disabled') +class TestAccelLeaks(unittest.TestCase): + + def setUp(self): + self.conn = s2.connect() + if 'http' in self.conn.driver: + self.skipTest('HTTP interface does not use the C extension') + + def tearDown(self): + try: + self.conn.close() + except Exception: + pass + + def blocks_per_query(self, results_type): + """Return the allocated blocks retained per query of WIDE_QUERY.""" + with s2.connect(results_type=results_type, pure_python=False) as conn: + with conn.cursor() as cur: + for _ in range(WARMUP): + cur.execute(WIDE_QUERY) + cur.fetchall() + + gc.collect() + before = sys.getallocatedblocks() + + for _ in range(ITERATIONS): + cur.execute(WIDE_QUERY) + cur.fetchall() + + gc.collect() + after = sys.getallocatedblocks() + + return (after - before) / ITERATIONS + + def test_no_leak_per_query(self): + for results_type in ('tuples', 'dicts', 'namedtuples', 'structsequences'): + with self.subTest(results_type=results_type): + leaked = self.blocks_per_query(results_type) + assert leaked < MAX_BLOCKS_PER_QUERY, \ + f'{results_type} leaks {leaked} blocks per query' + + def test_rows_outlive_the_result_state(self): + """Struct sequence rows must survive the state that created them. + + The type does not copy its field names, it stores the pointers and + reads them again in repr, so the names have to outlive every row + rather than the query that built them. + """ + with s2.connect( + results_type='structsequences', pure_python=False, + ) as conn: + with conn.cursor() as cur: + cur.execute(WIDE_QUERY) + rows = cur.fetchall() + + # Discard the state that built the rows, several times over. + for _ in range(10): + cur.execute('SELECT 1') + cur.fetchall() + + gc.collect() + + assert len(rows[0]) == N_COLS, len(rows[0]) + assert rows[0].c0 == 0, rows[0].c0 + assert getattr(rows[0], f'c{N_COLS - 1}') == N_COLS - 1 + assert f'c{N_COLS - 1}=' in repr(rows[0]), repr(rows[0]) + + +if __name__ == '__main__': + import nose2 + nose2.main() From edee40e7027642eae2c199bada28d9ce58fc2375 Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Tue, 29 Sep 2026 08:55:15 -0400 Subject: [PATCH 2/4] Tie struct sequence field names to the type, not a type attribute Address review feedback on #136. The capsule owning a struct sequence type's field name storage was stored as __singlestoredb_fields__ on the type, where user code could delete or replace it. That decrefs the capsule and frees the names while the type and its existing rows still point at them, so a later repr(row) reads freed memory -- confirmed as a UnicodeDecodeError off garbage bytes. Ownership now lives in a module-private dict inside the extension, keyed by a weak reference to the type. The weakref callback drops the entry, and with it the capsule, when the type is collected, so there is one free path and no Python-reachable way to trigger it early. Rows are instances of a heap type and hold a reference to it, so the type still outlives every row. Also read the already-parsed pure_python option in the tests instead of parsing SINGLESTOREDB_PURE_PYTHON with int(), which raised at collection time for valid values such as `true`. Co-Authored-By: Claude Opus 5 --- accel.c | 64 ++++++++++++++++++++++--- singlestoredb/tests/test_accel_leaks.py | 46 +++++++++++++++++- 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/accel.c b/accel.c index 52971fb1c..a5372bb02 100644 --- a/accel.c +++ b/accel.c @@ -578,6 +578,8 @@ typedef struct { PyObject *create_numpy_array_kwargs_vector[8]; PyObject *struct_unpack_args; PyObject *bson_decode_args; + PyObject *structsequence_fields_registry; + PyObject *structsequence_fields_release; } PyObjects; static PyObjects PyObj = {0}; @@ -934,8 +936,9 @@ int ensure_bson() { } -// Name of the capsule, stored in the struct sequence type's dict, that owns -// the type's field name storage. +// Name of the capsule that owns a struct sequence type's field name storage. +// The capsule is held by PyObj.structsequence_fields_registry, keyed by a weak +// reference to the type, and is dropped when the type is collected. #define STRUCTSEQUENCE_FIELDS_CAPSULE "singlestoredb.Row.fields" @@ -961,6 +964,31 @@ static void structsequence_fields_capsule_destructor(PyObject *py_capsule) { } +// Weak reference callback for a struct sequence type. Dropping the registry +// entry drops the last reference to the capsule, whose destructor frees the +// names. The weak reference itself is the key: by the time this runs the +// referent is already gone, so the type cannot be used to find the entry. +static PyObject *structsequence_fields_release(PyObject *self, PyObject *py_weakref) { + (void)self; + if (PyObj.structsequence_fields_registry) { + if (PyDict_DelItem(PyObj.structsequence_fields_registry, py_weakref)) { + // A callback must not raise. + PyErr_Clear(); + } + } + Py_INCREF(Py_None); + return Py_None; +} + + +static PyMethodDef structsequence_fields_release_def = { + "structsequence_fields_release", + (PyCFunction)structsequence_fields_release, + METH_O, + NULL +}; + + static void State_clear_fields(StateObject *self) { if (!self) return; DESTROY(self->offsets); @@ -1255,8 +1283,12 @@ static int State_init(StateObject *self, PyObject *args, PyObject *kwds) { // The type stores the field name pointers rather than copying the // strings, and reads them again when a row is repr'd. Rows can - // outlive this State, so the storage is handed to a capsule in the - // type's dict, which frees it when the type itself goes away. + // outlive this State, so the storage is handed to a capsule owned + // by a module-private registry, keyed by a weak reference to the + // type. The names are freed when the type is collected. Rows are + // instances of a heap type and so keep it alive; nothing on the + // type refers to the capsule, so Python code cannot release it + // early. PyObject *py_fields_capsule = PyCapsule_New( self->structsequence_desc.fields, STRUCTSEQUENCE_FIELDS_CAPSULE, @@ -1265,9 +1297,18 @@ static int State_init(StateObject *self, PyObject *args, PyObject *kwds) { if (!py_fields_capsule) goto error; self->structsequence_desc.fields = NULL; - rc = PyObject_SetAttrString((PyObject*)self->structsequence, - "__singlestoredb_fields__", - py_fields_capsule); + PyObject *py_fields_weakref = PyWeakref_NewRef( + (PyObject*)self->structsequence, + PyObj.structsequence_fields_release + ); + if (!py_fields_weakref) { + Py_DECREF(py_fields_capsule); + goto error; + } + + rc = PyDict_SetItem(PyObj.structsequence_fields_registry, + py_fields_weakref, py_fields_capsule); + Py_DECREF(py_fields_weakref); Py_DECREF(py_fields_capsule); if (rc != 0) goto error; } @@ -6140,6 +6181,15 @@ PyMODINIT_FUNC PyInit__singlestoredb_accel(void) { PyObj.bson_decode_args = PyTuple_New(1); if (!PyObj.bson_decode_args) goto error; + // Owns the field name storage of every live struct sequence type: + // weak reference to the type => capsule holding its names. + PyObj.structsequence_fields_registry = PyDict_New(); + if (!PyObj.structsequence_fields_registry) goto error; + + PyObj.structsequence_fields_release = PyCFunction_NewEx( + &structsequence_fields_release_def, NULL, NULL); + if (!PyObj.structsequence_fields_release) goto error; + return PyModule_Create(&_singlestoredb_accelmodule); error: diff --git a/singlestoredb/tests/test_accel_leaks.py b/singlestoredb/tests/test_accel_leaks.py index 4e4a91d06..c6c45c94e 100644 --- a/singlestoredb/tests/test_accel_leaks.py +++ b/singlestoredb/tests/test_accel_leaks.py @@ -2,7 +2,6 @@ # type: ignore """Test that the C accelerator does not leak memory per query.""" import gc -import os import sys import unittest @@ -24,7 +23,9 @@ MAX_BLOCKS_PER_QUERY = 5.0 has_accel = mysql_connection._singlestoredb_accel is not None -pure_python = bool(int(os.environ.get('SINGLESTOREDB_PURE_PYTHON', '0'))) +# Read the parsed option rather than the environment variable: the option's +# validator already accepts true/yes/on, which int() would choke on. +pure_python = bool(s2.get_option('pure_python')) @unittest.skipIf(not has_accel, 'C extension is not available') @@ -95,6 +96,47 @@ def test_rows_outlive_the_result_state(self): assert getattr(rows[0], f'c{N_COLS - 1}') == N_COLS - 1 assert f'c{N_COLS - 1}=' in repr(rows[0]), repr(rows[0]) + def test_field_names_survive_stripping_the_type_dict(self): + """No class attribute may own the field name storage. + + The names are read again by repr, so a deletable attribute holding + the only reference would turn `delattr` into a use-after-free. + """ + with s2.connect( + results_type='structsequences', pure_python=False, + ) as conn: + with conn.cursor() as cur: + cur.execute(WIDE_QUERY) + rows = cur.fetchall() + + row_type = type(rows[0]) + + # Nothing in the type's dict may be the owner: every entry there is + # reachable, and most of them are deletable. + for name, value in vars(row_type).items(): + assert type(value).__name__ != 'PyCapsule', name + + # Held so the field names are still read out of the type after the + # loop below deletes the type's own __repr__ entry. + row_repr = row_type.__repr__ + + # CPython reads these three back out of the dict itself, so deleting + # them breaks a struct sequence whatever owns its names. + keep = ('n_fields', 'n_sequence_fields', 'n_unnamed_fields') + + for name in list(vars(row_type)): + if name in keep: + continue + try: + delattr(row_type, name) + except (AttributeError, TypeError): + pass + + gc.collect() + + # repr reads the names out of the C field table, not the type dict. + assert f'c{N_COLS - 1}=' in row_repr(rows[0]), row_repr(rows[0]) + if __name__ == '__main__': import nose2 From 8f5cee2a309d40e6e8bf39f8e0f87c3ac200258e Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Tue, 29 Sep 2026 09:35:29 -0400 Subject: [PATCH 3/4] Measure the accelerator leak by result width, not by absolute blocks The namedtuples subtest of test_no_leak_per_query failed in CI while passing locally, reporting a steady 15.005 retained blocks per query. That is not our allocation. coverage.py's sys.monitoring backend keeps every code object it ever sees alive forever, deliberately, keyed by id() -- see code_objects in coverage/sysmon.py. collections.namedtuple compiles a fresh __new__ on every call and the accelerator builds one Row class per query, so a --cov run, which is what code-check.yml does, retains a code object per query with nothing wrong. pandas' DataFrame.itertuples builds its class the same way with no cache, so this is the tracer's accounting rather than a C API artifact or ours to fix. The leak in issue #135 cost one allocation per column per query, and the accelerator now measures flat at 15.005 blocks per query for both a 10-column and a 100-column result -- a constant that does not move when the width grows tenfold was never that bug. So assert the property the bug actually had: difference a narrow and a wide query, which cancels every per-query cost that is flat in the column count. The tracer overhead is flat, measured unchanged from 5 to 200 columns, so it cancels exactly rather than approximately. Keep a separate absolute per-query check, since differencing two widths cannot see something leaked once per query, and fund the namedtuples budget with an overhead figure measured at runtime so it stays tight when nothing is tracing. Verified both ways: the differential assertion still reports 1.0 blocks per column for tuples, dicts and namedtuples and 2.0 for structsequences when built against accel.c as of 4e348a84, twenty times the threshold, and the whole file passes on 3.11 and on 3.14.7 with and without --cov. Co-Authored-By: Claude Opus 5 --- singlestoredb/tests/test_accel_leaks.py | 112 +++++++++++++++++++++--- 1 file changed, 98 insertions(+), 14 deletions(-) diff --git a/singlestoredb/tests/test_accel_leaks.py b/singlestoredb/tests/test_accel_leaks.py index c6c45c94e..dc48b522a 100644 --- a/singlestoredb/tests/test_accel_leaks.py +++ b/singlestoredb/tests/test_accel_leaks.py @@ -8,20 +8,68 @@ import singlestoredb as s2 from singlestoredb.mysql import connection as mysql_connection -# The leak in issue #135 was one allocation per column per query, so a wide -# result makes it unmistakable: it shows up as ~N_COLS blocks per query, three -# orders of magnitude above the noise floor of a few blocks over the run. -N_COLS = 100 -WIDE_QUERY = 'SELECT ' + ', '.join(f'{i} AS c{i}' for i in range(N_COLS)) +# The leak in issue #135 was one allocation per column per query, so the +# signal to look for is retention that grows with the width of the result. +# Measuring two widths and taking the difference is what makes this robust: +# anything a query costs that is flat in the column count drops out, and one +# such cost is unavoidable here. Under coverage.py's sys.monitoring backend +# every code object ever seen is retained forever, deliberately, keyed by +# id() (see `code_objects` in coverage/sysmon.py). collections.namedtuple +# compiles a fresh __new__ on each call and the accelerator builds one Row +# class per query, so a coverage run retains ~15 blocks per query on the +# namedtuples path however narrow the result is. That is the tracer's +# accounting, not our allocation, and CI runs under --cov. +NARROW_COLS = 10 +WIDE_COLS = 100 WARMUP = 50 ITERATIONS = 200 -# Per-query budget, in allocated blocks. Zero is what a fixed accelerator -# actually measures; this leaves room for caches that fill on the first few -# queries while staying far below the N_COLS a per-column leak would cost. + +def query_for(n_cols): + return 'SELECT ' + ', '.join(f'{i} AS c{i}' for i in range(n_cols)) + + +# Per-column budget, in allocated blocks. A fixed accelerator measures zero; +# the leak this guards against cost one block per column, so anything above +# the noise floor of a fraction of a block is the bug coming back. +MAX_BLOCKS_PER_COLUMN = 0.05 + +# Per-query budget for the width-independent part, in allocated blocks. Room +# for caches that fill on the first few queries, plus the tracer overhead +# above, which is measured rather than assumed so the budget stays tight when +# nothing is tracing. MAX_BLOCKS_PER_QUERY = 5.0 +N_COLS = WIDE_COLS +WIDE_QUERY = query_for(WIDE_COLS) + + +def blocks_retained_per_namedtuple(): + """Return the blocks a tracer retains per collections.namedtuple() call. + + Zero when nothing is tracing. Non-zero under coverage, which the + accelerator then pays once per query on the namedtuples path. + """ + import collections + + fields = [f'c{i}' for i in range(WIDE_COLS)] + + def build(n): + for _ in range(n): + collections.namedtuple('Row', fields, rename=True) + + build(WARMUP) + gc.collect() + before = sys.getallocatedblocks() + + build(ITERATIONS) + gc.collect() + after = sys.getallocatedblocks() + + return max(0.0, (after - before) / ITERATIONS) + + has_accel = mysql_connection._singlestoredb_accel is not None # Read the parsed option rather than the environment variable: the option's # validator already accepts true/yes/on, which int() would choke on. @@ -43,19 +91,20 @@ def tearDown(self): except Exception: pass - def blocks_per_query(self, results_type): - """Return the allocated blocks retained per query of WIDE_QUERY.""" + def blocks_per_query(self, results_type, n_cols=WIDE_COLS): + """Return the allocated blocks retained per query of n_cols columns.""" + query = query_for(n_cols) with s2.connect(results_type=results_type, pure_python=False) as conn: with conn.cursor() as cur: for _ in range(WARMUP): - cur.execute(WIDE_QUERY) + cur.execute(query) cur.fetchall() gc.collect() before = sys.getallocatedblocks() for _ in range(ITERATIONS): - cur.execute(WIDE_QUERY) + cur.execute(query) cur.fetchall() gc.collect() @@ -63,12 +112,47 @@ def blocks_per_query(self, results_type): return (after - before) / ITERATIONS + def test_no_leak_per_column(self): + """Retention must not grow with the width of the result. + + This is the shape of the issue #135 leak, and differencing two widths + cancels every per-query cost that is flat in the column count -- see + the note on the tracer overhead at the top of this module. + """ + for results_type in ('tuples', 'dicts', 'namedtuples', 'structsequences'): + with self.subTest(results_type=results_type): + narrow = self.blocks_per_query(results_type, NARROW_COLS) + wide = self.blocks_per_query(results_type, WIDE_COLS) + + per_column = (wide - narrow) / (WIDE_COLS - NARROW_COLS) + + assert per_column < MAX_BLOCKS_PER_COLUMN, \ + f'{results_type} leaks {per_column} blocks per column ' \ + f'({narrow} blocks/query at {NARROW_COLS} columns, ' \ + f'{wide} at {WIDE_COLS})' + def test_no_leak_per_query(self): + """Retention must not grow per query either. + + The per-column check above cannot see a leak of something allocated + once per query, so budget that separately. The namedtuples path is + allowed the tracer's per-class overhead on top, measured here so the + budget stays tight when nothing is tracing. + """ + tracer_overhead = blocks_retained_per_namedtuple() + for results_type in ('tuples', 'dicts', 'namedtuples', 'structsequences'): with self.subTest(results_type=results_type): + budget = MAX_BLOCKS_PER_QUERY + if results_type == 'namedtuples': + budget += tracer_overhead + leaked = self.blocks_per_query(results_type) - assert leaked < MAX_BLOCKS_PER_QUERY, \ - f'{results_type} leaks {leaked} blocks per query' + + assert leaked < budget, \ + f'{results_type} leaks {leaked} blocks per query ' \ + f'(budget {budget}, of which {tracer_overhead} is ' \ + f'tracer overhead)' def test_rows_outlive_the_result_state(self): """Struct sequence rows must survive the state that created them. From 2a1b3b6e5b75e69e524966f11c54accf6a069d3b Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Tue, 29 Sep 2026 12:47:17 -0400 Subject: [PATCH 4/4] Check the per-column encoding copy for allocation failure _PyUnicode_AsUTF8 returns a calloc'd copy now, so it can fail. Three of its four call sites check for that; the per-column one in State_init did not, and NULL is the binary-column sentinel every reader of encodings[] goes by (accel.c:1944, 1974, 2159). A failed allocation for a text column would therefore have decoded that column as binary, with the PyErr_NoMemory only surfacing when the function exited. Split the ternary into an explicit branch so the failure is distinguishable from the sentinel and reaches the error label. The !py_encoding arm it replaces was dead -- the PyTuple_GetItem above already bails on NULL. Co-Authored-By: Claude Opus 5 --- accel.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/accel.c b/accel.c index a5372bb02..075276287 100644 --- a/accel.c +++ b/accel.c @@ -1200,8 +1200,14 @@ static int State_init(StateObject *self, PyObject *args, PyObject *kwds) { self->py_encodings[i] = (py_encoding == Py_None) ? NULL : py_encoding; Py_XINCREF(self->py_encodings[i]); - self->encodings[i] = (!py_encoding || py_encoding == Py_None) ? - NULL : _PyUnicode_AsUTF8(py_encoding); + // NULL is the binary-column sentinel, so an allocation failure here + // can not be left in place; it has to go to the error path. + if (py_encoding == Py_None) { + self->encodings[i] = NULL; + } else { + self->encodings[i] = _PyUnicode_AsUTF8(py_encoding); + if (!self->encodings[i]) goto error; + } self->py_invalid_values[i] = (!py_invalid_value || py_invalid_value == Py_None) ? NULL : py_converter;