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.
