gh-157227: `_remote_debugging`: Validate lists and tuples in `write_sample` by maurycy · Pull Request #157228 · python/cpython

maurycy · 2026-09-09T18:15:51Z

See #157227 for more details

Basically, BinaryWriter.write_sample() only checks whether stack_frames is a list, and does PyList_GET_ITEM() / PyStructSequence_GET_ITEM() all the way. The PR adds a check for these containers.

Now it's graceful:

maurycy@gimel cpython (fix-remote-debugging-writer-validation aec95c2*) % ./python.exe 
Python 3.16.0a0 (heads/fix-remote-debugging-writer-validation-dirty:aec95c21726, Sep  9 2026, 20:) [Clang 21.0.0 (clang-2100.1.1.101)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import _remote_debugging
>>> _remote_debugging.BinaryWriter("/tmp/o.bin", 1000, 1000000).write_sample([42], 2000)
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    _remote_debugging.BinaryWriter("/tmp/o.bin", 1000, 1000000).write_sample([42], 2000)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
TypeError: interp_info must be a tuple of at least 2 items
>>>

instead of segmentation fault.

Issue: _remote_debugging: BinaryWriter.write_sample() trivial segfault #157227

maurycy · 2026-09-09T18:20:59Z

        } \
    } while (0)

+#define CHECK_TUPLE_ITEMS(obj, n) do { \


Truth be told, we reinvent similar check over and over:

cpython/Objects/memoryobject.c

Lines 217 to 222 in 9a75080

#define CHECK_LIST_OR_TUPLE(v) \

