On 05/14/2014 03:03 PM, Guilherme de Almeida Suckevicz wrote:
> Hi Pádraig,
>
> Thanks for the help! So, Adjusting just the part on non windows platforms, I
> arrive here.
> I'm just with doubt in the ASSERT calls.
>
> Please, if you can, take a look:
>
> diff --git a/tests/test-getlogin.c b/tests/test-getlogin.c
> index 197aed8..0a46267 100644
> --- a/tests/test-getlogin.c
> +++ b/tests/test-getlogin.c
> @@ -27,6 +27,11 @@ SIGNATURE_CHECK (getlogin, char *, (void));
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <unistd.h>
> +#include <pwd.h>
> +
> +#include <sys/stat.h>
> +#include <sys/types.h>
>
> #include "macros.h"
>
> @@ -62,11 +67,29 @@ main (void)
> #if !((defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__)
> /* Unix platform */
> {
> - const char *name = getenv ("LOGNAME");
> - if (name == NULL || name[0] == '\0')
> - name = getenv ("USER");
> - if (name != NULL && name[0] != '\0')
> - ASSERT (strcmp (buf, name) == 0);
> + const char *tty;
> + struct stat stat_buf;
> + struct passwd *pwd;
> +
> + tty = ttyname (STDIN_FILENO);
> + if (tty == NULL)
> + {
> + if (errno == ENOTTY)
> + {
> + fprintf (stderr, "Skipping test: stdin is not a tty.\n");
> + return 77;
> + }
> + /* Some other error, call assert. */
> + ASSERT (0);
> + }
> +
> + if (stat (tty, &stat_buf) < 0)
> + ASSERT (0);
> +
> + pwd = getpwuid (stat_buf.st_uid);
> + ASSERT (! (pwd == NULL));
> +
> + ASSERT (strcmp (pwd->pw_name, buf) == 0);
> }
> #endif
> #if (defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__
>
>
> Sorry for any mistake!
>
> Thank you very much!
> Guilherme Almeida.
>
>
> 2014-05-13 18:47 GMT-03:00 Pádraig Brady <[email protected]
> <mailto:[email protected]>>:
>
> On 05/13/2014 08:39 PM, Guilherme de Almeida Suckevicz wrote:
> > Hi Pádraig,
> >
> > Thanks for the answer! I will work on a patch for that.
> >
> > I think the better way to fix this is check the getlogin(3) against the
> > owner of the controlling terminal, this is, using the ttyname(3),
> stat(2)
> > and the field pw_name of getpwuid(3).
> >
> > Is it right!?
>
> Well first of all the test is really only checking the windows
> replacement function,
> so I wouldn't jump through many hoops to correlate the actual getlogin()
> independently.
> Anyway let's see what combos of env variables we have...
>
> tp1:~$ python -c "import os, pwd; print os.environ.get('USER'),
> os.environ.get('LOGNAME'), os.environ.get('SUDO_USER'), os.getlogin(),
> pwd.getpwuid(os.stat(os.ttyname(0)).st_uid).pw_name"
> padraig padraig None padraig padraig
> tp1:~$ sudo !!
> root root padraig padraig padraig
>
> tp1:~$ ssh root@tp2
> [root@tp2 ~]# su - padraig
> tp2:~$ python -c "import os, pwd; print os.environ.get('USER'),
> os.environ.get('LOGNAME'), os.environ.get('SUDO_USER'), os.getlogin(),
> pwd.getpwuid(os.stat(os.ttyname(0)).st_uid).pw_name"
> padraig padraig None root root
> tp2:~$ sudo !!
> root root padraig root root
>
> So we see the last 2 (your method and getlogin()) correlate for each case
> here.
> The problematic cases are when LOGNAME/USER are different from getlogin(),
> and I don't see any other combo of env variables to identify those cases,
> so it seems your method is best for direct correlation.
>
> Now on non windows systems we're not actually replacing any code,
> so the simple solution would be on non windows to not care
> about the returned value at all.
>
> I.E. I'd just do the following to remove the problematic code
> which is invalid in all the above caes. If we actually implement
> getlogin() on non windows we can jump though hoops then to
> do the correlation.
>
> diff --git a/tests/test-getlogin.c b/tests/test-getlogin.c
> index 197aed8..7b19f8c 100644
> --- a/tests/test-getlogin.c
> +++ b/tests/test-getlogin.c
> @@ -59,16 +59,6 @@ main (void)
> }
>
> /* Compare against the value from the environment. */
> -#if !((defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__)
> - /* Unix platform */
> - {
> - const char *name = getenv ("LOGNAME");
> - if (name == NULL || name[0] == '\0')
> - name = getenv ("USER");
> - if (name != NULL && name[0] != '\0')
> - ASSERT (strcmp (buf, name) == 0);
> - }
> -#endif
> #if (defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__
> /* Native Windows platform.
> Note: This test would fail on Cygwin in an ssh session, because sshd
>
> thanks,
> Pádraig.
>
>
So you went the route of adding rather than deleting code. Fair enough.
I tweaked a bit in the attached which I;ll apply soon if you're OK with it.
It passes these here:
./gnulib-tool --create-testdir --with-tests --test getlogin
sudo !!
thanks,
Pádraig.
>From 97249cf2945e8fe18ed138af20d6e46761fd0470 Mon Sep 17 00:00:00 2001
From: Guilherme de Almeida Suckevicz <[email protected]>
Date: Wed, 14 May 2014 22:06:24 +0100
Subject: [PATCH] getlogin-tests: avoid false failure under sudo/ssh etc.
* modules/getlogin-tests (configure.ac): Check for ttyname().
* tests/test-getlogin.c (main): Don't depend on environment variables
to correlate with getlogin(), since sudo and ssh etc. can tamper
with the LOGNAME and USER env vars. Instead lookup the name from
the uid associated with the stdin tty.
---
ChangeLog | 9 +++++++++
modules/getlogin-tests | 1 +
tests/test-getlogin.c | 36 +++++++++++++++++++++++++++++++-----
3 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a8b2ee0..19dfba5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2014-05-14 Guilherme de Almeida Suckevicz <[email protected]>
+
+ getlogin-tests: avoid false failure under sudo/ssh etc.
+ * modules/getlogin-tests (configure.ac): Check for ttyname().
+ * tests/test-getlogin.c (main): Don't depend on environment variables
+ to correlate with getlogin(), since sudo and ssh etc. can tamper
+ with the LOGNAME and USER env vars. Instead lookup the name from
+ the uid associated with the stdin tty.
+
2014-05-11 Paul Eggert <[email protected]>
mbsstr, quotearg, xstrtol: pacify IRIX 6.5 cc
diff --git a/modules/getlogin-tests b/modules/getlogin-tests
index 795fba7..6facd60 100644
--- a/modules/getlogin-tests
+++ b/modules/getlogin-tests
@@ -6,6 +6,7 @@ tests/macros.h
Depends-on:
configure.ac:
+AC_CHECK_FUNCS_ONCE([ttyname])
Makefile.am:
TESTS += test-getlogin
diff --git a/tests/test-getlogin.c b/tests/test-getlogin.c
index 197aed8..32fd893 100644
--- a/tests/test-getlogin.c
+++ b/tests/test-getlogin.c
@@ -27,6 +27,11 @@ SIGNATURE_CHECK (getlogin, char *, (void));
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <unistd.h>
+#include <pwd.h>
+
+#include <sys/stat.h>
+#include <sys/types.h>
#include "macros.h"
@@ -62,11 +67,32 @@ main (void)
#if !((defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__)
/* Unix platform */
{
- const char *name = getenv ("LOGNAME");
- if (name == NULL || name[0] == '\0')
- name = getenv ("USER");
- if (name != NULL && name[0] != '\0')
- ASSERT (strcmp (buf, name) == 0);
+# if HAVE_TTYNAME
+ const char *tty;
+ struct stat stat_buf;
+ struct passwd *pwd;
+
+ tty = ttyname (STDIN_FILENO);
+ if (tty == NULL)
+ {
+ ASSERT (errno == ENOTTY);
+
+ fprintf (stderr, "Skipping test: stdin is not a tty.\n");
+ return 77;
+ }
+
+ ASSERT (stat (tty, &stat_buf) == 0);
+
+ pwd = getpwuid (stat_buf.st_uid);
+ if (! pwd)
+ {
+ fprintf (stderr, "Skipping test: no name found for uid %d\n",
+ stat_buf.st_uid);
+ return 77;
+ }
+
+ ASSERT (strcmp (pwd->pw_name, buf) == 0);
+# endif
}
#endif
#if (defined _WIN32 || defined __WIN32__) && !defined __CYGWIN__
--
1.7.7.6
