getpass fgets check

[Daniel Wainwright]

I don't believe that buf is guaranteed to be unchanged if there is an error
in fgets, according to the man page the contents are "indeterminate". So it
may write some characters to buf and not null-terminate it. So if you're
only assuming fgets conforms to the spec I think checking the return value
is safer, or is this an implementation detail of this fgets?

On 20 December 2011 22:40, William Pitcock <nenolod at dereferenced.org> wrote:

> Hi,
>
> On Tue, Dec 20, 2011 at 2:43 AM, Joakim Tjernlund
> <joakim.tjernlund at transmode.se> wrote:
> >> From: Daniel Wainwright <wainwright.daniel at gmail.com>
> >> To: uclibc at uclibc.org
> >> Date: 2011/12/20 08:44
> >> Subject: getpass fgets check
> >> Sent by: uclibc-bounces at uclibc.org
> >>
> >> Hi,
> >>
> >> I believe there is a simple error in getpass.c, line 80:
> >>
> >>
> >>
> >>   static char buf[PWD_BUFFER_SIZE];
> >>
> >>   ...
> >>
> >>   /* Read the password.  */
> >>   fgets (buf, PWD_BUFFER_SIZE-1, in);
> >>   if (buf != NULL)
> >>
> >>   ...
> >>
> >>
> >>
> >> So the result of fgets is not being checked here, results in reading the
> >> buffer uninitialised below.
> >
> > yes, and I think(if max passwd len is important) that it should read
> >  fgets (buf, PWD_BUFFER_SIZE, in)
> > as fgets man page says:
> >       fgets() reads in at most one less than size characters from stream
>  and
> >       stores  them  into  the buffer pointed to by s.
>
> I think that using 'sizeof buf' is cleaner and more futureproof than
> depending on PWD_BUFFER_SIZE.  Something like this should cleanly fix
> the potential usage of buf whilst in uninitialized state:
>
> diff --git a/libc/unistd/getpass.c b/libc/unistd/getpass.c
> index 8d80182..b8cb640 100644
> --- a/libc/unistd/getpass.c
> +++ b/libc/unistd/getpass.c
> @@ -76,9 +76,11 @@ char * getpass (const char *prompt)
>   fputs(prompt, out);
>   fflush(out);
>
> -  /* Read the password.  */
> -  fgets (buf, PWD_BUFFER_SIZE-1, in);
> -  if (buf != NULL)
> +  /* Read the password, ensuring buf is initialized first as fgets() may
> not
> +     touch the buffer itself. */
> +  *buf = '\0';
> +  fgets (buf, sizeof buf, in);
> +  if (*buf != '\0')
>     {
>       nread = strlen(buf);
>       if (nread < 0)
>
> Please test and see if that fixes your problem.
>
> William
>



-- 
Regards,

Daniel Wainwright