Skip to content

gh-124405: Fix NameError in openpty#124406

Merged
ambv merged 1 commit into
python:mainfrom
sobolevn:issue-124405
Sep 24, 2024
Merged

gh-124405: Fix NameError in openpty#124406
ambv merged 1 commit into
python:mainfrom
sobolevn:issue-124405

Conversation

@sobolevn

@sobolevn sobolevn commented Sep 24, 2024

Copy link
Copy Markdown
Member

@ambv how can I test this change? This looks very platform specific, but it seems that you have a reproducer :)

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks like the correct inlining of slave_open from #118826. Adding more test coverage sounds good though!

@sobolevn

Copy link
Copy Markdown
Member Author

Adding more test coverage sounds good though!

Thanks for the review! I need help adding tests, since I cannot reproduce this on my machine :(

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to sleep now, will look tomorrow. Based on typeshed I_PUSH should exist on Linux?

@hauntsaninja

Copy link
Copy Markdown
Contributor

"highly problematic" indeed ;-) https://github.com/python/typeshed/pull/6807/files#diff-f28c4d3b27ed19db8e4154e55b1ad119f6655aad863bd3e92d9aee2d481589d7R72

@vstinner

Copy link
Copy Markdown
Member

I tried to test this change by removing os.openpty():

import os
import pty
del os.openpty
master_fd, slave_fd = pty.openpty()
os.close(master_fd)
os.close(slave_fd)

But it fails on Linux with:

Traceback (most recent call last):
  File "/home/vstinner/python/main/x.py", line 4, in <module>
    master_fd, slave_fd = pty.openpty()
                          ~~~~~~~~~~~^^
  File "/home/vstinner/python/main/Lib/pty.py", line 34, in openpty
    master_fd, slave_name = _open_terminal()
                            ~~~~~~~~~~~~~~^^
  File "/home/vstinner/python/main/Lib/pty.py", line 58, in _open_terminal
    raise OSError('out of pty devices')
OSError: out of pty devices

On my Fedora 41, I have no /dev/pty* device, only /dev/tty* devices (97 files) that I cannot open (PermissionError) because my user is not in tty or dialout groups.

It seems like os.openpty() is implemented by communicating with /dev/ptmx device using ioctl(). strace:

openat(AT_FDCWD, "/dev/ptmx", O_RDWR)   = 3
ioctl(3, TIOCGPTN, [3])                 = 0
ioctl(3, TIOCSPTLCK, [0])               = 0
ioctl(3, TIOCGPTPEER, 0x102)            = 4
ioctl(3, FIOCLEX)                       = 0
ioctl(4, FIOCLEX)                       = 0

@vstinner

Copy link
Copy Markdown
Member

On my Fedora 41, I have no /dev/pty* device, only /dev/tty* devices (97 files) that I cannot open (PermissionError) because my user is not in tty or dialout groups.

Same on FreeBSD. On FreeBSD, os.openpty() is implemented with (syscalls recorded by truss):

posix_openpt(O_RDWR|O_NOCTTY)                    = 3 (0x3) 
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,FIODGNAME,0x820f8bb90)                   = 0 (0x0)
openat(AT_FDCWD,"/dev/pts/1",O_RDWR,00)          = 4 (0x4)
ioctl(3,FIOCLEX,0x0)                             = 0 (0x0)
ioctl(4,FIOCLEX,0x0)                             = 0 (0x0)

@vstinner

Copy link
Copy Markdown
Member

pty._open_terminal() code is quite old, it was added in 1994 when the pty module was created by commit 23cb2a8. The code almost didn't change since 1994: it still tries to open a /dev/ptyXY device and then use /dev/ttyXY device.

I guess that os.openpty() is now commonly available, and nobody uses the _open_terminal() fallback code path anymore, which looks broken on most platforms.

We should maybe do something with this code. Deprecate it. Remove it. I don't know :-)

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sobolevn

Copy link
Copy Markdown
Member Author

@vstinner thanks a lot for your help! 👍

@ambv ambv merged commit 17b3bc9 into python:main Sep 24, 2024
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.

4 participants