GeezerVison

I need to figure out how to test this -- it's a regression, after all, so no excuses this time.

Or... maybe I just mention the issue in a comment above the affected lines -- this will be my escape hatch to not write tests 💡

Ok, this one is clean and works!

Thanks for the contrib!

I need to figure out how to test this -- it's a regression, after all, so no excuses this time.

Or... maybe I just mention the issue in a comment above the affected lines -- this will be my escape hatch to not write tests 💡

My preference would be to create a regression test. It seems it shouldn't be too hard to create or identify a pseudofs object to pass in and catch the SystemExit. I'll work on one.

I saw in email that @johnslavik did some analysis on the root cause in readpath/readlink. It'd be nice to get that published somewhere. Even if there's nothing wrong with Python's wrapper around realpath, it may still be worth filing an issue to capture the investigation and consider updating the docs to flag this potential issue.

I've created #142383 with a test. Assuming that test is failing as expected, I'll try merging it with this fix.

I think I like it the way it is now. It's a problem of maintaining the equivalence invariant -- easy to follow.

@jaraco Can we display self._target in the error instead of target?

We could; it's not obvious to me what is the best choice. I elected not to change the behavior from what was stable (and just surgically address the one edge case). What's the case for using _target (realpath)? What was used on older Pythons?

What's the case for using _target (realpath)?

I'm not aiming to display realpath in the message, but to display the same path that was checked, not something potentially different which can lead to a confusing or incorrect message. It was part of the report -- had the same target that was checked been used in the error message, it would have been much clearer what happened. I'm fine with not displaying realpath, but only provided that we don't check the realpath -- so, what we check and what we display is always the same (this is the most important to me, because only then the message is certainly correct).

Apart from that, self._safe_realpath already does check whether the realpath exists, so to avoid checking the same path twice, I think that we should instead first check the original path and only then normalize to safe realpath. 4ca2bcf reverts this proposal.

What was used on older Pythons?

The discrepancy between what is displayed and what is checked was introduced in GH-26992.
Prior to that, realpath wasn't used for anything other than the sys.path[0] setup.

Thanks for the explanation. I do agree, it's a little disorienting that the check is on one value but the error message reflects a different value.

As you pointed out, GH-26992 originated this issue, and in that change, I explicitly attempted to avoid or to account for any meaningful changes to behavior. That included not changing the input path in the error message. I suspect it's important to the user that whatever they supplied as the script name is reflected in the error message (and not the realpath-mutated value).

So maybe it makes sense to use target for both the check and the error message (as you've proposed).

If that's important, that aspect is not captured by a regression test. I think we need a regression test or at least a comment that captures this refined expectation.

As I look at this further, I'm struggling to understand why the realpath issue didn't affect older Pythons.

Prior to that, realpath wasn't used for anything other than the sys.path[0] setup.

Except it was. The mutation happened here, which was then passed to _runscript here, which is consumed here. I don't yet understand why the broken realpath didn't cause problems subsequently.

Oh! I see now. The use of realpath was introduced in #23338. That suggests that the proper fix may be to go back to that older behavior and narrow the use of realpath to the sys.path[0] mutation.

In 855aac3, I've isolated the realpath usage to the sys.path[0] mutation. This change now fully restores the prior expectation that the indicated target is used as input. Does that fully address the issue?

That doesn't work. It causes a regression in test_issue42383.

Thanks for the explanation. I do agree, it's a little disorienting that the check is on one value but the error message reflects a different value.

Exactly.

As you pointed out, GH-26992 originated this issue, and in that change, I explicitly attempted to avoid or to account for any meaningful changes to behavior. That included not changing the input path in the error message. I suspect it's important to the user that whatever they supplied as the script name is reflected in the error message (and not the realpath-mutated value).

Precisely.

So maybe it makes sense to use target for both the check and the error message (as you've proposed).

If that's important, that aspect is not captured by a regression test. I think we need a regression test or at least a comment that captures this refined expectation.

Sure!

As I look at this further, I'm struggling to understand why the realpath issue didn't affect older Pythons.

Prior to that, realpath wasn't used for anything other than the sys.path[0] setup.

Except it was. The mutation happened here, which was then passed to _runscript here, which is consumed here. I don't yet understand why the broken realpath didn't cause problems subsequently.

Thank you -- that's correct, I must have misread it.

@jaraco Would the logic from 5b12904 suffice? I believe it passed all tests and it solved both problems, namely (1) potentially incorrect error message (2) support for live descriptors of internal kernel objects.
I can work further to adapt the tests to check the error messages as well.

@jaraco Thanks! Looks very good to me. Do we want a regrtest of the error message?

@jaraco Thanks! Looks very good to me. Do we want a regrtest of the error message?

I'm no longer thinking we should, especially now that the static method removes the temptation for there to be a mismatch.

Thanks @johnslavik for the PR, and @jaraco for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

GH-142497 is a backport of this pull request to the 3.14 branch.

GH-142498 is a backport of this pull request to the 3.13 branch.

Hi, I am seeing the following test failure on Solaris with this change present:

======================================================================
ERROR: test_script_target_anonymous_pipe (test.test_pdb.PdbTestCase.test_script_target_anonymous_pipe)
_ScriptTarget doesn't fail on an anonymous pipe.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builds/cpython-main/Lib/test/test_pdb.py", line 3687, in test_script_target_anonymous_pipe
    code_text = target.code
                ^^^^^^^^^^^
  File "/builds/cpython-main/Lib/pdb.py", line 228, in code
    with io.open_code(self._target) as fp:
         ~~~~~~~~~~~~^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: '/proc/22733/fd/4'

I am no expert here, but I tried looking into this and apparently opening anything other than regular file or directory from /proc/_pid_/root raises EACCES:
https://docs.oracle.com/cd/E88353_01/html/E37852/proc-5.html

It is however possible to open the pipe descriptor via /dev/fd and the following change fixes the issue:

--- ./cpython-main/Lib/test/test_pdb.py
+++ ./cpython-main/Lib/test/test_pdb.py
@@ -3564,8 +3564,9 @@
     def _fd_dir_for_pipe_targets(self):
         """Return a directory exposing live file descriptors, if any."""
         proc_fd = "/proc/self/fd"
-        if os.path.isdir(proc_fd) and os.path.exists(os.path.join(proc_fd, '0')):
-            return proc_fd
+        if not sys.platform.startswith("sunos"):
+            if os.path.isdir(proc_fd) and os.path.exists(os.path.join(proc_fd, '0')):
+                return proc_fd

         dev_fd = "/dev/fd"
         if os.path.isdir(dev_fd) and os.path.exists(os.path.join(dev_fd, '0')):