Re: screen says "Bad tty" if /dev/console is a symlink

[Guillem Jover <guillem@debian.org>]

On Sun, 2013-01-27 at 01:30:54 +0400, Игорь Пашев wrote:
> Did I break anything?
> Too paranoid?

Given the “intention” of the previous code, not enough I'd say. :)

About the question of removing the pathname checks, that's something
the maintainers would have to answer.

> Index: screen/tty.sh
> ===================================================================
> --- screen.orig/tty.sh  2013-01-26 07:55:33.092171499 +0000
> +++ screen/tty.sh       2013-01-26 08:56:34.254462991 +0000
> @@ -1505,12 +1505,29 @@
>  CheckTtyname (tty)
>  char *tty;
>  {
> +  char real[MAX_PATH];

I guess you meant PATH_MAX here, in any case POSIX does not guarantee
MAX variables to be defined, it would be better to use the POSIX.1-2008
variant of realpath(3) that allocates when passed a NULL (by checking
if it's available at configure time).

>    struct stat st;
> -
> -  if (lstat(tty, &st) || !S_ISCHR(st.st_mode) ||
> -     (st.st_nlink > 1 && strncmp(tty, "/dev/", 5)))
> -    return -1;
> -  return 0;
> +  if (0 == lstat(tty, &st)) {
> +    if (S_ISCHR(st.st_mode)) {
> +      if ((1 == st.st_nlink) || (0 == strncmp(tty, "/dev/", 5)))
> +          return 0;
> +    } else if (S_ISLNK(st.st_mode)) {
> +      if (0 == stat(tty, &st) && S_ISCHR(st.st_mode)) {
> +        if (1 == st.st_nlink) {
> +           return 0;
> +        } else if (realpath(tty, real)) {

I don't understand why do you a realpath() here depending on the
number of hard links, but see below.

> +#if defined(__sun__)
> +            if (!strncmp(real, "/devices/", 9))
> +              return 0;
> +#else
> +            if (!strncmp(real, "/dev/", 5))
> +              return 0;
> +#endif
> +        }

The first thing to do should be the realpath(), otherwise you are
exposed to TOCTOU issues, also it will simplify the code as you don't
have any longer different codepaths doing mostly the same (here you
check the path in different ways depending on the codepath).

But if the code is using this path to open(2) it later on, then you
should simply do the realpath() before calling this function so that
that same path can be used for that open() call, which means you
don't need to modify this function at all. Otherwise if this function
is called on something like ttyname(3)'s return value, then changing
this function is the correct thing to do.

Regards,
Guillem