Re: [PATCH] Fix getlogin() to check only stdin to get a valid tty

On 2023-07-12 19:33, Jordi Sanfeliu via Newlib wrote:
> Hello,
> 
> In my hobby OS [1] which uses Newlib C as its libc, I noticed that the 
> GNU command 'logname' does output nothing when it is redirected or pipe'd.
> 
> The current getlogin() implementation [2] forces the three primary file 
> descriptors (stdin, stdout and stderr) to be a valid tty before checking 
> the utmp file, otherwise it returns NULL. This makes impossible to 
> redirect (to a file or to a pipe), the output of a program that is using 
> this function because one of its file descriptors won't be a tty.

I think your analysis is wrong. See below for reasons why.

> diff --git a/newlib/libc/unix/getlogin.c b/newlib/libc/unix/getlogin.c
> index da4f47a95..e646bcb08 100644
> --- a/newlib/libc/unix/getlogin.c
> +++ b/newlib/libc/unix/getlogin.c
> @@ -16,9 +16,7 @@ getlogin ()
>     extern char *ttyname ();
>     char *tty;
> 
> -  if (((tty = ttyname (0)) == 0)
> -      || ((tty = ttyname (1)) == 0)
> -      || ((tty = ttyname (2)) == 0))

These 3 lines of code checks if one of stdin, stdout or stderr is 
connected to a terminal device. If the return value of ttyname is 0, it 
means that there is no terminal device connected to that fd.
As I read the code, it first tries with stdin. If stdin is closed or 
redirected, it tries with stdout instead and then lastly, falls back to 
trying with stderr. If none of the 3 fd's provides a terminal device, 
then the getlogin will return 0.

As a result, doing your change would force the process to have a 
connected stdin or the getlogin call would return 0.

> +  if ((tty = ttyname (0)) == 0)
>       return 0;
> 
>     if ((utmp_fd = open (UTMP_FILE, O_RDONLY)) == -1)
> 
> 
> 1. https://www.fiwix.org
> 2. 
> https://sourceware.org/git/?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/unix/getlogin.c;hb=HEAD
> 


While looking at the code pointed to in link 2, I'm a bit puzzled about 
the following lines:

static char buf[10];
...
strncpy (buf, utmp_buf.ut_user, sizeof (utmp_buf.ut_user));

As buf is 10 bytes, I suppose sizeof(utmp_buf.ut_user) should never be 
allowed to exceed 10 bytes, but in the newlib source tree, I find these 
files that define a size for utmp_buf.ut_user:

./newlib/libc/sys/sysvi386/sys/utmp.h: 8
./winsup/cygwin/include/cygwin/utmp.h: 16

I'm not sure if this getlogin.c file is included in a cygwin build, but 
if it is, I think there is a buffer overflow here.


Kind regards,
Torbjörn