gh-131591: Implement PEP 768 #131592

pablogsal · 2025-03-22T20:05:40Z

Issue: Implement PEP 768 – Safe external debugger interface for CPython #131591

pablogsal · 2025-03-22T20:06:09Z

CC: @godlygeek @ivonastojanovic

Fix syntax error

cpython-cla-bot · 2025-03-25T16:00:03Z

The following commit authors need to sign the Contributor License Agreement:

stojanovic.i@hotmail.com

Click the button to sign:

godlygeek · 2025-03-26T00:01:27Z

Python/initconfig.c

@@ -317,6 +318,7 @@ The following implementation-specific options are available:\n\
 -X perf: support the Linux \"perf\" profiler; also PYTHONPERFSUPPORT=1\n\
 -X perf_jit: support the Linux \"perf\" profiler with DWARF support;\n\
         also PYTHON_PERF_JIT_SUPPORT=1\n\
+-X disable-remote-debug: disable remote debugging; also PYTHON_DISABLE_REMOTE_DEBUG\n\


All of the others use _ instead of - so this should probably be

Suggested change

-X disable-remote-debug: disable remote debugging; also PYTHON_DISABLE_REMOTE_DEBUG\n\

-X disable_remote_debug: disable remote debugging; also PYTHON_DISABLE_REMOTE_DEBUG\n\

godlygeek · 2025-03-26T00:03:21Z

Python/initconfig.c

+    if (env) {
+        active = 0;
+    }
+    const wchar_t *xoption = config_get_xoption(config, L"disable-remote-debug");


As above, all the other X options use underscores so this should probably be

Suggested change

const wchar_t *xoption = config_get_xoption(config, L"disable-remote-debug");

const wchar_t *xoption = config_get_xoption(config, L"disable_remote_debug");

godlygeek

Initial observations (I've looked at everything but the interesting stuff, haven't gotten to Python/remote_debugging.c or Lib/test/test_sys.py yet)

godlygeek · 2025-03-26T00:09:19Z

Include/cpython/pystate.h

+    int enabled;
+    int debugger_pending_call;


We should probably be explicit about our sizes:

Suggested change

int enabled;

int debugger_pending_call;

int32_t enabled;

int32_t debugger_pending_call;

Granted int is 32 bit on all modern platforms, but given that this goes into the offsets and needs to be read from another process with potentially different integer types (like a 64-bit process attaching to a 32-bit one or vice versa) it's better to be explicit, I think.

Or, if you'd prefer, the offsets themselves could contain the size of the field - but I think that's overkill.

godlygeek · 2025-03-26T00:14:24Z

Include/cpython/pystate.h

