fix watch implementation

fix watch implementation

[Jens Elkner]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

Hi,

when evaluating zsh as the new default shell for interactive use it was
discovered, that the 'watch' feature doesn't work on Solaris 11.3 and
Ubuntu 14.04, 15.04 and probably other modern platforms: 'log' never
shows anything and logins/logouts never show up.

The root cause is, that the current implementation uses a rather
discouraged way to obtain utmp[x] records, i.e. it reads binary records
instead of using the POSIX/recommended way, i.e. {set,get,end}ut[x]ent() -
similar to getpwent(). To check I just inserted a printf: 1st record
seems to be ok but is always of ut_type != USER_PROCESS and all following
records read contain really garbage with ut_type = 0 (probably by
accident). So no wonder, why it doesn't work as expected.

The attached patch fixes the implementation, has been successfully
tested on the platforms mentioned above (was developed against 5.0.7,
which is, what the vendors of the mentioned OS ship, however, someone on
irc reported, that it applies cleanly to the current master version as
well).

Would be nice if it could be merged ASAP. IIRC, we started to evaluate
zsh as default shell 5+ years ago. First test was, whether watch works:
it didn't, but had no time to dig deeper, so we stopped and postponed
the eval ...

have fun,
jel.
-- 
Otto-von-Guericke University     http://www.cs.uni-magdeburg.de/
Department of Computer Science   Geb. 29 R 027, Universitaetsplatz 2
39106 Magdeburg, Germany         Tel: +49 391 67 52768

