diff for usr.bin/mg/fileio.c

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

diff for usr.bin/mg/fileio.c

Han Boetes-3
I got a problem report from Mark Willson:

    "I recently installed mg (via the Debian package) under WSL on Windows
10.
    I found that the 'backup-to-home-directory' option didn't work.

    The cause of this appears to be that getlogin under WSL returns NULL,
    probably due to my use of wsltty to invoke bash.  The patch below fixes
    the issue for me:"

[snip]
-               if ((un = getlogin()) != NULL)
+               if ((un = getenv("LOGNAME")) != NULL)
[snip]

Which put me onto the track of what was going on. I found the
following in the Linux manpage:

BUGS
       Unfortunately, it is often rather easy to fool  getlogin().
 Sometimes
       it  does not work at all, because some program messed up the utmp
file.
       Often, it gives only the first 8 characters of  the  login  name.
 The
       user  currently  logged  in  on the controlling terminal of our
program
       need not be the user who started it.  Avoid  getlogin()  for
security-
       related purposes.

       Note  that glibc does not follow the POSIX specification and uses
stdin
       instead of /dev/tty.  A bug.  (Other recent systems, like SunOS 5.8
and
       HP-UX  11.11  and FreeBSD 4.8 all return the login name also when
stdin
       is redirected.)

       Nobody knows precisely what cuserid() does; avoid it in  portable
pro‐
       grams.   Or  avoid  it  altogether: use getpwuid(geteuid()) instead,
if
       that is what you meant.  Do not use cuserid().

So I started looking at the code and rewrote it a bit, which I think
makes it more portable and removes a syscall in the process. I do
suspect this can be written even more elegantly, but didn't want to
rework the code too much.

I also took the liberty to remove some whitespace.


Index: fileio.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/fileio.c,v
retrieving revision 1.104
@@ -703,7 +706,7 @@ expandtilde(const char *fn)
        struct stat      statbuf;
        const char      *cp;
        char             user[LOGIN_NAME_MAX], path[NFILEN];
-       char            *un, *ret;
+       char            *ret;
        size_t           ulen, plen;

        path[0] = '\0';
@@ -722,21 +725,21 @@ expandtilde(const char *fn)
                        return (NULL);
                return(ret);
        }
+       pw = getpwuid(geteuid());
        if (ulen == 0) { /* ~/ or ~ */
-               if ((un = getlogin()) != NULL)
-                       (void)strlcpy(user, un, sizeof(user));
+               if (pw != NULL)
+                       (void)strlcpy(user, pw->pw_name, sizeof(user));
                else
                        user[0] = '\0';
        } else { /* ~user/ or ~user */
                memcpy(user, &fn[1], ulen);
                user[ulen] = '\0';
        }