+/* Remote debugger support */
+# define MAX_SCRIPT_PATH_SIZE 512
+typedef struct _remote_debugger_support {
+    int enabled;


I believe this field is unused. You added enabled here, but never reference it anywhere, and the offsets instead use:

.remote_debugging_enabled = offsetof(PyInterpreterState, config.remote_debug),

I think this line should just be removed (which makes sense, anyway - we want a per-process enablement, not a per-thread enablement).

godlygeek · 2025-03-26T00:16:33Z

Include/internal/pycore_debug_offsets.h

+    .debugger_support = { \
+        .eval_breaker = offsetof(PyThreadState, eval_breaker), \
+        .remote_debugger_support = offsetof(PyThreadState, remote_debugger_support),  \
+        .remote_debugging_enabled = offsetof(PyInterpreterState, config.remote_debug),  \


Should this field really go into the .debugger_support section? I think it should probably go into .interpreter_state instead, since everything else in this struct is specific to a single threadstate, except for this one field being per-interpreter.

godlygeek · 2025-03-26T00:20:36Z

Include/internal/pycore_interp_structs.h

@@ -936,7 +936,6 @@ struct _is {
    _PyThreadStateImpl _initial_thread;
    // _initial_thread should be the last field of PyInterpreterState.
    // See https://github.com/python/cpython/issues/127117.
-


Accidental change?

godlygeek · 2025-03-26T00:26:38Z

Python/ceval_gil.c

+
+#ifdef Py_REMOTE_DEBUG
+    const PyConfig *config = _PyInterpreterState_GetConfig(tstate->interp);
+    if (config->remote_debug) {


Might be worth doing:

Suggested change

if (config->remote_debug) {

if (config->remote_debug == 1) {

to harden this at least a little bit against heap corruption.

godlygeek · 2025-03-26T00:57:02Z

Python/ceval_gil.c

+                    int fd = PyObject_AsFileDescriptor(fileobj);
+                    if (fd == -1) {
+                        PyErr_FormatUnraisable("Error when getting file descriptor for debugger script %s", path);
+                        return 0;


Leaks fileobj

godlygeek · 2025-03-26T00:59:10Z

Python/sysmodule.c

+
+If no stack profiler is activated, this function has no effect.


What? This seems like a copy-paste error that should just be removed.

Suggested change

If no stack profiler is activated, this function has no effect.

godlygeek · 2025-03-26T01:02:16Z

Python/sysmodule.c

+to how signals are handled. There is no interface to determine when the
+code has been executed. The caller is responsible for making sure that
+the file still exists whenever the remote process tries to read it and that
+it hasn't been overwritten.


It's probably worth documenting that the remote process needs to be CPython, and needs to be Python 3.14+, right?

godlygeek · 2025-03-26T01:04:30Z

Python/sysmodule.c

+{
+#ifdef MS_WINDOWS
+    // Get UTF-16 (wide char) version of the path for Windows
+    wchar_t *debugger_script_path_w = PyUnicode_AsWideCharString(script, NULL);


Above we said that script could be either str|bytes, but I believe PyUnicode_AsWideCharString only accepts str. Perhaps on Windows the bytes option doesn't even make sense, though. Should we document that Unix allows bytes but Windows does not?

godlygeek · 2025-03-26T01:08:10Z

Python/sysmodule.c

+
+    PyMem_Free(debugger_script_path_w);
+#else
+    const char *debugger_script_path = PyUnicode_AsUTF8(script);


Actually, looks like we're not currently handling bytes even for Unix.

pythongh-131591: Implement PEP 768

6f6b4cd

bedevere-app bot mentioned this pull request Mar 22, 2025

Implement PEP 768 – Safe external debugger interface for CPython #131591

Open

pablogsal and others added 6 commits March 23, 2025 00:39

Add tests and use io.open_code

9b86022

Some fixes

af84100

Prepare for windows support

19ef7ae

Update windows files

444453c

Fix syntax error

1d3ad3c

Merge pull request #97 from ivonastojanovic/fix_windows_files

eeec1f6

Fix syntax error

godlygeek reviewed Mar 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-131591: Implement PEP 768 #131592

gh-131591: Implement PEP 768 #131592

pablogsal commented Mar 22, 2025 •

edited by bedevere-app bot

Loading

pablogsal commented Mar 22, 2025

cpython-cla-bot bot commented Mar 25, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek left a comment

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

godlygeek Mar 26, 2025

	-X disable-remote-debug: disable remote debugging; also PYTHON_DISABLE_REMOTE_DEBUG\n\
	-X disable_remote_debug: disable remote debugging; also PYTHON_DISABLE_REMOTE_DEBUG\n\

	const wchar_t *xoption = config_get_xoption(config, L"disable-remote-debug");
	const wchar_t *xoption = config_get_xoption(config, L"disable_remote_debug");


		If no stack profiler is activated, this function has no effect.

gh-131591: Implement PEP 768 #131592

Are you sure you want to change the base?

gh-131591: Implement PEP 768 #131592

Conversation

pablogsal commented Mar 22, 2025 • edited by bedevere-app bot Loading

pablogsal commented Mar 22, 2025

cpython-cla-bot bot commented Mar 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godlygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Mar 22, 2025 •

edited by bedevere-app bot

Loading