if (!PyList_Check(v) && !PyTuple_Check(v)) { \

PyErr_SetString(PyExc_TypeError, \

#v " must be a list or a tuple"); \

return NULL; \

}

cpython/Modules/_testbuffer.c

Lines 26 to 33 in 9a75080

#define CHECK_LIST_OR_TUPLE(v) \

do { \

if (!PyList_Check(v) && !PyTuple_Check(v)) { \

PyErr_SetString(PyExc_TypeError, \

#v " must be a list or a tuple"); \

return NULL; \

} \

} while (0)

Sometimes without a macro:

cpython/Modules/_io/stringio.c

Lines 922 to 930 in 9a75080

/* We allow the state tuple to be longer than 4, because we may need

someday to extend the object's state without breaking

backward-compatibility. */

if (!PyTuple_Check(state) || PyTuple_GET_SIZE(state) < 4) {

PyErr_Format(PyExc_TypeError,

"%.200s.__setstate__ argument should be 4-tuple, got %.200s",

Py_TYPE(self)->tp_name, Py_TYPE(state)->tp_name);

return NULL;

}

cpython/Python/_warnings.c

Lines 450 to 465 in 9a75080

tmp_item = PyList_GET_ITEM(filters, i);

if (!PyTuple_Check(tmp_item) || PyTuple_GET_SIZE(tmp_item) != 5) {

PyErr_Format(PyExc_ValueError,

"warnings.%s item %zd isn't a 5-tuple", list_name, i);

result = false;

break;

}

/* Python code: action, msg, cat, mod, ln = item */

Py_INCREF(tmp_item);

action = PyTuple_GET_ITEM(tmp_item, 0);

msg = PyTuple_GET_ITEM(tmp_item, 1);

cat = PyTuple_GET_ITEM(tmp_item, 2);

mod = PyTuple_GET_ITEM(tmp_item, 3);

ln_obj = PyTuple_GET_ITEM(tmp_item, 4);

pablogsal · 2026-09-11T16:26:21Z

    Py_ssize_t num_interpreters = PyList_GET_SIZE(stack_frames);
    for (Py_ssize_t i = 0; i < num_interpreters; i++) {
        PyObject *interp_info = PyList_GET_ITEM(stack_frames, i);
+        CHECK_TUPLE_ITEMS(interp_info, 2);


We validate as we go, so a bad thread in the middle leaves the earlier threads already written and total_samples bumped even though write_sample raised. Should we walk and validate the whole structure first so this is all-or-nothing? Otherwise the caller has no way to recover from the TypeError.

pablogsal · 2026-09-11T16:26:21Z


        PyObject *interp_id_obj = PyStructSequence_GET_ITEM(interp_info, 0);
        PyObject *threads = PyStructSequence_GET_ITEM(interp_info, 1);
+        CHECK_LIST(threads);


This narrows the hole but does not close it. PyLong_AsLong(status_obj) goes through _PyNumber_Index, so an __index__ that mutates the same list re-enters us while we still hold a cached size and a borrowed thread_info:

threads = [] class Evil: def __index__(self): del threads[:] return 0 threads.extend([(1, Evil(), []), (2, 0, [])]) _remote_debugging.BinaryWriter("/tmp/o.bin", 1000, 0, compression=0).write_sample([(0, threads)], 2000)

This still segfaults on PyList_GET_ITEM(threads, 1) because del threads[:] frees ob_item. writer_intern_string has the same problem via PyObject_Hash. If we want to claim write_sample is safe against arbitrary input we need strong references to the containers (or to re-read the sizes), not only a type check up front. Happy to take this as a follow-up, but then let's not close the issue with this PR.

pablogsal · 2026-09-11T16:26:21Z

        int32_t end_column = LOCATION_NOT_AVAILABLE;

        if (location != Py_None) {
+            CHECK_TUPLE_ITEMS(location, 4);


normalize_location() in Lib/profiling/sampling/collector.py accepts an int location (a bare lineno) and binary_collector.py hands stack_frames straight to write_sample, so this now raises where the other collectors work. Do we want a PyLong_Check(location) case here too, or is that branch in normalize_location() dead and we should drop it instead?

@pablogsal I still need some time, but this is a dead path, excercised only by tests. To put aside this PR, do you want me to open a separate PR that removes these?

Yes, please, a separate PR for the integer-location branches and their tests makes sense. Let's keep this one focused on container validation. Thanks!

pablogsal · 2026-10-12T16:06:04Z

        } \
    } while (0)

+#define CHECK_TUPLE_ITEMS(obj, n) do { \


Please, add a NEWS entry for this crash fix.

pablogsal · 2026-10-12T16:06:04Z


        PyObject *interp_id_obj = PyStructSequence_GET_ITEM(interp_info, 0);
        PyObject *threads = PyStructSequence_GET_ITEM(interp_info, 1);
+        CHECK_LIST(threads);


The __index__ repro above is still open: we iterate threads with a cached size and a borrowed thread_info while PyLong_AsLong(status_obj) can run arbitrary code. No rush, but can you tell me if you want to fix it here or in a separate issue? In the second case let's not close gh-157227 with this PR.

pablogsal · 2026-10-12T16:06:04Z

+                    filename, 1000, 0, compression=0
+                ) as writer:
+                    with self.assertRaisesRegex(TypeError, field):
+                        writer.write_sample(sample, 2000)


Nit, feel free to ignore: if breaking the writer is intended, can we assert that finalize() raises ValueError after the TypeError?

pablogsal · 2026-10-12T16:06:04Z


        PyObject *interp_id_obj = PyStructSequence_GET_ITEM(interp_info, 0);
        PyObject *threads = PyStructSequence_GET_ITEM(interp_info, 1);
+        CHECK_LIST(threads);


This still crashes when status.index clears threads: the next iteration uses the cached size and reads freed storage. We need mutation-safe traversal and a regression test.

pablogsal · 2026-10-12T16:06:04Z

 process_thread_sample(BinaryWriter *writer, PyObject *thread_info,
                      uint32_t interpreter_id, uint64_t timestamp_us)
 {
+    CHECK_TUPLE_ITEMS(thread_info, 3);


A malformed second thread still counts the first and breaks the writer, so earlier valid samples cannot be finalized. Can we validate before committing the sample?

pablogsal · 2026-10-12T16:06:06Z

        int32_t end_column = LOCATION_NOT_AVAILABLE;

        if (location != Py_None) {
+            CHECK_TUPLE_ITEMS(location, 4);


Yes, please, a separate PR for the integer-location branches and their tests makes sense. Let's keep this one focused on container validation. Thanks!

maurycy added 2 commits September 9, 2026 20:03

CHECK_TUPLE_ITEMS, CHECK_LIST macros

aec95c2

test

9a75080

maurycy requested a review from pablogsal as a code owner September 9, 2026 18:15

bedevere-app Bot mentioned this pull request Sep 9, 2026

_remote_debugging: BinaryWriter.write_sample() trivial segfault #157227

Open

bedevere-app Bot added the awaiting review label Sep 9, 2026

maurycy commented Sep 9, 2026

View reviewed changes

pablogsal reviewed Sep 11, 2026

View reviewed changes

maurycy mentioned this pull request Oct 1, 2026

_remote_debugging: write_sample advances the timestamp before writing the sample #158550

Closed

maurycy added 3 commits October 4, 2026 21:39

Merge branch 'main' into fix-remote-debugging-writer-validation

0e4f214

Merge branch 'main' into fix-remote-debugging-writer-validation

242cfb1

Merge branch 'main' into fix-remote-debugging-writer-validation

f449a15

pablogsal reviewed Oct 12, 2026

View reviewed changes

Navigation Menu

Repository navigation

gh-157227: `_remote_debugging`: Validate lists and tuples in `write_sample` - #157228

gh-157227: `_remote_debugging`: Validate lists and tuples in `write_sample`#157228
maurycy wants to merge 5 commits into
python:mainfrom
maurycy:fix-remote-debugging-writer-validation

maurycy commented Sep 9, 2026 •

edited

Loading

Uh oh!

maurycy Sep 9, 2026 •

edited

Loading

Uh oh!

pablogsal Sep 11, 2026

Uh oh!

pablogsal Sep 11, 2026

Uh oh!

pablogsal Sep 11, 2026

Uh oh!

maurycy Oct 4, 2026 •

edited

Loading

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

pablogsal Oct 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	#define CHECK_LIST_OR_TUPLE(v) \
	if (!PyList_Check(v) && !PyTuple_Check(v)) { \
	PyErr_SetString(PyExc_TypeError, \
	#v " must be a list or a tuple"); \
	return NULL; \
	}

	#define CHECK_LIST_OR_TUPLE(v) \
	do { \
	if (!PyList_Check(v) && !PyTuple_Check(v)) { \
	PyErr_SetString(PyExc_TypeError, \
	#v " must be a list or a tuple"); \
	return NULL; \
	} \
	} while (0)

	/* We allow the state tuple to be longer than 4, because we may need
	someday to extend the object's state without breaking
	backward-compatibility. */
	if (!PyTuple_Check(state) \|\| PyTuple_GET_SIZE(state) < 4) {
	PyErr_Format(PyExc_TypeError,
	"%.200s.__setstate__ argument should be 4-tuple, got %.200s",
	Py_TYPE(self)->tp_name, Py_TYPE(state)->tp_name);
	return NULL;
	}

	tmp_item = PyList_GET_ITEM(filters, i);
	if (!PyTuple_Check(tmp_item) \|\| PyTuple_GET_SIZE(tmp_item) != 5) {
	PyErr_Format(PyExc_ValueError,
	"warnings.%s item %zd isn't a 5-tuple", list_name, i);
	result = false;
	break;
	}

	/* Python code: action, msg, cat, mod, ln = item */
	Py_INCREF(tmp_item);
	action = PyTuple_GET_ITEM(tmp_item, 0);
	msg = PyTuple_GET_ITEM(tmp_item, 1);
	cat = PyTuple_GET_ITEM(tmp_item, 2);
	mod = PyTuple_GET_ITEM(tmp_item, 3);
	ln_obj = PyTuple_GET_ITEM(tmp_item, 4);

Conversation

maurycy commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maurycy Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurycy Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maurycy commented Sep 9, 2026 •

edited

Loading

maurycy Sep 9, 2026 •

edited

Loading

maurycy Oct 4, 2026 •

edited

Loading