Re: [PATCH] linux ttyname and ttyname_r: return link if appropriate

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Wed, Aug 10, 2016 at 06:24:30PM -0500, Serge E. Hallyn wrote:
> Quoting Dmitry V. Levin (ldv@altlinux.org):
> > On Tue, Aug 09, 2016 at 04:39:37PM -0500, Serge E. Hallyn wrote:
> > [...]
> > > --- a/sysdeps/unix/sysv/linux/ttyname.c
> > > +++ b/sysdeps/unix/sysv/linux/ttyname.c
> > [...]
> > > +      /* If the link doesn't exist, then it points to a device in another
> > > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > > +	 fd, as it points to a name unreachable in our namespace.  */
> > > +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> > > +	return strcpy (ttyname_buf, procname);
> > 
> > With buflen == 4095 and sizeof(procname) == 30, this buflen check looks
> > redundant.  To keep the safe side, I'd rather add a static assert instead,
> > e.g.
> > 
> > #define TTYNAME_BUFLEN 4095
> > ...
> > buflen = TTYNAME_BUFLEN;
> > ...
> > _Static_assert (sizeof (procname) < TTYNAME_BUFLEN, "buflen too small");
> > 
> > [...]
> > > --- a/sysdeps/unix/sysv/linux/ttyname_r.c
> > > +++ b/sysdeps/unix/sysv/linux/ttyname_r.c
> > [...]
> > > +      /* If the link doesn't exist, then it points to a device in another
> > > +	 namespace.  If it is a UNIX98 pty, then return the /proc/self
> > > +	 fd, as it points to a name unreachable in our namespace.  */
> > > +      if (is_pty (&st) && strlen (procname) < buflen - 1)
> > > +	{
> > > +	  strcpy (buf, procname);
> > > +	  return 0;
> > > +	}
> > 
> > Unlike ttyname.c, here buflen might be quite small, and this code skips
> > strlen (procname) == buflen - 1.  Shouldn't it rather be
> > strlen (procname) < buflen ?
> 
> Hm, yeah that's right.
> 
> > If is_pty(&st) is true but buflen is too small for procname, is there any
> > chance of finding the device using getttyname_r?  Shouldn't ttyname_r fail
> > with ERANGE instead?
> 
> So you mean
> 
> 	if (is_pty (&st)) {
> 		if (strlen (procname) < buflen) {
> 			strcpy (buf, procname);
> 			return 0;
> 		}
> 		return -ERANGE;
> 	}
> 
> ?

Yes, something like that.  In fact, it has to be

	__set_errno (ERANGE);
	return ERANGE;


-- 
ldv

Attachment: pgpXzVjIweN6n.pgp
Description: PGP signature