Skip to content

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented May 6, 2025

We can skip-news this one, it's a small fix on top of #133491 plus an integration test as @gaogaotiantian requested.

CC @ambv @pablogsal

@godlygeek godlygeek marked this pull request as draft May 6, 2025 06:16
@godlygeek
Copy link
Contributor Author

Marking as draft until I finish wrestling with getting the tests working on all platforms...

@godlygeek godlygeek changed the title gh-133490: Colorize remote pdb gh-133490: Fix syntax highlighting for remote PDB May 6, 2025
@godlygeek godlygeek marked this pull request as ready for review May 6, 2025 07:50
@pablogsal pablogsal merged commit fd37f1a into python:main May 6, 2025
55 checks passed
@gaogaotiantian
Copy link
Member

So first of all, I'm not sure if we should merge this without approval from @ambv. We introduced some new interface to the colorize system.

This is also not how I imagined the integration test should be. Why do we have to make the testing process part of the test? I don't think that makes it easier than juggling with 2 processes. Now that we have passed beta freeze, we don't need to rush into anything. I can work on a prototype and see how that works. Maybe I'm wrong - the integration tests is too fragile and we should do less of it.

The current test_pdb is using a separate process to run pdb tests and I think they worked fine. Of course it's just one process and there's no socket communication. I agree that when we first develop the test framework, there will be a lot of platform related issues, but most of those should only appear once.

@godlygeek
Copy link
Contributor Author

Why do we have to make the testing process part of the test? I don't think that makes it easier than juggling with 2 processes.

Because the Linux buildbots have /proc/sys/kernel/yama/ptrace_scope set to 1, and so if we spawn two child processes, one of them can't attach to the other.

We could spawn 1 child process and have it spawn a grandchild process that it's allowed to connect to. I tried it, but it was drastically more complicated. If you want to invest in it, though, it is possible, just ugly.

@gaogaotiantian
Copy link
Member

Because the Linux buildbots have /proc/sys/kernel/yama/ptrace_scope set to 1, and so if we spawn two child processes, one of them can't attach to the other.

That makes sense, and that also means pdb attach won't work in such scenario right? In real life.

@pablogsal
Copy link
Member

pablogsal commented May 6, 2025

That makes sense, and that also means pdb attach won't work in such scenario right? In real life.

Yeah but that is working as expected. Is the same deal with gdb or any other debugger. Including PEP 768

@godlygeek
Copy link
Contributor Author

that also means pdb attach won't work in such scenario right? In real life.

Unless you use sudo, yeah

@pablogsal
Copy link
Member

So first of all, I'm not sure if we should merge this without approval from @ambv. We introduced some new interface to the colorize system.

Not really, we are leveraging all the existing theme infrastructure and mirroring the pyrepl APIs. The APIs are also experimental in any case. We can change them if we need after beta 1. I have also reviewed the original change and I can confirm that this is aligned to how is intended to be used.

@gaogaotiantian
Copy link
Member

Yeah I understand that's expected behavior. I meant we are testing a feature on a platform that does not support the feature.

@pablogsal
Copy link
Member

I meant we are testing a feature on a platform that does not support the feature.

Not really, is just that the permission model is more restricted. The platform does support the feature and the feature will work if you do it between process on the same process tree.

@gaogaotiantian
Copy link
Member

The platform does support the feature and the feature will work if you do it between process on the same process tree.

What would be a reasonable use case in this scenario? How would user use python -m pdb -p for a process that is on the same tree (if I understand correctly, not siblings, they need to be parent/child)?

@godlygeek
Copy link
Contributor Author

How would user use python -m pdb -p for a process that is on the same tree

Yeah, the "same tree" thing won't help pdb -p. It's meant for things like strace or gdb that want to start a child process and then immediately attach to it and start tracing it. It doesn't help for strace -p or gdb -p or python -m pdb -p that want to attach to an arbitrary process. In all of those cases you'll either need to run the attaching process with sudo, or to have an administrator switch the YAMA ptrace scope to 0 instead of 1.

@godlygeek
Copy link
Contributor Author

And of course the integration tests that I just added rely on the "same tree" thing, by having the tests spawn a subprocess and then call pdb.main() directly. But that's not something users would ever want to do; that's specific to testing. If users wanted to have PDB launch a process, they'd just use

python -m pdb /path/to/script.py

@gaogaotiantian
Copy link
Member

What I meant is - if our buildbot does not provide an environment to support a real life scenario, then it's impossible for us to do reasonable integration test. I guess what we currently have is a compromise, which is not bad at all. I just wonder if we have some way to deal with the environment.

@godlygeek
Copy link
Contributor Author

It's not just about the build bots, either. Python's test suite is run by end users and redistributors, so even if the build bots could be set up with an environment with fewer security restrictions for the sake of realistic remote PDB integration tests, you still wouldn't want to rely on that, because it would still not work by default in most end user's Linux systems.

@gaogaotiantian
Copy link
Member

because it would still not work by default in most end user's Linux systems.

Do you mean that on most of the end users's Linux system, python -m pdb -p won't work by default?

@godlygeek
Copy link
Contributor Author

godlygeek commented May 6, 2025

Yes, most Linux distros default ptrace_scope to 1 these days as a security measure, and an administrator needs to specifically opt in to allowing a process to be debugged by an arbitrary process, either by running the debugger under sudo or granting the CAP_SYS_PTRACE capability or manually updating the ptrace_scope to 0 by doing

echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope

We should probably explicitly call out how to configure different platforms so that sys.remote_exec is permitted in the docs...

@pablogsal
Copy link
Member

pablogsal commented May 6, 2025

because it would still not work by default in most end user's Linux systems.

Do you mean that on most of the end users's Linux system, python -m pdb -p won't work by default?

Just to insist on this: this applies to the same as any other debugging tool on the system like gdb, strace or anything. It just needs sudo to work as a precaution if the system has yama activated.

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants