[PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[Takashi Yano]

The commit 25c4ad6ea52f did not fix the CTTY behavior enough. For
example, in the following test case, TTY will be associated as
a CTTY on the second open() call even though the TTY is already
CTTY of another session. This patch fixes the issue.

  #include <unistd.h>
  #include <sys/fcntl.h>

  int main()
  {
    if (fork () == 0) {
      char *tty = ttyname(0);
      int fd;
      setsid();
      fd = open(tty, O_RDWR);
      close(fd);
      fd = open(tty, O_RDWR);
      usleep (60000000L);
    }
    return 0;
  }

Fixes: 25c4ad6ea52f ("Cygwin: pinfo: Align CTTY behavior to the
statement of POSIX.")
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/termios.cc | 1 -
 winsup/cygwin/pinfo.cc            | 5 +++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/fhandler/termios.cc b/winsup/cygwin/fhandler/termios.cc
index f94e20ff6..5b92cdd31 100644
--- a/winsup/cygwin/fhandler/termios.cc
+++ b/winsup/cygwin/fhandler/termios.cc
@@ -736,7 +736,6 @@ fhandler_termios::ioctl (int cmd, void *varg)
       return -1;
     }
 
-  myself->ctty = -1;
   if (!myself->set_ctty (this, 0))
     {
       set_errno (EPERM);
diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
index 586a4204d..735b3be8e 100644
--- a/winsup/cygwin/pinfo.cc
+++ b/winsup/cygwin/pinfo.cc
@@ -530,7 +530,7 @@ _pinfo::set_ctty (fhandler_termios *fh, int flags)
   debug_printf ("old %s, ctty device number %y, tc.ntty device number %y flags & O_NOCTTY %y", __ctty (), ctty, tc.ntty, flags & O_NOCTTY);
   if (fh && (ctty <= 0 || ctty == tc.ntty) && !(flags & O_NOCTTY))
     {
-      if (tc.getsid () && tc.getsid () != sid)
+      if (tc.getsid () && tc.getsid () != sid && ctty == -2)
 	; /* Do nothing if another session is associated with the TTY. */
       else
 	{
@@ -576,7 +576,8 @@ _pinfo::set_ctty (fhandler_termios *fh, int flags)
 	 an obvious bug surfaces. */
       if (sid == pid && !tc.getsid ())
 	tc.setsid (sid);
-      sid = tc.getsid ();
+      if (ctty > 0)
+	sid = tc.getsid ();
       /* See above */
       if ((!tc.getpgid () || being_debugged ()) && pgid == pid)
 	tc.setpgid (pgid);
-- 
2.39.0

Re: [PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[Corinna Vinschen]

Hi Takashi,

On Dec 28 17:35, Takashi Yano wrote:
> The commit 25c4ad6ea52f did not fix the CTTY behavior enough. For
> example, in the following test case, TTY will be associated as
> a CTTY on the second open() call even though the TTY is already
> CTTY of another session. This patch fixes the issue.

The patch is ok, thanks.

But while looking into this patch, I realized how confusing the old code
is.  An unsuspecting reader will have a really hard time to figure out
what ctty values of -1 or -2 actually mean.  The CVS log entry from 2012
isn't enlightening either:

  On second thought, in the spirit of keeping things kludgy, set ctty to
  -2 here as a special flag ...

Would you mind to introduce speaking symbolic values for them and add
some comments to make them more transparent?

Also, given this was a "kludge" from 10 years ago, is it really still
needed?

As I said, it's confusing :}


Thanks,
Corinna

Re: [PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[Takashi Yano]

On Mon, 9 Jan 2023 10:25:33 +0100
Corinna Vinschen wrote:
> On Dec 28 17:35, Takashi Yano wrote:
> > The commit 25c4ad6ea52f did not fix the CTTY behavior enough. For
> > example, in the following test case, TTY will be associated as
> > a CTTY on the second open() call even though the TTY is already
> > CTTY of another session. This patch fixes the issue.
> 
> The patch is ok, thanks.
> 
> But while looking into this patch, I realized how confusing the old code
> is.  An unsuspecting reader will have a really hard time to figure out
> what ctty values of -1 or -2 actually mean.  The CVS log entry from 2012
> isn't enlightening either:
> 
>   On second thought, in the spirit of keeping things kludgy, set ctty to
>   -2 here as a special flag ...
> 
> Would you mind to introduce speaking symbolic values for them and add
> some comments to make them more transparent?

Ok. Do you mean, first push this CTTY patch, then,
add comment for ctty values -1 and -2 in another patch?

> Also, given this was a "kludge" from 10 years ago, is it really still
> needed?
> 
> As I said, it's confusing :}

Currently, the special values mean:
-1: CTTY is not initialized yet. Can associate with the TTY
    which is associated with the own session.
-2: CTTY has been released by setsid(). Can associate with
    a new TTY as CTTY, but cannot associate with the TTYs
    already associated with other sessions.

So, I think the two different values are necessary. 

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[Corinna Vinschen]

On Jan 10 18:52, Takashi Yano wrote:
> On Mon, 9 Jan 2023 10:25:33 +0100
> Corinna Vinschen wrote:
> > On Dec 28 17:35, Takashi Yano wrote:
> > > The commit 25c4ad6ea52f did not fix the CTTY behavior enough. For
> > > example, in the following test case, TTY will be associated as
> > > a CTTY on the second open() call even though the TTY is already
> > > CTTY of another session. This patch fixes the issue.
> > 
> > The patch is ok, thanks.
> > 
> > But while looking into this patch, I realized how confusing the old code
> > is.  An unsuspecting reader will have a really hard time to figure out
> > what ctty values of -1 or -2 actually mean.  The CVS log entry from 2012
> > isn't enlightening either:
> > 
> >   On second thought, in the spirit of keeping things kludgy, set ctty to
> >   -2 here as a special flag ...
> > 
> > Would you mind to introduce speaking symbolic values for them and add
> > some comments to make them more transparent?
> 
> Ok. Do you mean, first push this CTTY patch, then,
> add comment for ctty values -1 and -2 in another patch?

Sure, that would be fine.

> 
> > Also, given this was a "kludge" from 10 years ago, is it really still
> > needed?
> > 
> > As I said, it's confusing :}
> 
> Currently, the special values mean:
> -1: CTTY is not initialized yet. Can associate with the TTY
>     which is associated with the own session.
> -2: CTTY has been released by setsid(). Can associate with
>     a new TTY as CTTY, but cannot associate with the TTYs
>     already associated with other sessions.
> 
> So, I think the two different values are necessary. 

Ok, good to know.


Thanks,
Corinna

Re: [PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[Takashi Yano]

On Mon, 9 Jan 2023 10:25:33 +0100
Corinna Vinschen wrote:
> Also, given this was a "kludge" from 10 years ago, is it really still
> needed?

Ah, do you mean the "kludge":
winsup/cygwin/syscalls.cc: 1455:
      /* This is a temporary kludge until all utilities can catch up
	 with a change in behavior that implements linux functionality:
	 opening a tty should not automatically cause it to become the
	 controlling tty for the process.  */
      if (!(flags & O_NOCTTY) && fd > 2 && myself->ctty != -2)
	{
	  flags |= O_NOCTTY;
	  /* flag that, if opened, this fhandler could later be capable
	     of being a controlling terminal if /dev/tty is opened. */
	  opt |= PC_CTTY;
	}

and

winsup/cygwin/dtable.cc: 767:
  /* This is a temporary kludge until all utilities can catch up with
     a change in behavior that implements linux functionality:  opening
     a tty should not automatically cause it to become the controlling
     tty for the process.  */
  if (newfd > 2)
    flags |= O_NOCTTY;
?

These codes might be able to be deleted. I'll check if these
are not needed.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH] Cygwin: pinfo: Additional fix for CTTY behavior.

[Corinna Vinschen]

On Jan 10 22:37, Takashi Yano wrote:
> On Mon, 9 Jan 2023 10:25:33 +0100
> Corinna Vinschen wrote:
> > Also, given this was a "kludge" from 10 years ago, is it really still
> > needed?
> 
> Ah, do you mean the "kludge":
> winsup/cygwin/syscalls.cc: 1455:
>       /* This is a temporary kludge until all utilities can catch up
> 	 with a change in behavior that implements linux functionality:
> 	 opening a tty should not automatically cause it to become the
> 	 controlling tty for the process.  */
>       if (!(flags & O_NOCTTY) && fd > 2 && myself->ctty != -2)
> 	{
> 	  flags |= O_NOCTTY;
> 	  /* flag that, if opened, this fhandler could later be capable
> 	     of being a controlling terminal if /dev/tty is opened. */
> 	  opt |= PC_CTTY;
> 	}
> 
> and
> 
> winsup/cygwin/dtable.cc: 767:
>   /* This is a temporary kludge until all utilities can catch up with
>      a change in behavior that implements linux functionality:  opening
>      a tty should not automatically cause it to become the controlling
>      tty for the process.  */
>   if (newfd > 2)
>     flags |= O_NOCTTY;
> ?
> 
> These codes might be able to be deleted. I'll check if these
> are not needed.

Actually I meant commit c38a2d837303, introducing the -2 value for ctty.
But yeah, the above stuff is also interesting and every opportunity to
get rid of old workarounds is nice.


Corinna