[-- Attachment #2: watch.patch --]
[-- Type: text/plain, Size: 3971 bytes --]

--- zsh-5.0.7/Src/watch.c.orig	Thu Jul 31 20:41:49 2014
+++ zsh-5.0.7/Src/watch.c	Wed Jan 11 16:37:49 2017
@@ -87,6 +87,9 @@
 
 #if !defined(WATCH_STRUCT_UTMP) && defined(HAVE_STRUCT_UTMPX) && defined(REAL_UTMPX_FILE)
 # define WATCH_STRUCT_UTMP struct utmpx
+# define setutent setutxent
+# define getutent getutxent
+# define endutent endutxent 
 /*
  * In utmpx, the ut_name field is replaced by ut_user.
  * Howver, on some systems ut_name may already be defined this
@@ -141,9 +144,9 @@
 #  define WATCH_WTMP_FILE "/dev/null"
 # endif
 
-static int wtabsz;
-static WATCH_STRUCT_UTMP *wtab;
-static time_t lastutmpcheck;
+static int wtabsz = 0;
+static WATCH_STRUCT_UTMP *wtab = NULL;
+static time_t lastutmpcheck = 0;
 
 /* get the time of login/logout for WATCH */
 
@@ -449,34 +452,44 @@
 /* initialize the user List */
 
 /**/
-static void
-readwtab(void)
+static int
+readwtab(WATCH_STRUCT_UTMP **head, int initial_sz)
 {
-    WATCH_STRUCT_UTMP *uptr;
-    int wtabmax = 32;
-    FILE *in;
+    WATCH_STRUCT_UTMP *uptr, *tmp;
+    int wtabmax = initial_sz < 2 ? 32 : initial_sz;
+    int sz = 0;
 
-    wtabsz = 0;
-    if (!(in = fopen(WATCH_UTMP_FILE, "r")))
-	return;
-    uptr = wtab = (WATCH_STRUCT_UTMP *)zalloc(wtabmax * sizeof(WATCH_STRUCT_UTMP));
-    while (fread(uptr, sizeof(WATCH_STRUCT_UTMP), 1, in))
+
+    uptr = *head = (WATCH_STRUCT_UTMP *)zalloc(wtabmax * sizeof(WATCH_STRUCT_UTMP));
+	setutent();
+    while ((tmp = getutent()) != NULL)
 # ifdef USER_PROCESS
-	if   (uptr->ut_type == USER_PROCESS)
+	if   (tmp->ut_type == USER_PROCESS)
 # else /* !USER_PROCESS */
-	if   (uptr->ut_name[0])
+	if   (tmp->ut_name[0])
 # endif /* !USER_PROCESS */
 	{
+		memcpy(uptr, tmp, sizeof (WATCH_STRUCT_UTMP));
 	    uptr++;
-	    if (++wtabsz == wtabmax)
-		uptr = (wtab = (WATCH_STRUCT_UTMP *)realloc((void *) wtab, (wtabmax *= 2) *
-						      sizeof(WATCH_STRUCT_UTMP))) + wtabsz;
+	    if (++sz == wtabmax) {
+			uptr = (WATCH_STRUCT_UTMP *)
+				realloc(*head, (wtabmax *= 2) * sizeof(WATCH_STRUCT_UTMP));
+			if (uptr == NULL) {
+				/* memory pressure - so stop consuming and use, what we have
+				 * Other option is to exit() here, as zmalloc does on error */
+				sz--;
+				break;
+			}
+			*head = uptr;
+			uptr += sz;
+		}
 	}
-    fclose(in);
+    endutent();
 
-    if (wtabsz)
-	qsort((void *) wtab, wtabsz, sizeof(WATCH_STRUCT_UTMP),
+    if (sz)
+	qsort((void *) *head, sz, sizeof(WATCH_STRUCT_UTMP),
 	           (int (*) _((const void *, const void *)))ucmp);
+	return sz;
 }
 
 /* Check for login/logout events; executed before *
@@ -486,55 +499,28 @@
 void
 dowatch(void)
 {
-    FILE *in;
     WATCH_STRUCT_UTMP *utab, *uptr, *wptr;
     struct stat st;
     char **s;
     char *fmt;
-    int utabsz = 0, utabmax = wtabsz + 4;
-    int uct, wct;
+    int utabsz, uct, wct;
 
     s = watch;
 
     holdintr();
-    if (!wtab) {
-	readwtab();
-	noholdintr();
-	return;
-    }
+    if (!wtab)
+	wtabsz = readwtab(&wtab, 32);
     if ((stat(WATCH_UTMP_FILE, &st) == -1) || (st.st_mtime <= lastutmpcheck)) {
 	noholdintr();
 	return;
     }
     lastutmpcheck = st.st_mtime;
-    uptr = utab = (WATCH_STRUCT_UTMP *) zalloc(utabmax * sizeof(WATCH_STRUCT_UTMP));
-
-    if (!(in = fopen(WATCH_UTMP_FILE, "r"))) {
-	free(utab);
-	noholdintr();
-	return;
-    }
-    while (fread(uptr, sizeof *uptr, 1, in))
-# ifdef USER_PROCESS
-	if (uptr->ut_type == USER_PROCESS)
-# else /* !USER_PROCESS */
-	if (uptr->ut_name[0])
-# endif /* !USER_PROCESS */
-	{
-	    uptr++;
-	    if (++utabsz == utabmax)
-		uptr = (utab = (WATCH_STRUCT_UTMP *)realloc((void *) utab, (utabmax *= 2) *
-						      sizeof(WATCH_STRUCT_UTMP))) + utabsz;
-	}
-    fclose(in);
+    utabsz = readwtab(&utab, wtabsz + 4);
     noholdintr();
     if (errflag) {
 	free(utab);
 	return;
     }
-    if (utabsz)
-	qsort((void *) utab, utabsz, sizeof(WATCH_STRUCT_UTMP),
-	           (int (*) _((const void *, const void *)))ucmp);
 
     wct = wtabsz;
     uct = utabsz;

Re: fix watch implementation

[Peter Stephenson]

On Thu, 12 Jan 2017 07:46:44 +0100
Jens Elkner <jel+zsh@cs.uni-magdeburg.de> wrote:
> when evaluating zsh as the new default shell for interactive use it was
> discovered, that the 'watch' feature doesn't work on Solaris 11.3 and
> Ubuntu 14.04, 15.04 and probably other modern platforms: 'log' never
> shows anything and logins/logouts never show up.
>
> The attached patch fixes the implementation, has been successfully
> tested on the platforms mentioned above (was developed against 5.0.7,
> which is, what the vendors of the mentioned OS ship, however, someone on
> irc reported, that it applies cleanly to the current master version as
> well).

Thanks, as it's all #ifdef'd for the appropriate cases and we're not
stabilising the shell for a release at the moment I've just tweaked the
formatting in minor ways and committed it.  There may be some people
around who can exercise it, but not many, by the sound of it.

pws

Re: fix watch implementation

[Jens Elkner]

On Thu, Jan 12, 2017 at 10:21:38AM +0000, Peter Stephenson wrote:
> On Thu, 12 Jan 2017 07:46:44 +0100
> Jens Elkner <jel+zsh@cs.uni-magdeburg.de> wrote:
> > when evaluating zsh as the new default shell for interactive use it was
> > discovered, that the 'watch' feature doesn't work on Solaris 11.3 and
> > Ubuntu 14.04, 15.04 and probably other modern platforms: 'log' never
> > shows anything and logins/logouts never show up.
> >
> > The attached patch fixes the implementation, has been successfully
> > tested on the platforms mentioned above (was developed against 5.0.7,
> > which is, what the vendors of the mentioned OS ship, however, someone on
> > irc reported, that it applies cleanly to the current master version as
> > well).
> 
> Thanks, as it's all #ifdef'd for the appropriate cases and we're not
> stabilising the shell for a release at the moment I've just tweaked the
> formatting in minor ways and committed it.  There may be some people
> around who can exercise it, but not many, by the sound of it.

OK, thanx! As long as the 'log' command shows something and this aligns
with the output of the 'last' utility, everything should be fine.

Regards,
jel.
-- 
Otto-von-Guericke University     http://www.cs.uni-magdeburg.de/
Department of Computer Science   Geb. 29 R 027, Universitaetsplatz 2
39106 Magdeburg, Germany         Tel: +49 391 67 52768

Re: fix watch implementation (and Re: UTF-8 locales on BSDs do not support collation correctly)

[Jun T.]

After the commit d3cf881 (worker/40333), watch.c fails to
compile on OpenBSD (worker/40416), simply because
OpenBSD does not have getutent() (nor getutxent()).

We need to use the old way (open/fread/close) to read
the utmp/utmpx file on systems like OpenBSD.


diff --git a/Src/watch.c b/Src/watch.c
index 6103ef1..f033805 100644
--- a/Src/watch.c
+++ b/Src/watch.c
@@ -91,6 +91,9 @@
 #  define setutent setutxent
 #  define getutent getutxent
 #  define endutent endutxent
+#  ifndef HAVE_GETUTENT
+#   define HAVE_GETUTENT
+#  endif
 # endif
 
 /*
@@ -482,21 +485,32 @@ ucmp(WATCH_STRUCT_UTMP *u, WATCH_STRUCT_UTMP *v)
 static int
 readwtab(WATCH_STRUCT_UTMP **head, int initial_sz)
 {
-    WATCH_STRUCT_UTMP *uptr, *tmp;
+    WATCH_STRUCT_UTMP *uptr;
     int wtabmax = initial_sz < 2 ? 32 : initial_sz;
     int sz = 0;
+# ifdef HAVE_GETUTENT
+    WATCH_STRUCT_UTMP *tmp;
+# else
+    FILE *in;
+# endif
 
     uptr = *head = (WATCH_STRUCT_UTMP *)
 	zalloc(wtabmax * sizeof(WATCH_STRUCT_UTMP));
+# ifdef HAVE_GETUTENT
     setutent();
     while ((tmp = getutent()) != NULL) {
+	memcpy(uptr, tmp, sizeof (WATCH_STRUCT_UTMP));
+# else
+    if (!(in = fopen(WATCH_UTMP_FILE, "r")))
+	return 0;
+    while (fread(uptr, sizeof(WATCH_STRUCT_UTMP), 1, in)) {
+# endif
 # ifdef USER_PROCESS
-	if   (tmp->ut_type == USER_PROCESS)
+	if (uptr->ut_type == USER_PROCESS)
 # else /* !USER_PROCESS */
-	if   (tmp->ut_name[0])
+	if (uptr->ut_name[0])
 # endif /* !USER_PROCESS */
 	{
-	    memcpy(uptr, tmp, sizeof (WATCH_STRUCT_UTMP));
 	    uptr++;
 	    if (++sz == wtabmax) {
 		uptr = (WATCH_STRUCT_UTMP *)
@@ -512,7 +526,11 @@ readwtab(WATCH_STRUCT_UTMP **head, int initial_sz)
 	    }
 	}
     }
+# ifdef HAVE_GETUTENT
     endutent();
+# else
+    fclose(in);
+# endif
 
     if (sz)
 	qsort((void *) *head, sz, sizeof(WATCH_STRUCT_UTMP),
diff --git a/configure.ac b/configure.ac
index c6ece67..0551a69 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1325,7 +1325,7 @@ AC_CHECK_FUNCS(strftime strptime mktime timelocal \
 	       cygwin_conv_path \
 	       nanosleep \
 	       srand_deterministic \
-	       setutxent getutxent endutxent)
+	       setutxent getutxent endutxent getutent)
 AC_FUNC_STRCOLL
 
 AH_TEMPLATE([REALPATH_ACCEPTS_NULL],