Page MenuHomeFreeBSD

(RFC) Implement TIOCNOTTY
ClosedPublic

Authored by kevans on Wed, Nov 27, 5:01 PM.

Details

Reviewers
ian
kib
Group Reviewers
manpages
Commits
rS355248: tty: implement TIOCNOTTY
Summary

Generally, it's preferred that an application fork/setsid if it doesn't want to keep its controlling TTY, but it could be that a debugger is trying to steal it instead -- so it would hook in, drop the controlling TTY, then do some magic to set things up again. In this case, TIOCNOTTY is quite handy -- and still respected by at least OpenBSD, NetBSD, and Linux as far as I can tell.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kevans created this revision.Wed, Nov 27, 5:01 PM
bcr accepted this revision as: manpages.Wed, Nov 27, 5:59 PM
bcr added a subscriber: bcr.

OK from manpages.

kevans added inline comments.Wed, Nov 27, 6:01 PM
sys/kern/tty.c
1230 ↗(On Diff #64948)

It occurs to me that this should just kick back an error if not invoked on the controlling TTY, since /dev/ctty cannot be opened AFAICT.

kib added inline comments.Wed, Nov 27, 6:29 PM
sys/kern/tty.c
1217 ↗(On Diff #64948)

Should there be an assert that all of them are not NULL ? (I am not sure).

1232 ↗(On Diff #64948)

Could this allow userspace to create LOR ? Think about two processes with different cttys, each of which open other's ctty and does TIOCNTTY.

Hm, no because proctree_lock is locked exclusively. But this is still weird, and it is in-line with your comment at line 1230.

1257 ↗(On Diff #64948)

You may want to look at devfs_close(), if you did not already.

kevans added inline comments.Wed, Nov 27, 9:24 PM
sys/kern/tty.c
1217 ↗(On Diff #64948)

Hmm... I think we can't. There's a pretty small window between TIOCSCTTY and the devfs_ioctl handler subsequently grabbing shared proctree lock to set up ttyvp/ttydp in which we could end up racing TIOCNOTTY/TIOCSCTTY, if someone were... so inclined.

I do suspect devfs_ioctl should probably be amended to not VREF vp or alter the session if we lost p_session->s_ttyp between d_ioctl and re-acquiring the proctree lock, to be safe. I question the vpold parts of devfs_ioctl, though -- if TIOCSCTTY succeeded, either p->p_session == t->t_session and s_ttyvp == vp (unless maybe devfs multi-mount?) or there was no previous controlling tty.

kevans added inline comments.Thu, Nov 28, 9:59 PM
sys/kern/tty.c
1257 ↗(On Diff #64948)

Ok, I think this makes sense. If my reading's correct, devfs_close doesn't drop the s_ttyvp just yet (despite knowing that it's right) because we want to ensure it doesn't get recycled until the controlling tty's actually going away (e.g. process exit/death, TIOCSTTY, or TIOCNOTTY).

kevans updated this revision to Diff 65031.Thu, Nov 28, 10:16 PM

Highlights:

  • Don't allow TIOCNOTTY to be invoked on any TTY that isn't the controlling TTY -- locking looks less weird now.
  • Make sure we're covered in case of a race between TIOCNOTTY/TIOCSCTTY, don't touch the session if ttyp has disappeared.
kib added inline comments.Thu, Nov 28, 10:57 PM
share/man/man4/tty.4
205 ↗(On Diff #65031)

s/provided/provides/ ?

sys/kern/tty.c
1203 ↗(On Diff #65031)

I think it is better to note why it is safe to relock. Also, do you need to check tty_gone() after relock ?

kevans updated this revision to Diff 65035.Fri, Nov 29, 12:24 AM
kevans marked 5 inline comments as done.
  • Editorial changes
  • Also make sure the tty hasn't gotten away while we dropped the lock
kib accepted this revision.Fri, Nov 29, 12:39 AM
This revision is now accepted and ready to land.Fri, Nov 29, 12:39 AM
This revision was automatically updated to reflect the committed changes.