-       pw = getpwnam(user);
        if (pw != NULL) {
                plen = strlcpy(path, pw->pw_dir, sizeof(path));
                if (plen == 0 || path[plen - 1] != '/') {
                        if (strlcat(path, "/", sizeof(path)) >=
sizeof(path)) {
-                               dobeep();
+                               dobeep();
                                ewprintf("Path too long");
                                return (NULL);
                        }
Reply | Threaded
Open this post in threaded view
|

Re: diff for usr.bin/mg/fileio.c

Florian Obser-2
On Tue, Apr 10, 2018 at 06:49:10PM +0200, Han Boetes wrote:

> I got a problem report from Mark Willson:
>
>     "I recently installed mg (via the Debian package) under WSL on Windows
> 10.
>     I found that the 'backup-to-home-directory' option didn't work.
>
>     The cause of this appears to be that getlogin under WSL returns NULL,
>     probably due to my use of wsltty to invoke bash.  The patch below fixes
>     the issue for me:"
>
> [snip]
> -               if ((un = getlogin()) != NULL)
> +               if ((un = getenv("LOGNAME")) != NULL)
> [snip]
>
> Which put me onto the track of what was going on. I found the
> following in the Linux manpage:
>
> BUGS
>        Unfortunately, it is often rather easy to fool  getlogin().
>  Sometimes
>        it  does not work at all, because some program messed up the utmp
> file.
>        Often, it gives only the first 8 characters of  the  login  name.
>  The
>        user  currently  logged  in  on the controlling terminal of our
> program
>        need not be the user who started it.  Avoid  getlogin()  for
> security-
>        related purposes.
>
>        Note  that glibc does not follow the POSIX specification and uses
> stdin
>        instead of /dev/tty.  A bug.  (Other recent systems, like SunOS 5.8
> and
>        HP-UX  11.11  and FreeBSD 4.8 all return the login name also when
> stdin
>        is redirected.)
>
>        Nobody knows precisely what cuserid() does; avoid it in  portable
> pro‐
>        grams.   Or  avoid  it  altogether: use getpwuid(geteuid()) instead,
> if
>        that is what you meant.  Do not use cuserid().
>
> So I started looking at the code and rewrote it a bit, which I think
> makes it more portable and removes a syscall in the process. I do
> suspect this can be written even more elegantly, but didn't want to
> rework the code too much.
>
> I also took the liberty to remove some whitespace.
>
>
> Index: fileio.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/fileio.c,v
> retrieving revision 1.104
> @@ -703,7 +706,7 @@ expandtilde(const char *fn)
>         struct stat      statbuf;
>         const char      *cp;
>         char             user[LOGIN_NAME_MAX], path[NFILEN];
> -       char            *un, *ret;
> +       char            *ret;
>         size_t           ulen, plen;
>
>         path[0] = '\0';
> @@ -722,21 +725,21 @@ expandtilde(const char *fn)
>                         return (NULL);
>                 return(ret);
>         }
> +       pw = getpwuid(geteuid());
>         if (ulen == 0) { /* ~/ or ~ */
> -               if ((un = getlogin()) != NULL)
> -                       (void)strlcpy(user, un, sizeof(user));
> +               if (pw != NULL)
> +                       (void)strlcpy(user, pw->pw_name, sizeof(user));
>                 else
>                         user[0] = '\0';
>         } else { /* ~user/ or ~user */
>                 memcpy(user, &fn[1], ulen);
>                 user[ulen] = '\0';
>         }
> -       pw = getpwnam(user);
>         if (pw != NULL) {
>                 plen = strlcpy(path, pw->pw_dir, sizeof(path));
>                 if (plen == 0 || path[plen - 1] != '/') {
>                         if (strlcat(path, "/", sizeof(path)) >=
> sizeof(path)) {
> -                               dobeep();
> +                               dobeep();
>                                 ewprintf("Path too long");
>                                 return (NULL);
>                         }

not quite, you leave pw unitialized in the else part.

Lucas Gabriel Vuotto came up with a similar patch (to a different problem) back in 2017:
https://marc.info/?l=openbsd-tech&m=149521389822841&w=2

Which I subsequently slacked on, sorry!

Here is a slightly tweaked version of Lucas' diff:
- removed now unused un variable
- geteuid() instead of getuid()

Han, Lucas, does this work for you?

diff --git fileio.c fileio.c
index 0987f6f30de..339088f5e2d 100644
--- fileio.c
+++ fileio.c
@@ -703,7 +703,7 @@ expandtilde(const char *fn)
  struct stat statbuf;
  const char *cp;
  char user[LOGIN_NAME_MAX], path[NFILEN];
- char *un, *ret;
+ char *ret;
  size_t ulen, plen;
 
  path[0] = '\0';
@@ -722,16 +722,13 @@ expandtilde(const char *fn)
  return (NULL);
  return(ret);
  }
- if (ulen == 0) { /* ~/ or ~ */
- if ((un = getlogin()) != NULL)
- (void)strlcpy(user, un, sizeof(user));
- else
- user[0] = '\0';
- } else { /* ~user/ or ~user */
+ if (ulen == 0) /* ~/ or ~ */
+ pw = getpwuid(geteuid());
+ else { /* ~user/ or ~user */
  memcpy(user, &fn[1], ulen);
  user[ulen] = '\0';
+ pw = getpwnam(user);
  }
- pw = getpwnam(user);
  if (pw != NULL) {
  plen = strlcpy(path, pw->pw_dir, sizeof(path));
  if (plen == 0 || path[plen - 1] != '/') {


--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: diff for usr.bin/mg/fileio.c

Han Boetes-3
Yes this also works for me.


On Thu, Apr 12, 2018 at 4:52 PM, Florian Obser <[hidden email]> wrote:

> On Tue, Apr 10, 2018 at 06:49:10PM +0200, Han Boetes wrote:
> > I got a problem report from Mark Willson:
> >
> >     "I recently installed mg (via the Debian package) under WSL on
> Windows
> > 10.
> >     I found that the 'backup-to-home-directory' option didn't work.
> >
> >     The cause of this appears to be that getlogin under WSL returns NULL,
> >     probably due to my use of wsltty to invoke bash.  The patch below
> fixes
> >     the issue for me:"
> >
> > [snip]
> > -               if ((un = getlogin()) != NULL)
> > +               if ((un = getenv("LOGNAME")) != NULL)
> > [snip]
> >
> > Which put me onto the track of what was going on. I found the
> > following in the Linux manpage:
> >
> > BUGS
> >        Unfortunately, it is often rather easy to fool  getlogin().
> >  Sometimes
> >        it  does not work at all, because some program messed up the utmp
> > file.
> >        Often, it gives only the first 8 characters of  the  login  name.
> >  The
> >        user  currently  logged  in  on the controlling terminal of our
> > program
> >        need not be the user who started it.  Avoid  getlogin()  for
> > security-
> >        related purposes.
> >
> >        Note  that glibc does not follow the POSIX specification and uses
> > stdin
> >        instead of /dev/tty.  A bug.  (Other recent systems, like SunOS
> 5.8
> > and
> >        HP-UX  11.11  and FreeBSD 4.8 all return the login name also when
> > stdin
> >        is redirected.)
> >
> >        Nobody knows precisely what cuserid() does; avoid it in  portable
> > pro‐
> >        grams.   Or  avoid  it  altogether: use getpwuid(geteuid())
> instead,
> > if
> >        that is what you meant.  Do not use cuserid().
> >
> > So I started looking at the code and rewrote it a bit, which I think
> > makes it more portable and removes a syscall in the process. I do
> > suspect this can be written even more elegantly, but didn't want to
> > rework the code too much.
> >
> > I also took the liberty to remove some whitespace.
> >
> >
> > Index: fileio.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/mg/fileio.c,v
> > retrieving revision 1.104
> > @@ -703,7 +706,7 @@ expandtilde(const char *fn)
> >         struct stat      statbuf;
> >         const char      *cp;
> >         char             user[LOGIN_NAME_MAX], path[NFILEN];
> > -       char            *un, *ret;
> > +       char            *ret;
> >         size_t           ulen, plen;
> >
> >         path[0] = '\0';
> > @@ -722,21 +725,21 @@ expandtilde(const char *fn)
> >                         return (NULL);
> >                 return(ret);
> >         }
> > +       pw = getpwuid(geteuid());
> >         if (ulen == 0) { /* ~/ or ~ */
> > -               if ((un = getlogin()) != NULL)
> > -                       (void)strlcpy(user, un, sizeof(user));
> > +               if (pw != NULL)
> > +                       (void)strlcpy(user, pw->pw_name, sizeof(user));
> >                 else
> >                         user[0] = '\0';
> >         } else { /* ~user/ or ~user */
> >                 memcpy(user, &fn[1], ulen);
> >                 user[ulen] = '\0';
> >         }
> > -       pw = getpwnam(user);
> >         if (pw != NULL) {
> >                 plen = strlcpy(path, pw->pw_dir, sizeof(path));
> >                 if (plen == 0 || path[plen - 1] != '/') {
> >                         if (strlcat(path, "/", sizeof(path)) >=
> > sizeof(path)) {
> > -                               dobeep();
> > +                               dobeep();
> >                                 ewprintf("Path too long");
> >                                 return (NULL);
> >                         }
>
> not quite, you leave pw unitialized in the else part.
>
> Lucas Gabriel Vuotto came up with a similar patch (to a different problem)
> back in 2017:
> https://marc.info/?l=openbsd-tech&m=149521389822841&w=2
>
> Which I subsequently slacked on, sorry!
>
> Here is a slightly tweaked version of Lucas' diff:
> - removed now unused un variable
> - geteuid() instead of getuid()
>
> Han, Lucas, does this work for you?
>
> diff --git fileio.c fileio.c
> index 0987f6f30de..339088f5e2d 100644
> --- fileio.c
> +++ fileio.c
> @@ -703,7 +703,7 @@ expandtilde(const char *fn)
>         struct stat      statbuf;
>         const char      *cp;
>         char             user[LOGIN_NAME_MAX], path[NFILEN];
> -       char            *un, *ret;
> +       char            *ret;
>         size_t           ulen, plen;
>
>         path[0] = '\0';
> @@ -722,16 +722,13 @@ expandtilde(const char *fn)
>                         return (NULL);
>                 return(ret);
>         }
> -       if (ulen == 0) { /* ~/ or ~ */
> -               if ((un = getlogin()) != NULL)
> -                       (void)strlcpy(user, un, sizeof(user));
> -               else
> -                       user[0] = '\0';
> -       } else { /* ~user/ or ~user */
> +       if (ulen == 0) /* ~/ or ~ */
> +               pw = getpwuid(geteuid());
> +       else { /* ~user/ or ~user */
>                 memcpy(user, &fn[1], ulen);
>                 user[ulen] = '\0';
> +               pw = getpwnam(user);
>         }
> -       pw = getpwnam(user);
>         if (pw != NULL) {
>                 plen = strlcpy(path, pw->pw_dir, sizeof(path));
>                 if (plen == 0 || path[plen - 1] != '/') {
>
>
> --
> I'm not entirely sure you are real.
>
Reply | Threaded
Open this post in threaded view
|

Re: diff for usr.bin/mg/fileio.c

Florian Obser-2
On Thu, Apr 12, 2018 at 07:04:23PM +0200, Han Boetes wrote:
> Yes this also works for me.
>

Awesome, any OKs?

Diff again for convenience:

diff --git fileio.c fileio.c
index 0987f6f30de..339088f5e2d 100644
--- fileio.c
+++ fileio.c
@@ -703,7 +703,7 @@ expandtilde(const char *fn)
  struct stat statbuf;
  const char *cp;
  char user[LOGIN_NAME_MAX], path[NFILEN];
- char *un, *ret;
+ char *ret;
  size_t ulen, plen;
 
  path[0] = '\0';
@@ -722,16 +722,13 @@ expandtilde(const char *fn)
  return (NULL);
  return(ret);
  }
- if (ulen == 0) { /* ~/ or ~ */
- if ((un = getlogin()) != NULL)
- (void)strlcpy(user, un, sizeof(user));
- else
- user[0] = '\0';
- } else { /* ~user/ or ~user */
+ if (ulen == 0) /* ~/ or ~ */
+ pw = getpwuid(geteuid());
+ else { /* ~user/ or ~user */
  memcpy(user, &fn[1], ulen);
  user[ulen] = '\0';
+ pw = getpwnam(user);
  }
- pw = getpwnam(user);
  if (pw != NULL) {
  plen = strlcpy(path, pw->pw_dir, sizeof(path));
  if (plen == 0 || path[plen - 1] != '/') {


--
I'm not entirely sure you are real.