Skip to content

Fix reuse of pty slave devices - #2449

Open
cgull wants to merge 2 commits into
ish-app:masterfrom
cgull:pty-reuse-fix
Open

Fix reuse of pty slave devices#2449
cgull wants to merge 2 commits into
ish-app:masterfrom
cgull:pty-reuse-fix

Conversation

@cgull

@cgull cgull commented Aug 31, 2024

Copy link
Copy Markdown
Contributor

This should fix the problem brought up by @francoisvignon in my original work on #2387. At the time I wrote that I hadn't even thought about pty masters allowing serial reuse of the pty slave, and so it worked more like physical serial devices, where you don't want them to work after the modem hangs up.

This is definitely one of those less well documented and supported bits of functionality in ptys (there are many). But it seems like a good idea to support this because Linux does.

Comment thread fs/tty.c
lock(&tty->lock);
if (tty->hung_up || pty_is_half_closed_master(tty)) {
unlock(&pids_lock);
err = -1;

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.

This should be an errno constant

Comment thread fs/pty.c
// hang_up on the pty master. But the session leader may have a
// reference, and the pty master always has a reference.
if (tty->refcount - 1 == (tty->session ? 2 : 1)) {
tty->pty.other->hung_up = false;

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.

This needs locking, right?

Comment thread fs/pty.c
// If userland's reference count on the pty slave is now 1, clear
// hang_up on the pty master. But the session leader may have a
// reference, and the pty master always has a reference.
if (tty->refcount - 1 == (tty->session ? 2 : 1)) {

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.

I found this check a bit unsettling in the last PR but figured it could be fixed later. Seeing it again, I'm thinking much the same thing... Ideally we would define explicitly which references count for this and which ones don't, and with that knowledge the likely solution is to create a second refcount field for tracking just those.

@emkey1

emkey1 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Independently reproduced and confirmed — this fixes a real divergence from Linux, and the patch still applies cleanly to current master (7864dd6).

Test: open a pty master, open its slave, write and read a line, close the slave fully, then reopen the same slave and write another line.

int m = posix_openpt(O_RDWR|O_NOCTTY); grantpt(m); unlockpt(m);
ptsname_r(m, name, sizeof name);
int s  = open(name, O_RDWR|O_NOCTTY);  write(s,  "one\n", 4);  read(m, ...);
close(s);
int s2 = open(name, O_RDWR|O_NOCTTY);  write(s2, "two\n", 4);  read(m, ...);

Same static i386 binary, four ways:

after reopen
real Linux reads 5 bytes, "two"
upstream master reads 0 bytes (EOF) — master stays hung up
master + this PR reads 5 bytes, "two" — matches Linux
iSH-AOK fork reads 5 bytes, "two"

The failure mode on master is a silent EOF rather than an error, which is the kind of thing that makes a reusing program look like it just stopped receiving output.

Two notes, neither a objection:

  • The err = -1 in tty_read's hangup path looks like a separate correctness fix bundled in — err reaching goto error uninitialized otherwise. Worth calling out in the description since it is not obviously part of "pty slave reuse".
  • The meson.build hunk adding ish-lldb.lldb is unrelated to the pty change. Given the one-commit-per-PR convention it may be worth splitting, purely to keep this one narrowly reviewable.

@cgull — for what it is worth, our fork reached equivalent behaviour independently, so the semantics you are restoring here match what we settled on too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants