[PATCH 00/15] pull: write(1) improvements

[PATCH 00/15] pull: write(1) improvements

[Sami Kerola]

Hello,

This patch set was sent earlier, but because the changes were not merged or
rejected.  So here is a resubmission that I further worked with to improve
quality of the changes with hope this time these will get final verdict.
Old attributions are kept, and more can be added to my git assuming feedback
is given.


----------------------------------------------------------------
The following changes since commit f29bc6e1cc4ed9f76bded543c6ab393f674ec3ed:
  lslocks: add --noinaccessible (2016-07-01 14:45:04 +0200)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git write-improvements
for you to fetch changes up to 6d0f3a2c208917eff769340434a5d736d3f8e57c:
  lib: try to find tty in get_terminal_name() (2016-07-03 22:17:43 +0100)
----------------------------------------------------------------

Sami Kerola (15):
  write: remove unused variable
  write: get rid of function prototypes
  write: remove pointless fileno(3) calls
  write: set atime value in term_chk() only when needed
  write: use xstrncpy() from strutils.h
  write: run atexit() checks at the end of execution
  write: add control structure to clarify what is going on
  write: improve function and variable names
  write: remove unnecessary utmp variables
  write: make timestamp to be obviously just a clock time
  write: remove PUTC macro
  write: improve coding style
  write: tell when effective gid and tty path group mismatch
  write: stop removing and adding /dev/ in front of tty string
  lib: try to find tty in get_terminal_name()

 include/ttyutils.h       |   2 +-
 lib/ttyutils.c           |  15 +-
 login-utils/login.c      |   2 +-
 login-utils/su-common.c  |   4 +-
 term-utils/Makemodule.am |   1 +
 term-utils/write.c       | 418 +++++++++++++++++++++++------------------------
 6 files changed, 218 insertions(+), 224 deletions(-)

-- 
2.9.0

[PATCH 01/15] write: remove unused variable

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 7c15231..0afa46a 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -73,8 +73,6 @@ static void __attribute__ ((__noreturn__)) done(int);
 int term_chk(char *, int *, time_t *, int);
 int utmp_chk(char *, char *);
 
-static gid_t root_access;
-
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
 {
 	fputs(USAGE_HEADER, out);
@@ -122,8 +120,6 @@ int main(int argc, char **argv)
 			usage(stderr);
 		}
 
-	root_access = !getegid();
-
 	/* check that sender has write enabled */
 	if (isatty(fileno(stdin)))
 		myttyfd = fileno(stdin);
-- 
2.9.0

[PATCH 02/15] write: get rid of function prototypes

[Sami Kerola]

Marking functions static and writing them in order where functions are
introduced before use is enough.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 273 ++++++++++++++++++++++++++---------------------------
 1 file changed, 132 insertions(+), 141 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 0afa46a..b8bd08e 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -65,14 +65,6 @@
 #include "nls.h"
 #include "xalloc.h"
 
-static void __attribute__ ((__noreturn__)) usage(FILE * out);
-void search_utmp(char *, char *, char *, uid_t);
-void do_write(char *, char *, uid_t);
-void wr_fputs(char *);
-static void __attribute__ ((__noreturn__)) done(int);
-int term_chk(char *, int *, time_t *, int);
-int utmp_chk(char *, char *);
-
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
 {
 	fputs(USAGE_HEADER, out);
@@ -91,104 +83,36 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-int main(int argc, char **argv)
+/*
+ * term_chk - check that a terminal exists, and get the message bit
+ *     and the access time
+ */
+static int term_chk(char *tty, int *msgsokP, time_t * atimeP, int showerror)
 {
-	time_t atime;
-	uid_t myuid;
-	int msgsok = 0, myttyfd, c;
-	char tty[PATH_MAX], *mytty;
-
-	static const struct option longopts[] = {
-		{"version", no_argument, NULL, 'V'},
-		{"help", no_argument, NULL, 'h'},
-		{NULL, 0, NULL, 0}
-	};
-
-	setlocale(LC_ALL, "");
-	bindtextdomain(PACKAGE, LOCALEDIR);
-	textdomain(PACKAGE);
-	atexit(close_stdout);
-
-	while ((c = getopt_long(argc, argv, "Vh", longopts, NULL)) != -1)
-		switch (c) {
-		case 'V':
-			printf(UTIL_LINUX_VERSION);
-			return EXIT_SUCCESS;
-		case 'h':
-			usage(stdout);
-		default:
-			usage(stderr);
-		}
-
-	/* check that sender has write enabled */
-	if (isatty(fileno(stdin)))
-		myttyfd = fileno(stdin);
-	else if (isatty(fileno(stdout)))
-		myttyfd = fileno(stdout);
-	else if (isatty(fileno(stderr)))
-		myttyfd = fileno(stderr);
-	else
-		myttyfd = -1;
-
-	if (myttyfd != -1) {
-		if (!(mytty = ttyname(myttyfd)))
-			errx(EXIT_FAILURE,
-			     _("can't find your tty's name"));
-
-		/*
-		 * We may have /dev/ttyN but also /dev/pts/xx. Below,
-		 * term_chk() will put "/dev/" in front, so remove that
-		 * part.
-		 */
-		if (!strncmp(mytty, "/dev/", 5))
-			mytty += 5;
-		if (term_chk(mytty, &msgsok, &atime, 1))
-			exit(EXIT_FAILURE);
-		if (!msgsok)
-			errx(EXIT_FAILURE,
-			     _("you have write permission turned off"));
-		msgsok = 0;
-	} else
-		mytty = "<no tty>";
-
-	myuid = getuid();
+	struct stat s;
+	char path[PATH_MAX];
 
-	/* check args */
-	switch (argc) {
-	case 2:
-		search_utmp(argv[1], tty, mytty, myuid);
-		do_write(tty, mytty, myuid);
-		break;
-	case 3:
-		if (!strncmp(argv[2], "/dev/", 5))
-			argv[2] += 5;
-		if (utmp_chk(argv[1], argv[2]))
-			errx(EXIT_FAILURE,
-			     _("%s is not logged in on %s"),
-			     argv[1], argv[2]);
-		if (term_chk(argv[2], &msgsok, &atime, 1))
-			exit(EXIT_FAILURE);
-		if (myuid && !msgsok)
-			errx(EXIT_FAILURE,
-			     _("%s has messages disabled on %s"),
-			     argv[1], argv[2]);
-		do_write(argv[2], mytty, myuid);
-		break;
-	default:
-		usage(stderr);
+	if (strlen(tty) + 6 > sizeof(path))
+		return 1;
+	sprintf(path, "/dev/%s", tty);
+	if (stat(path, &s) < 0) {
+		if (showerror)
+			warn("%s", path);
+		return 1;
 	}
-
-	done(0);
-	/* NOTREACHED */
-	return EXIT_FAILURE;
+	if (getuid() == 0)	/* root can always write */
+		*msgsokP = 1;
+	else
+		*msgsokP = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
+	*atimeP = s.st_atime;
+	return 0;
 }
 
-
 /*
  * utmp_chk - checks that the given user is actually logged in on
  *     the given tty
  */
-int utmp_chk(char *user, char *tty)
+static int utmp_chk(char *user, char *tty)
 {
 	struct utmp u;
 	struct utmp *uptr;
@@ -221,7 +145,7 @@ int utmp_chk(char *user, char *tty)
  * Special case for writing to yourself - ignore the terminal you're
  * writing from, unless that's the only terminal with messages enabled.
  */
-void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
+static void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
 {
 	struct utmp u;
 	struct utmp *uptr;
@@ -280,34 +204,39 @@ void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
 }
 
 /*
- * term_chk - check that a terminal exists, and get the message bit
- *     and the access time
+ * done - cleanup and exit
  */
-int term_chk(char *tty, int *msgsokP, time_t * atimeP, int showerror)
+static void __attribute__ ((__noreturn__))
+    done(int dummy __attribute__ ((__unused__)))
 {
-	struct stat s;
-	char path[PATH_MAX];
+	printf("EOF\r\n");
+	_exit(EXIT_SUCCESS);
+}
 
-	if (strlen(tty) + 6 > sizeof(path))
-		return 1;
-	sprintf(path, "/dev/%s", tty);
-	if (stat(path, &s) < 0) {
-		if (showerror)
-			warn("%s", path);
-		return 1;
+/*
+ * wr_fputs - like fputs(), but makes control characters visible and
+ *     turns \n into \r\n.
+ */
+static void wr_fputs(char *s)
+{
+	char c;
+
+#define	PUTC(c)	if (fputc_careful(c, stdout, '^') == EOF) \
+    err(EXIT_FAILURE, _("carefulputc failed"));
+	while (*s) {
+		c = *s++;
+		if (c == '\n')
+			PUTC('\r');
+		PUTC(c);
 	}
-	if (getuid() == 0)	/* root can always write */
-		*msgsokP = 1;
-	else
-		*msgsokP = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
-	*atimeP = s.st_atime;
-	return 0;
+	return;
+#undef PUTC
 }
 
 /*
  * do_write - actually make the connection
  */
-void do_write(char *tty, char *mytty, uid_t myuid)
+static void do_write(char *tty, char *mytty, uid_t myuid)
 {
 	char *login, *pwuid, *nows;
 	struct passwd *pwd;
@@ -353,32 +282,94 @@ void do_write(char *tty, char *mytty, uid_t myuid)
 		wr_fputs(line);
 }
 
-/*
- * done - cleanup and exit
- */
-static void __attribute__ ((__noreturn__))
-    done(int dummy __attribute__ ((__unused__)))
+int main(int argc, char **argv)
 {
-	printf("EOF\r\n");
-	_exit(EXIT_SUCCESS);
-}
+	time_t atime;
+	uid_t myuid;
+	int msgsok = 0, myttyfd, c;
+	char tty[PATH_MAX], *mytty;
 
-/*
- * wr_fputs - like fputs(), but makes control characters visible and
- *     turns \n into \r\n.
- */
-void wr_fputs(char *s)
-{
-	char c;
+	static const struct option longopts[] = {
+		{"version", no_argument, NULL, 'V'},
+		{"help", no_argument, NULL, 'h'},
+		{NULL, 0, NULL, 0}
+	};
 
-#define	PUTC(c)	if (fputc_careful(c, stdout, '^') == EOF) \
-    err(EXIT_FAILURE, _("carefulputc failed"));
-	while (*s) {
-		c = *s++;
-		if (c == '\n')
-			PUTC('\r');
-		PUTC(c);
+	setlocale(LC_ALL, "");
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
+	atexit(close_stdout);
+
+	while ((c = getopt_long(argc, argv, "Vh", longopts, NULL)) != -1)
+		switch (c) {
+		case 'V':
+			printf(UTIL_LINUX_VERSION);
+			return EXIT_SUCCESS;
+		case 'h':
+			usage(stdout);
+		default:
+			usage(stderr);
+		}
+
+	/* check that sender has write enabled */
+	if (isatty(fileno(stdin)))
+		myttyfd = fileno(stdin);
+	else if (isatty(fileno(stdout)))
+		myttyfd = fileno(stdout);
+	else if (isatty(fileno(stderr)))
+		myttyfd = fileno(stderr);
+	else
+		myttyfd = -1;
+
+	if (myttyfd != -1) {
+		if (!(mytty = ttyname(myttyfd)))
+			errx(EXIT_FAILURE,
+			     _("can't find your tty's name"));
+
+		/*
+		 * We may have /dev/ttyN but also /dev/pts/xx. Below,
+		 * term_chk() will put "/dev/" in front, so remove that
+		 * part.
+		 */
+		if (!strncmp(mytty, "/dev/", 5))
+			mytty += 5;
+		if (term_chk(mytty, &msgsok, &atime, 1))
+			exit(EXIT_FAILURE);
+		if (!msgsok)
+			errx(EXIT_FAILURE,
+			     _("you have write permission turned off"));
+		msgsok = 0;
+	} else
+		mytty = "<no tty>";
+
+	myuid = getuid();
+
+	/* check args */
+	switch (argc) {
+	case 2:
+		search_utmp(argv[1], tty, mytty, myuid);
+		do_write(tty, mytty, myuid);
+		break;
+	case 3:
+		if (!strncmp(argv[2], "/dev/", 5))
+			argv[2] += 5;
+		if (utmp_chk(argv[1], argv[2]))
+			errx(EXIT_FAILURE,
+			     _("%s is not logged in on %s"),
+			     argv[1], argv[2]);
+		if (term_chk(argv[2], &msgsok, &atime, 1))
+			exit(EXIT_FAILURE);
+		if (myuid && !msgsok)
+			errx(EXIT_FAILURE,
+			     _("%s has messages disabled on %s"),
+			     argv[1], argv[2]);
+		do_write(argv[2], mytty, myuid);
+		break;
+	default:
+		usage(stderr);
 	}
-	return;
-#undef PUTC
+
+	done(0);
+	/* NOTREACHED */
+	return EXIT_FAILURE;
 }
-- 
2.9.0

[PATCH 03/15] write: remove pointless fileno(3) calls

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index b8bd08e..03af660 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -312,12 +312,12 @@ int main(int argc, char **argv)
 		}
 
 	/* check that sender has write enabled */
-	if (isatty(fileno(stdin)))
-		myttyfd = fileno(stdin);
-	else if (isatty(fileno(stdout)))
-		myttyfd = fileno(stdout);
-	else if (isatty(fileno(stderr)))
-		myttyfd = fileno(stderr);
+	if (isatty(STDIN_FILENO))
+		myttyfd = STDIN_FILENO;
+	else if (isatty(STDOUT_FILENO))
+		myttyfd = STDOUT_FILENO;
+	else if (isatty(STDERR_FILENO))
+		myttyfd = STDERR_FILENO;
 	else
 		myttyfd = -1;
 
-- 
2.9.0

[PATCH 04/15] write: set atime value in term_chk() only when needed

[Sami Kerola]

The search_utmp() is needs atime but main() does not, so remove the later.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 03af660..fb9cdf5 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -104,7 +104,8 @@ static int term_chk(char *tty, int *msgsokP, time_t * atimeP, int showerror)
 		*msgsokP = 1;
 	else
 		*msgsokP = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
-	*atimeP = s.st_atime;
+	if (atimeP)
+		*atimeP = s.st_atime;
 	return 0;
 }
 
@@ -284,7 +285,6 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 
 int main(int argc, char **argv)
 {
-	time_t atime;
 	uid_t myuid;
 	int msgsok = 0, myttyfd, c;
 	char tty[PATH_MAX], *mytty;
@@ -333,7 +333,7 @@ int main(int argc, char **argv)
 		 */
 		if (!strncmp(mytty, "/dev/", 5))
 			mytty += 5;
-		if (term_chk(mytty, &msgsok, &atime, 1))
+		if (term_chk(mytty, &msgsok, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (!msgsok)
 			errx(EXIT_FAILURE,
@@ -357,7 +357,7 @@ int main(int argc, char **argv)
 			errx(EXIT_FAILURE,
 			     _("%s is not logged in on %s"),
 			     argv[1], argv[2]);
-		if (term_chk(argv[2], &msgsok, &atime, 1))
+		if (term_chk(argv[2], &msgsok, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (myuid && !msgsok)
 			errx(EXIT_FAILURE,
-- 
2.9.0

[PATCH 05/15] write: use xstrncpy() from strutils.h

[Sami Kerola]

Earlier if the tty path was exactly length of the maximum ut_line then last
character of the path was overwrote by \0.  This is in practise theoretical
bug, as it is unheard that a tty device path could be 32 characters long.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index fb9cdf5..11a6580 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -63,6 +63,7 @@
 #include "carefulputc.h"
 #include "closestream.h"
 #include "nls.h"
+#include "strutils.h"
 #include "xalloc.h"
 
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
@@ -164,8 +165,7 @@ static void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
 		memcpy(&u, uptr, sizeof(u));
 		if (strncmp(user, u.ut_user, sizeof(u.ut_user)) == 0) {
 			++nloggedttys;
-			strncpy(atty, u.ut_line, sizeof(u.ut_line));
-			atty[sizeof(u.ut_line)] = '\0';
+			xstrncpy(atty, u.ut_line, sizeof(atty));
 			if (term_chk(atty, &msgsok, &atime, 0))
 				/* bad term? skip */
 				continue;
-- 
2.9.0

[PATCH 06/15] write: run atexit() checks at the end of execution

[Sami Kerola]

Earlier use of _exit() caused program to terminate abnormally from atexit()
perspective and standard file descriptor close checks did not run resulting
to blindness if writes were successful, or not.  Easy fix is to avoid
running _exit() altogether.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 11a6580..568892d 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -66,6 +66,8 @@
 #include "strutils.h"
 #include "xalloc.h"
 
+static sig_atomic_t signal_received = 0;
+
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
 {
 	fputs(USAGE_HEADER, out);
@@ -205,13 +207,11 @@ static void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
 }
 
 /*
- * done - cleanup and exit
+ * signal_handler - cause write loop to exit
  */
-static void __attribute__ ((__noreturn__))
-    done(int dummy __attribute__ ((__unused__)))
+static void signal_handler(int signo)
 {
-	printf("EOF\r\n");
-	_exit(EXIT_SUCCESS);
+	signal_received = signo;
 }
 
 /*
@@ -243,6 +243,7 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 	struct passwd *pwd;
 	time_t now;
 	char path[PATH_MAX], *host, line[512];
+	struct sigaction sigact;
 
 	/* Determine our login name(s) before the we reopen() stdout */
 	if ((pwd = getpwuid(myuid)) != NULL)
@@ -258,8 +259,11 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 	if ((freopen(path, "w", stdout)) == NULL)
 		err(EXIT_FAILURE, "%s", path);
 
-	signal(SIGINT, done);
-	signal(SIGHUP, done);
+	sigact.sa_handler = signal_handler;
+	sigemptyset(&sigact.sa_mask);
+	sigact.sa_flags = 0;
+	sigaction(SIGINT, &sigact, NULL);
+	sigaction(SIGHUP, &sigact, NULL);
 
 	/* print greeting */
 	host = xgethostname();
@@ -279,8 +283,12 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 	free(host);
 	printf("\r\n");
 
-	while (fgets(line, sizeof(line), stdin) != NULL)
+	while (fgets(line, sizeof(line), stdin) != NULL) {
+		if (signal_received)
+			break;
 		wr_fputs(line);
+	}
+	printf("EOF\r\n");
 }
 
 int main(int argc, char **argv)
@@ -368,8 +376,5 @@ int main(int argc, char **argv)
 	default:
 		usage(stderr);
 	}
-
-	done(0);
-	/* NOTREACHED */
-	return EXIT_FAILURE;
+	return EXIT_SUCCESS;
 }
-- 
2.9.0

[PATCH 07/15] write: add control structure to clarify what is going on

[Sami Kerola]

This is done purely an improve readability of the source code.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 96 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 54 insertions(+), 42 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 568892d..2557f33 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -68,6 +68,14 @@
 
 static sig_atomic_t signal_received = 0;
 
+struct write_control {
+	uid_t src_uid;
+	const char *src_login;
+	char *src_tty;
+	const char *dst_login;
+	char dst_tty[PATH_MAX];
+};
+
 static void __attribute__ ((__noreturn__)) usage(FILE * out)
 {
 	fputs(USAGE_HEADER, out);
@@ -116,7 +124,7 @@ static int term_chk(char *tty, int *msgsokP, time_t * atimeP, int showerror)
  * utmp_chk - checks that the given user is actually logged in on
  *     the given tty
  */
-static int utmp_chk(char *user, char *tty)
+static int utmp_chk(const struct write_control *ctl)
 {
 	struct utmp u;
 	struct utmp *uptr;
@@ -127,8 +135,8 @@ static int utmp_chk(char *user, char *tty)
 
 	while ((uptr = getutent())) {
 		memcpy(&u, uptr, sizeof(u));
-		if (strncmp(user, u.ut_user, sizeof(u.ut_user)) == 0 &&
-		    strncmp(tty, u.ut_line, sizeof(u.ut_line)) == 0) {
+		if (strncmp(ctl->dst_login, u.ut_user, sizeof(u.ut_user)) == 0 &&
+		    strncmp(ctl->dst_tty, u.ut_line, sizeof(u.ut_line)) == 0) {
 			res = 0;
 			break;
 		}
@@ -149,7 +157,7 @@ static int utmp_chk(char *user, char *tty)
  * Special case for writing to yourself - ignore the terminal you're
  * writing from, unless that's the only terminal with messages enabled.
  */
-static void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
+static void search_utmp(struct write_control *ctl)
 {
 	struct utmp u;
 	struct utmp *uptr;
@@ -165,16 +173,16 @@ static void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
 	user_is_me = 0;
 	while ((uptr = getutent())) {
 		memcpy(&u, uptr, sizeof(u));
-		if (strncmp(user, u.ut_user, sizeof(u.ut_user)) == 0) {
+		if (strncmp(ctl->dst_login, u.ut_user, sizeof(u.ut_user)) == 0) {
 			++nloggedttys;
 			xstrncpy(atty, u.ut_line, sizeof(atty));
 			if (term_chk(atty, &msgsok, &atime, 0))
 				/* bad term? skip */
 				continue;
-			if (myuid && !msgsok)
+			if (ctl->src_uid && !msgsok)
 				/* skip ttys with msgs off */
 				continue;
-			if (strcmp(atty, mytty) == 0) {
+			if (strcmp(atty, ctl->src_tty) == 0) {
 				user_is_me = 1;
 				/* don't write to yourself */
 				continue;
@@ -185,25 +193,26 @@ static void search_utmp(char *user, char *tty, char *mytty, uid_t myuid)
 			++nttys;
 			if (atime > bestatime) {
 				bestatime = atime;
-				strcpy(tty, atty);
+				xstrncpy(ctl->dst_tty, atty, sizeof(ctl->dst_tty));
 			}
 		}
 	}
 
 	endutent();
 	if (nloggedttys == 0)
-		errx(EXIT_FAILURE, _("%s is not logged in"), user);
+		errx(EXIT_FAILURE, _("%s is not logged in"), ctl->dst_login);
 	if (nttys == 0) {
 		if (user_is_me) {
 			/* ok, so write to yourself! */
-			strcpy(tty, mytty);
+			xstrncpy(ctl->dst_tty, ctl->src_tty, sizeof(ctl->dst_tty));
 			return;
 		}
-		errx(EXIT_FAILURE, _("%s has messages disabled"), user);
+		errx(EXIT_FAILURE, _("%s has messages disabled"), ctl->dst_login);
 	} else if (nttys > 1) {
 		warnx(_("%s is logged in more than once; writing to %s"),
-		      user, tty);
+		      ctl->dst_login, ctl->dst_tty);
 	}
+
 }
 
 /*
@@ -237,7 +246,7 @@ static void wr_fputs(char *s)
 /*
  * do_write - actually make the connection
  */
-static void do_write(char *tty, char *mytty, uid_t myuid)
+static void do_write(const struct write_control *ctl)
 {
 	char *login, *pwuid, *nows;
 	struct passwd *pwd;
@@ -246,16 +255,16 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 	struct sigaction sigact;
 
 	/* Determine our login name(s) before the we reopen() stdout */
-	if ((pwd = getpwuid(myuid)) != NULL)
+	if ((pwd = getpwuid(ctl->src_uid)) != NULL)
 		pwuid = pwd->pw_name;
 	else
 		pwuid = "???";
 	if ((login = getlogin()) == NULL)
 		login = pwuid;
 
-	if (strlen(tty) + 6 > sizeof(path))
-		errx(EXIT_FAILURE, _("tty path %s too long"), tty);
-	snprintf(path, sizeof(path), "/dev/%s", tty);
+	if (strlen(ctl->dst_tty) + 6 > sizeof(path))
+		errx(EXIT_FAILURE, _("tty path %s too long"), ctl->dst_tty);
+	snprintf(path, sizeof(path), "/dev/%s", ctl->dst_tty);
 	if ((freopen(path, "w", stdout)) == NULL)
 		err(EXIT_FAILURE, "%s", path);
 
@@ -276,10 +285,10 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 	printf("\r\n\007\007\007");
 	if (strcmp(login, pwuid))
 		printf(_("Message from %s@%s (as %s) on %s at %s ..."),
-		       login, host, pwuid, mytty, nows + 11);
+		       login, host, pwuid, ctl->src_tty, nows + 11);
 	else
 		printf(_("Message from %s@%s on %s at %s ..."),
-		       login, host, mytty, nows + 11);
+		       login, host, ctl->src_tty, nows + 11);
 	free(host);
 	printf("\r\n");
 
@@ -293,9 +302,8 @@ static void do_write(char *tty, char *mytty, uid_t myuid)
 
 int main(int argc, char **argv)
 {
-	uid_t myuid;
-	int msgsok = 0, myttyfd, c;
-	char tty[PATH_MAX], *mytty;
+	int msgsok = 0, src_fd, c;
+	struct write_control ctl = { 0 };
 
 	static const struct option longopts[] = {
 		{"version", no_argument, NULL, 'V'},
@@ -321,16 +329,16 @@ int main(int argc, char **argv)
 
 	/* check that sender has write enabled */
 	if (isatty(STDIN_FILENO))
-		myttyfd = STDIN_FILENO;
+		src_fd = STDIN_FILENO;
 	else if (isatty(STDOUT_FILENO))
-		myttyfd = STDOUT_FILENO;
+		src_fd = STDOUT_FILENO;
 	else if (isatty(STDERR_FILENO))
-		myttyfd = STDERR_FILENO;
+		src_fd = STDERR_FILENO;
 	else
-		myttyfd = -1;
+		src_fd = -1;
 
-	if (myttyfd != -1) {
-		if (!(mytty = ttyname(myttyfd)))
+	if (src_fd != -1) {
+		if (!(ctl.src_tty = ttyname(src_fd)))
 			errx(EXIT_FAILURE,
 			     _("can't find your tty's name"));
 
@@ -339,39 +347,43 @@ int main(int argc, char **argv)
 		 * term_chk() will put "/dev/" in front, so remove that
 		 * part.
 		 */
-		if (!strncmp(mytty, "/dev/", 5))
-			mytty += 5;
-		if (term_chk(mytty, &msgsok, NULL, 1))
+		if (!strncmp(ctl.src_tty, "/dev/", 5))
+			ctl.src_tty += 5;
+		if (term_chk(ctl.src_tty, &msgsok, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (!msgsok)
 			errx(EXIT_FAILURE,
 			     _("you have write permission turned off"));
 		msgsok = 0;
 	} else
-		mytty = "<no tty>";
+		ctl.src_tty = "<no tty>";
 
-	myuid = getuid();
+	ctl.src_uid = getuid();
 
 	/* check args */
 	switch (argc) {
 	case 2:
-		search_utmp(argv[1], tty, mytty, myuid);
-		do_write(tty, mytty, myuid);
+		ctl.dst_login = argv[1];
+		search_utmp(&ctl);
+		do_write(&ctl);
 		break;
 	case 3:
+		ctl.dst_login = argv[1];
 		if (!strncmp(argv[2], "/dev/", 5))
-			argv[2] += 5;
-		if (utmp_chk(argv[1], argv[2]))
+			xstrncpy(ctl.dst_tty, argv[2] + 5, sizeof(ctl.dst_tty));
+		else
+			xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
+		if (utmp_chk(&ctl))
 			errx(EXIT_FAILURE,
 			     _("%s is not logged in on %s"),
-			     argv[1], argv[2]);
-		if (term_chk(argv[2], &msgsok, NULL, 1))
+			     ctl.dst_login, ctl.dst_tty);
+		if (term_chk(ctl.dst_tty, &msgsok, NULL, 1))
 			exit(EXIT_FAILURE);
-		if (myuid && !msgsok)
+		if (ctl.src_uid && !msgsok)
 			errx(EXIT_FAILURE,
 			     _("%s has messages disabled on %s"),
-			     argv[1], argv[2]);
-		do_write(argv[2], mytty, myuid);
+			     ctl.dst_login, ctl.dst_tty);
+		do_write(&ctl);
 		break;
 	default:
 		usage(stderr);
-- 
2.9.0

[PATCH 08/15] write: improve function and variable names

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 73 ++++++++++++++++++++++++++----------------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 2557f33..6bf15dd 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -95,10 +95,10 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 }
 
 /*
- * term_chk - check that a terminal exists, and get the message bit
+ * check_tty - check that a terminal exists, and get the message bit
  *     and the access time
  */
-static int term_chk(char *tty, int *msgsokP, time_t * atimeP, int showerror)
+static int check_tty(char *tty, int *tty_writeable, time_t *tty_atime, int showerror)
 {
 	struct stat s;
 	char path[PATH_MAX];
@@ -112,19 +112,19 @@ static int term_chk(char *tty, int *msgsokP, time_t * atimeP, int showerror)
 		return 1;
 	}
 	if (getuid() == 0)	/* root can always write */
-		*msgsokP = 1;
+		*tty_writeable = 1;
 	else
-		*msgsokP = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
-	if (atimeP)
-		*atimeP = s.st_atime;
+		*tty_writeable = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
+	if (tty_atime)
+		*tty_atime = s.st_atime;
 	return 0;
 }
 
 /*
- * utmp_chk - checks that the given user is actually logged in on
+ * check_utmp - checks that the given user is actually logged in on
  *     the given tty
  */
-static int utmp_chk(const struct write_control *ctl)
+static int check_utmp(const struct write_control *ctl)
 {
 	struct utmp u;
 	struct utmp *uptr;
@@ -161,25 +161,22 @@ static void search_utmp(struct write_control *ctl)
 {
 	struct utmp u;
 	struct utmp *uptr;
-	time_t bestatime, atime;
-	int nloggedttys, nttys, msgsok = 0, user_is_me;
+	time_t best_atime = 0, tty_atime;
+	int num_ttys = 0, valid_ttys = 0, tty_writeable = 0, user_is_me = 0;
 	char atty[sizeof(u.ut_line) + 1];
 
 	utmpname(_PATH_UTMP);
 	setutent();
 
-	nloggedttys = nttys = 0;
-	bestatime = 0;
-	user_is_me = 0;
 	while ((uptr = getutent())) {
 		memcpy(&u, uptr, sizeof(u));
 		if (strncmp(ctl->dst_login, u.ut_user, sizeof(u.ut_user)) == 0) {
-			++nloggedttys;
+			num_ttys++;
 			xstrncpy(atty, u.ut_line, sizeof(atty));
-			if (term_chk(atty, &msgsok, &atime, 0))
+			if (check_tty(atty, &tty_writeable, &tty_atime, 0))
 				/* bad term? skip */
 				continue;
-			if (ctl->src_uid && !msgsok)
+			if (ctl->src_uid && !tty_writeable)
 				/* skip ttys with msgs off */
 				continue;
 			if (strcmp(atty, ctl->src_tty) == 0) {
@@ -190,25 +187,25 @@ static void search_utmp(struct write_control *ctl)
 			if (u.ut_type != USER_PROCESS)
 				/* it's not a valid entry */
 				continue;
-			++nttys;
-			if (atime > bestatime) {
-				bestatime = atime;
+			valid_ttys++;
+			if (tty_atime > best_atime) {
+				best_atime = tty_atime;
 				xstrncpy(ctl->dst_tty, atty, sizeof(ctl->dst_tty));
 			}
 		}
 	}
 
 	endutent();
-	if (nloggedttys == 0)
+	if (num_ttys == 0)
 		errx(EXIT_FAILURE, _("%s is not logged in"), ctl->dst_login);
-	if (nttys == 0) {
+	if (valid_ttys == 0) {
 		if (user_is_me) {
 			/* ok, so write to yourself! */
 			xstrncpy(ctl->dst_tty, ctl->src_tty, sizeof(ctl->dst_tty));
 			return;
 		}
 		errx(EXIT_FAILURE, _("%s has messages disabled"), ctl->dst_login);
-	} else if (nttys > 1) {
+	} else if (valid_ttys > 1) {
 		warnx(_("%s is logged in more than once; writing to %s"),
 		      ctl->dst_login, ctl->dst_tty);
 	}
@@ -224,10 +221,10 @@ static void signal_handler(int signo)
 }
 
 /*
- * wr_fputs - like fputs(), but makes control characters visible and
+ * write_line - like fputs(), but makes control characters visible and
  *     turns \n into \r\n.
  */
-static void wr_fputs(char *s)
+static void write_line(char *s)
 {
 	char c;
 
@@ -248,7 +245,7 @@ static void wr_fputs(char *s)
  */
 static void do_write(const struct write_control *ctl)
 {
-	char *login, *pwuid, *nows;
+	char *login, *pwuid, *time_stamp;
 	struct passwd *pwd;
 	time_t now;
 	char path[PATH_MAX], *host, line[512];
@@ -280,29 +277,29 @@ static void do_write(const struct write_control *ctl)
 		host = xstrdup("???");
 
 	now = time((time_t *) NULL);
-	nows = ctime(&now);
-	nows[16] = '\0';
+	time_stamp = ctime(&now);
+	time_stamp[16] = '\0';
 	printf("\r\n\007\007\007");
 	if (strcmp(login, pwuid))
 		printf(_("Message from %s@%s (as %s) on %s at %s ..."),
-		       login, host, pwuid, ctl->src_tty, nows + 11);
+		       login, host, pwuid, ctl->src_tty, time_stamp + 11);
 	else
 		printf(_("Message from %s@%s on %s at %s ..."),
-		       login, host, ctl->src_tty, nows + 11);
+		       login, host, ctl->src_tty, time_stamp + 11);
 	free(host);
 	printf("\r\n");
 
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		if (signal_received)
 			break;
-		wr_fputs(line);
+		write_line(line);
 	}
 	printf("EOF\r\n");
 }
 
 int main(int argc, char **argv)
 {
-	int msgsok = 0, src_fd, c;
+	int tty_writeable = 0, src_fd, c;
 	struct write_control ctl = { 0 };
 
 	static const struct option longopts[] = {
@@ -344,17 +341,17 @@ int main(int argc, char **argv)
 
 		/*
 		 * We may have /dev/ttyN but also /dev/pts/xx. Below,
-		 * term_chk() will put "/dev/" in front, so remove that
+		 * check_tty() will put "/dev/" in front, so remove that
 		 * part.
 		 */
 		if (!strncmp(ctl.src_tty, "/dev/", 5))
 			ctl.src_tty += 5;
-		if (term_chk(ctl.src_tty, &msgsok, NULL, 1))
+		if (check_tty(ctl.src_tty, &tty_writeable, NULL, 1))
 			exit(EXIT_FAILURE);
-		if (!msgsok)
+		if (!tty_writeable)
 			errx(EXIT_FAILURE,
 			     _("you have write permission turned off"));
-		msgsok = 0;
+		tty_writeable = 0;
 	} else
 		ctl.src_tty = "<no tty>";
 
@@ -373,13 +370,13 @@ int main(int argc, char **argv)
 			xstrncpy(ctl.dst_tty, argv[2] + 5, sizeof(ctl.dst_tty));
 		else
 			xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
-		if (utmp_chk(&ctl))
+		if (check_utmp(&ctl))
 			errx(EXIT_FAILURE,
 			     _("%s is not logged in on %s"),
 			     ctl.dst_login, ctl.dst_tty);
-		if (term_chk(ctl.dst_tty, &msgsok, NULL, 1))
+		if (check_tty(ctl.dst_tty, &tty_writeable, NULL, 1))
 			exit(EXIT_FAILURE);
-		if (ctl.src_uid && !msgsok)
+		if (ctl.src_uid && !tty_writeable)
 			errx(EXIT_FAILURE,
 			     _("%s has messages disabled on %s"),
 			     ctl.dst_login, ctl.dst_tty);
-- 
2.9.0

[PATCH 09/15] write: remove unnecessary utmp variables

[Sami Kerola]

glibc documentation tells getutent() calls are not thread safe, and
recommends to copy the context of the structures when information is wished
to be stored.  This leads to excessive copying, that in this case is not
relevant.  write(1) is single threaded program and there is no reason to
assume this would change in future.

Reference: http://www.gnu.org/software/libc/manual/html_node/Manipulating-the-Database.html
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 6bf15dd..8288e9d 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -126,17 +126,15 @@ static int check_tty(char *tty, int *tty_writeable, time_t *tty_atime, int showe
  */
 static int check_utmp(const struct write_control *ctl)
 {
-	struct utmp u;
-	struct utmp *uptr;
+	struct utmp *u;
 	int res = 1;
 
 	utmpname(_PATH_UTMP);
 	setutent();
 
-	while ((uptr = getutent())) {
-		memcpy(&u, uptr, sizeof(u));
-		if (strncmp(ctl->dst_login, u.ut_user, sizeof(u.ut_user)) == 0 &&
-		    strncmp(ctl->dst_tty, u.ut_line, sizeof(u.ut_line)) == 0) {
+	while ((u = getutent())) {
+		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) == 0 &&
+		    strncmp(ctl->dst_tty, u->ut_line, sizeof(u->ut_line)) == 0) {
 			res = 0;
 			break;
 		}
@@ -159,38 +157,34 @@ static int check_utmp(const struct write_control *ctl)
  */
 static void search_utmp(struct write_control *ctl)
 {
-	struct utmp u;
-	struct utmp *uptr;
+	struct utmp *u;
 	time_t best_atime = 0, tty_atime;
 	int num_ttys = 0, valid_ttys = 0, tty_writeable = 0, user_is_me = 0;
-	char atty[sizeof(u.ut_line) + 1];
 
 	utmpname(_PATH_UTMP);
 	setutent();
 
-	while ((uptr = getutent())) {
-		memcpy(&u, uptr, sizeof(u));
-		if (strncmp(ctl->dst_login, u.ut_user, sizeof(u.ut_user)) == 0) {
+	while ((u = getutent())) {
+		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) == 0) {
 			num_ttys++;
-			xstrncpy(atty, u.ut_line, sizeof(atty));
-			if (check_tty(atty, &tty_writeable, &tty_atime, 0))
+			if (check_tty(u->ut_line, &tty_writeable, &tty_atime, 0))
 				/* bad term? skip */
 				continue;
 			if (ctl->src_uid && !tty_writeable)
 				/* skip ttys with msgs off */
 				continue;
-			if (strcmp(atty, ctl->src_tty) == 0) {
+			if (strcmp(u->ut_line, ctl->src_tty) == 0) {
 				user_is_me = 1;
 				/* don't write to yourself */
 				continue;
 			}
-			if (u.ut_type != USER_PROCESS)
+			if (u->ut_type != USER_PROCESS)
 				/* it's not a valid entry */
 				continue;
 			valid_ttys++;
 			if (tty_atime > best_atime) {
 				best_atime = tty_atime;
-				xstrncpy(ctl->dst_tty, atty, sizeof(ctl->dst_tty));
+				xstrncpy(ctl->dst_tty, u->ut_line, sizeof(ctl->dst_tty));
 			}
 		}
 	}
-- 
2.9.0

[PATCH 10/15] write: make timestamp to be obviously just a clock time

[Sami Kerola]

By looking the code one will had hard time knowing that a slice of ctime()
from characters 11 to 16 is HH:MM time format.  Use of strftime("%H:%M")
makes this a lot less mysterious.

In same go make \007 hex printouts to be \a that is the same thing: alarm.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 8288e9d..696a7a7 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -239,9 +239,10 @@ static void write_line(char *s)
  */
 static void do_write(const struct write_control *ctl)
 {
-	char *login, *pwuid, *time_stamp;
+	char *login, *pwuid, timestamp[6];
 	struct passwd *pwd;
 	time_t now;
+	struct tm *tm;
 	char path[PATH_MAX], *host, line[512];
 	struct sigaction sigact;
 
@@ -265,21 +266,21 @@ static void do_write(const struct write_control *ctl)
 	sigaction(SIGINT, &sigact, NULL);
 	sigaction(SIGHUP, &sigact, NULL);
 
-	/* print greeting */
 	host = xgethostname();
 	if (!host)
 		host = xstrdup("???");
 
-	now = time((time_t *) NULL);
-	time_stamp = ctime(&now);
-	time_stamp[16] = '\0';
-	printf("\r\n\007\007\007");
+	now = time((time_t *)NULL);
+	tm = localtime(&now);
+	strftime(timestamp, sizeof(timestamp), "%H:%M", tm);
+	/* print greeting */
+	printf("\r\n\a\a\a");
 	if (strcmp(login, pwuid))
 		printf(_("Message from %s@%s (as %s) on %s at %s ..."),
-		       login, host, pwuid, ctl->src_tty, time_stamp + 11);
+		       login, host, pwuid, ctl->src_tty, timestamp);
 	else
 		printf(_("Message from %s@%s on %s at %s ..."),
-		       login, host, ctl->src_tty, time_stamp + 11);
+		       login, host, ctl->src_tty, timestamp);
 	free(host);
 	printf("\r\n");
 
-- 
2.9.0

[PATCH 11/15] write: remove PUTC macro

[Sami Kerola]

Function like macros make following the execution flow unnecessarily
difficult, and deserves to be removed.

Requested-by: Karel Zak <kzak@redhat.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 696a7a7..517bd88 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -220,18 +220,15 @@ static void signal_handler(int signo)
  */
 static void write_line(char *s)
 {
-	char c;
-
-#define	PUTC(c)	if (fputc_careful(c, stdout, '^') == EOF) \
-    err(EXIT_FAILURE, _("carefulputc failed"));
 	while (*s) {
-		c = *s++;
-		if (c == '\n')
-			PUTC('\r');
-		PUTC(c);
+		const int c = *s++;
+
+		if (c == '\n' && fputc_careful('\r', stdout, '^') == EOF)
+			goto fail;
+		if (fputc_careful(c, stdout, '^') == EOF)
+ fail:
+			err(EXIT_FAILURE, _("carefulputc failed"));
 	}
-	return;
-#undef PUTC
 }
 
 /*
-- 
2.9.0

[PATCH 12/15] write: improve coding style

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 71 +++++++++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 517bd88..4300489 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -45,19 +45,19 @@
  *
  */
 
-#include <stdio.h>
-#include <unistd.h>
-#include <utmp.h>
 #include <errno.h>
-#include <time.h>
+#include <getopt.h>
+#include <paths.h>
 #include <pwd.h>
-#include <string.h>
-#include <stdlib.h>
 #include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
 #include <sys/param.h>
 #include <sys/stat.h>
-#include <paths.h>
-#include <getopt.h>
+#include <time.h>
+#include <unistd.h>
+#include <utmp.h>
 
 #include "c.h"
 #include "carefulputc.h"
@@ -76,7 +76,7 @@ struct write_control {
 	char dst_tty[PATH_MAX];
 };
 
-static void __attribute__ ((__noreturn__)) usage(FILE * out)
+static void __attribute__((__noreturn__)) usage(FILE *out)
 {
 	fputs(USAGE_HEADER, out);
 	fprintf(out,
@@ -103,7 +103,7 @@ static int check_tty(char *tty, int *tty_writeable, time_t *tty_atime, int showe
 	struct stat s;
 	char path[PATH_MAX];
 
-	if (strlen(tty) + 6 > sizeof(path))
+	if (sizeof(path) < strlen(tty) + 6)
 		return 1;
 	sprintf(path, "/dev/%s", tty);
 	if (stat(path, &s) < 0) {
@@ -165,27 +165,28 @@ static void search_utmp(struct write_control *ctl)
 	setutent();
 
 	while ((u = getutent())) {
-		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) == 0) {
-			num_ttys++;
-			if (check_tty(u->ut_line, &tty_writeable, &tty_atime, 0))
-				/* bad term? skip */
-				continue;
-			if (ctl->src_uid && !tty_writeable)
-				/* skip ttys with msgs off */
-				continue;
-			if (strcmp(u->ut_line, ctl->src_tty) == 0) {
-				user_is_me = 1;
-				/* don't write to yourself */
-				continue;
-			}
-			if (u->ut_type != USER_PROCESS)
-				/* it's not a valid entry */
-				continue;
-			valid_ttys++;
-			if (tty_atime > best_atime) {
-				best_atime = tty_atime;
-				xstrncpy(ctl->dst_tty, u->ut_line, sizeof(ctl->dst_tty));
-			}
+		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) != 0)
+			continue;
+		num_ttys++;
+		if (check_tty(u->ut_line, &tty_writeable, &tty_atime, 0))
+			/* bad term? skip */
+			continue;
+		if (ctl->src_uid && !tty_writeable)
+			/* skip ttys with msgs off */
+			continue;
+		if (strcmp(u->ut_line, ctl->src_tty) == 0) {
+			user_is_me = 1;
+			/* don't write to yourself */
+			continue;
+		}
+		if (u->ut_type != USER_PROCESS)
+			/* it's not a valid entry */
+			continue;
+		valid_ttys++;
+		if (best_atime < tty_atime) {
+			best_atime = tty_atime;
+			xstrncpy(ctl->dst_tty, u->ut_line,
+				 sizeof(ctl->dst_tty));
 		}
 	}
 
@@ -199,11 +200,10 @@ static void search_utmp(struct write_control *ctl)
 			return;
 		}
 		errx(EXIT_FAILURE, _("%s has messages disabled"), ctl->dst_login);
-	} else if (valid_ttys > 1) {
+	}
+	if (1 < valid_ttys)
 		warnx(_("%s is logged in more than once; writing to %s"),
 		      ctl->dst_login, ctl->dst_tty);
-	}
-
 }
 
 /*
@@ -251,7 +251,7 @@ static void do_write(const struct write_control *ctl)
 	if ((login = getlogin()) == NULL)
 		login = pwuid;
 
-	if (strlen(ctl->dst_tty) + 6 > sizeof(path))
+	if (sizeof(path) < strlen(ctl->dst_tty) + 6)
 		errx(EXIT_FAILURE, _("tty path %s too long"), ctl->dst_tty);
 	snprintf(path, sizeof(path), "/dev/%s", ctl->dst_tty);
 	if ((freopen(path, "w", stdout)) == NULL)
@@ -330,7 +330,6 @@ int main(int argc, char **argv)
 		if (!(ctl.src_tty = ttyname(src_fd)))
 			errx(EXIT_FAILURE,
 			     _("can't find your tty's name"));
-
 		/*
 		 * We may have /dev/ttyN but also /dev/pts/xx. Below,
 		 * check_tty() will put "/dev/" in front, so remove that
-- 
2.9.0

[PATCH 13/15] write: tell when effective gid and tty path group mismatch

[Sami Kerola]

Most commonly this error happens when write(1) executable does not have
correct group ownership and setgid bit.  The earlier message was unclear
what exactly was wrong.

$ mesg
is y
$ write testuser
write: you have write permission turned off

Alternatively the 'getegid() == s.st_gid' could be considered unnecessary.
Afterall if to write to destination tty is denied that does not go unnoticed
at thet time when tty is opened.

Reviewed-by: Benno Schulenberg <bensberg@justemail.net>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/write.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 4300489..7aae796 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -113,8 +113,13 @@ static int check_tty(char *tty, int *tty_writeable, time_t *tty_atime, int showe
 	}
 	if (getuid() == 0)	/* root can always write */
 		*tty_writeable = 1;
-	else
-		*tty_writeable = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
+	else {
+		if (getegid() != s.st_gid) {
+			warnx(_("effective gid does not match group of %s"), path);
+			return 1;
+		}
+		*tty_writeable = s.st_mode & S_IWGRP;
+	}
 	if (tty_atime)
 		*tty_atime = s.st_atime;
 	return 0;
-- 
2.9.0

[PATCH 14/15] write: stop removing and adding /dev/ in front of tty string

[Sami Kerola]

Add both path and tty name representations of tty's to control structure,
that point to same string beginning from different depths.  This way there
is no need to removing and adding /dev/ in front of tty string all the time.

Secondly this change makes it possible to use of get_terminal_name() from
ttyutils.c.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/Makemodule.am |  1 +
 term-utils/write.c       | 81 +++++++++++++++++++++++-------------------------
 2 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/term-utils/Makemodule.am b/term-utils/Makemodule.am
index 8ddb068..1b7c5fc 100644
--- a/term-utils/Makemodule.am
+++ b/term-utils/Makemodule.am
@@ -93,6 +93,7 @@ dist_man_MANS += term-utils/write.1
 write_SOURCES = term-utils/write.c
 write_CFLAGS = $(SUID_CFLAGS) $(AM_CFLAGS)
 write_LDFLAGS = $(SUID_LDFLAGS) $(AM_LDFLAGS)
+write_LDADD = $(LDADD) libcommon.la
 
 if USE_TTY_GROUP
 if MAKEINSTALL_DO_CHOWN
diff --git a/term-utils/write.c b/term-utils/write.c
index 7aae796..a014d95 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -64,6 +64,7 @@
 #include "closestream.h"
 #include "nls.h"
 #include "strutils.h"
+#include "ttyutils.h"
 #include "xalloc.h"
 
 static sig_atomic_t signal_received = 0;
@@ -71,9 +72,11 @@ static sig_atomic_t signal_received = 0;
 struct write_control {
 	uid_t src_uid;
 	const char *src_login;
-	char *src_tty;
+	const char *src_tty_path;
+	const char *src_tty_name;
 	const char *dst_login;
-	char dst_tty[PATH_MAX];
+	char *dst_tty_path;
+	const char *dst_tty_name;
 };
 
 static void __attribute__((__noreturn__)) usage(FILE *out)
@@ -98,24 +101,20 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
  * check_tty - check that a terminal exists, and get the message bit
  *     and the access time
  */
-static int check_tty(char *tty, int *tty_writeable, time_t *tty_atime, int showerror)
+static int check_tty(const char *tty, int *tty_writeable, time_t *tty_atime, int showerror)
 {
 	struct stat s;
-	char path[PATH_MAX];
 
-	if (sizeof(path) < strlen(tty) + 6)
-		return 1;
-	sprintf(path, "/dev/%s", tty);
-	if (stat(path, &s) < 0) {
+	if (stat(tty, &s) < 0) {
 		if (showerror)
-			warn("%s", path);
+			warn("%s", tty);
 		return 1;
 	}
 	if (getuid() == 0)	/* root can always write */
 		*tty_writeable = 1;
 	else {
 		if (getegid() != s.st_gid) {
-			warnx(_("effective gid does not match group of %s"), path);
+			warnx(_("effective gid does not match group of %s"), tty);
 			return 1;
 		}
 		*tty_writeable = s.st_mode & S_IWGRP;
@@ -139,7 +138,7 @@ static int check_utmp(const struct write_control *ctl)
 
 	while ((u = getutent())) {
 		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) == 0 &&
-		    strncmp(ctl->dst_tty, u->ut_line, sizeof(u->ut_line)) == 0) {
+		    strncmp(ctl->dst_tty_name, u->ut_line, sizeof(u->ut_line)) == 0) {
 			res = 0;
 			break;
 		}
@@ -165,6 +164,7 @@ static void search_utmp(struct write_control *ctl)
 	struct utmp *u;
 	time_t best_atime = 0, tty_atime;
 	int num_ttys = 0, valid_ttys = 0, tty_writeable = 0, user_is_me = 0;
+	char path[UT_LINESIZE + 6];
 
 	utmpname(_PATH_UTMP);
 	setutent();
@@ -173,13 +173,14 @@ static void search_utmp(struct write_control *ctl)
 		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) != 0)
 			continue;
 		num_ttys++;
-		if (check_tty(u->ut_line, &tty_writeable, &tty_atime, 0))
+		sprintf(path, "/dev/%s", u->ut_line);
+		if (check_tty(path, &tty_writeable, &tty_atime, 0))
 			/* bad term? skip */
 			continue;
 		if (ctl->src_uid && !tty_writeable)
 			/* skip ttys with msgs off */
 			continue;
-		if (strcmp(u->ut_line, ctl->src_tty) == 0) {
+		if (strcmp(u->ut_line, ctl->src_tty_name) == 0) {
 			user_is_me = 1;
 			/* don't write to yourself */
 			continue;
@@ -190,8 +191,9 @@ static void search_utmp(struct write_control *ctl)
 		valid_ttys++;
 		if (best_atime < tty_atime) {
 			best_atime = tty_atime;
-			xstrncpy(ctl->dst_tty, u->ut_line,
-				 sizeof(ctl->dst_tty));
+			free(ctl->dst_tty_path);
+			ctl->dst_tty_path = xstrdup(path);
+			ctl->dst_tty_name = ctl->dst_tty_path + 5;
 		}
 	}
 
@@ -201,14 +203,17 @@ static void search_utmp(struct write_control *ctl)
 	if (valid_ttys == 0) {
 		if (user_is_me) {
 			/* ok, so write to yourself! */
-			xstrncpy(ctl->dst_tty, ctl->src_tty, sizeof(ctl->dst_tty));
+			if (!ctl->src_tty_path)
+				errx(EXIT_FAILURE, _("can't find your tty's name"));
+			ctl->dst_tty_path = xstrdup(ctl->src_tty_path);
+			ctl->dst_tty_name = ctl->dst_tty_path + 5;
 			return;
 		}
 		errx(EXIT_FAILURE, _("%s has messages disabled"), ctl->dst_login);
 	}
 	if (1 < valid_ttys)
 		warnx(_("%s is logged in more than once; writing to %s"),
-		      ctl->dst_login, ctl->dst_tty);
+		      ctl->dst_login, ctl->dst_tty_name);
 }
 
 /*
@@ -245,7 +250,7 @@ static void do_write(const struct write_control *ctl)
 	struct passwd *pwd;
 	time_t now;
 	struct tm *tm;
-	char path[PATH_MAX], *host, line[512];
+	char *host, line[512];
 	struct sigaction sigact;
 
 	/* Determine our login name(s) before the we reopen() stdout */
@@ -256,11 +261,8 @@ static void do_write(const struct write_control *ctl)
 	if ((login = getlogin()) == NULL)
 		login = pwuid;
 
-	if (sizeof(path) < strlen(ctl->dst_tty) + 6)
-		errx(EXIT_FAILURE, _("tty path %s too long"), ctl->dst_tty);
-	snprintf(path, sizeof(path), "/dev/%s", ctl->dst_tty);
-	if ((freopen(path, "w", stdout)) == NULL)
-		err(EXIT_FAILURE, "%s", path);
+	if ((freopen(ctl->dst_tty_path, "w", stdout)) == NULL)
+		err(EXIT_FAILURE, "%s", ctl->dst_tty_path);
 
 	sigact.sa_handler = signal_handler;
 	sigemptyset(&sigact.sa_mask);
@@ -279,10 +281,10 @@ static void do_write(const struct write_control *ctl)
 	printf("\r\n\a\a\a");
 	if (strcmp(login, pwuid))
 		printf(_("Message from %s@%s (as %s) on %s at %s ..."),
-		       login, host, pwuid, ctl->src_tty, timestamp);
+		       login, host, pwuid, ctl->src_tty_name, timestamp);
 	else
 		printf(_("Message from %s@%s on %s at %s ..."),
-		       login, host, ctl->src_tty, timestamp);
+		       login, host, ctl->src_tty_name, timestamp);
 	free(host);
 	printf("\r\n");
 
@@ -331,25 +333,16 @@ int main(int argc, char **argv)
 	else
 		src_fd = -1;
 
-	if (src_fd != -1) {
-		if (!(ctl.src_tty = ttyname(src_fd)))
-			errx(EXIT_FAILURE,
-			     _("can't find your tty's name"));
-		/*
-		 * We may have /dev/ttyN but also /dev/pts/xx. Below,
-		 * check_tty() will put "/dev/" in front, so remove that
-		 * part.
-		 */
-		if (!strncmp(ctl.src_tty, "/dev/", 5))
-			ctl.src_tty += 5;
-		if (check_tty(ctl.src_tty, &tty_writeable, NULL, 1))
+	if (src_fd != -1 &&
+	    get_terminal_name(src_fd, &ctl.src_tty_path, &ctl.src_tty_name, NULL) == 0) {
+		if (check_tty(ctl.src_tty_path, &tty_writeable, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (!tty_writeable)
 			errx(EXIT_FAILURE,
 			     _("you have write permission turned off"));
 		tty_writeable = 0;
 	} else
-		ctl.src_tty = "<no tty>";
+		ctl.src_tty_name = "<no tty>";
 
 	ctl.src_uid = getuid();
 
@@ -363,23 +356,25 @@ int main(int argc, char **argv)
 	case 3:
 		ctl.dst_login = argv[1];
 		if (!strncmp(argv[2], "/dev/", 5))
-			xstrncpy(ctl.dst_tty, argv[2] + 5, sizeof(ctl.dst_tty));
+			ctl.dst_tty_path = xstrdup(argv[2]);
 		else
-			xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
+			xasprintf(&ctl.dst_tty_path, "/dev/%s", argv[2]);
+		ctl.dst_tty_name = ctl.dst_tty_path + 5;
 		if (check_utmp(&ctl))
 			errx(EXIT_FAILURE,
 			     _("%s is not logged in on %s"),
-			     ctl.dst_login, ctl.dst_tty);
-		if (check_tty(ctl.dst_tty, &tty_writeable, NULL, 1))
+			     ctl.dst_login, ctl.dst_tty_name);
+		if (check_tty(ctl.dst_tty_path, &tty_writeable, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (ctl.src_uid && !tty_writeable)
 			errx(EXIT_FAILURE,
 			     _("%s has messages disabled on %s"),
-			     ctl.dst_login, ctl.dst_tty);
+			     ctl.dst_login, ctl.dst_tty_name);
 		do_write(&ctl);
 		break;
 	default:
 		usage(stderr);
 	}
+	free(ctl.dst_tty_path);
 	return EXIT_SUCCESS;
 }
-- 
2.9.0

[PATCH 15/15] lib: try to find tty in get_terminal_name()

[Sami Kerola]

Try all standard terminal input/output file descriptors when finding tty
name in get_germinal_name().  This should make all invocations of the
function as robust as they can get.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/ttyutils.h      |  2 +-
 lib/ttyutils.c          | 15 +++++++++++++--
 login-utils/login.c     |  2 +-
 login-utils/su-common.c |  4 ++--
 term-utils/write.c      | 16 +++-------------
 5 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/ttyutils.h b/include/ttyutils.h
index 200e9a5..7278d36 100644
--- a/include/ttyutils.h
+++ b/include/ttyutils.h
@@ -51,7 +51,7 @@ struct chardata {
 	} while (0)
 
 extern int get_terminal_width(int default_width);
-extern int get_terminal_name(int fd, const char **path, const char **name,
+extern int get_terminal_name(const char **path, const char **name,
 			     const char **number);
 
 #define UL_TTY_KEEPCFLAGS	(1 << 1)
diff --git a/lib/ttyutils.c b/lib/ttyutils.c
index 4e62c20..06dd800 100644
--- a/lib/ttyutils.c
+++ b/lib/ttyutils.c
@@ -5,6 +5,7 @@
  * Written by Karel Zak <kzak@redhat.com>
  */
 #include <ctype.h>
+#include <unistd.h>
 
 #include "c.h"
 #include "ttyutils.h"
@@ -42,13 +43,14 @@ int get_terminal_width(int default_width)
 	return width > 0 ? width : default_width;
 }
 
-int get_terminal_name(int fd,
-		      const char **path,
+int get_terminal_name(const char **path,
 		      const char **name,
 		      const char **number)
 {
 	const char *tty;
 	const char *p;
+	int fd;
+
 
 	if (name)
 		*name = NULL;
@@ -57,6 +59,15 @@ int get_terminal_name(int fd,
 	if (number)
 		*number = NULL;
 
+	if (isatty(STDIN_FILENO))
+		fd = STDIN_FILENO;
+	else if (isatty(STDOUT_FILENO))
+		fd = STDOUT_FILENO;
+	else if (isatty(STDERR_FILENO))
+		fd = STDERR_FILENO;
+	else
+		return -1;
+
 	tty = ttyname(fd);
 	if (!tty)
 		return -1;
diff --git a/login-utils/login.c b/login-utils/login.c
index 4d71f54..7501f6d 100644
--- a/login-utils/login.c
+++ b/login-utils/login.c
@@ -356,7 +356,7 @@ static void init_tty(struct login_context *cxt)
 
 	cxt->tty_mode = (mode_t) getlogindefs_num("TTYPERM", TTY_MODE);
 
-	get_terminal_name(0, &cxt->tty_path, &cxt->tty_name, &cxt->tty_number);
+	get_terminal_name(&cxt->tty_path, &cxt->tty_name, &cxt->tty_number);
 
 	/*
 	 * In case login is suid it was possible to use a hardlink as stdin
diff --git a/login-utils/su-common.c b/login-utils/su-common.c
index 78a734c..1776b6b 100644
--- a/login-utils/su-common.c
+++ b/login-utils/su-common.c
@@ -165,7 +165,7 @@ log_syslog(struct passwd const *pw, bool successful)
       old_user = pwd ? pwd->pw_name : "";
     }
 
-  if (get_terminal_name(STDERR_FILENO, NULL, &tty, NULL) != 0 || !tty)
+  if (get_terminal_name(NULL, &tty, NULL) != 0 || !tty)
     tty = "none";
 
   openlog (program_invocation_short_name, 0 , LOG_AUTH);
@@ -192,7 +192,7 @@ static void log_btmp(struct passwd const *pw)
 		pw && pw->pw_name ? pw->pw_name : "(unknown)",
 		sizeof(ut.ut_user));
 
-	get_terminal_name(STDERR_FILENO, NULL, &tty_name, &tty_num);
+	get_terminal_name(NULL, &tty_name, &tty_num);
 	if (tty_num)
 		xstrncpy(ut.ut_id, tty_num, sizeof(ut.ut_id));
 	if (tty_name)
diff --git a/term-utils/write.c b/term-utils/write.c
index a014d95..c6d440d 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -298,7 +298,7 @@ static void do_write(const struct write_control *ctl)
 
 int main(int argc, char **argv)
 {
-	int tty_writeable = 0, src_fd, c;
+	int tty_writeable = 0, c;
 	struct write_control ctl = { 0 };
 
 	static const struct option longopts[] = {
@@ -323,18 +323,8 @@ int main(int argc, char **argv)
 			usage(stderr);
 		}
 
-	/* check that sender has write enabled */
-	if (isatty(STDIN_FILENO))
-		src_fd = STDIN_FILENO;
-	else if (isatty(STDOUT_FILENO))
-		src_fd = STDOUT_FILENO;
-	else if (isatty(STDERR_FILENO))
-		src_fd = STDERR_FILENO;
-	else
-		src_fd = -1;
-
-	if (src_fd != -1 &&
-	    get_terminal_name(src_fd, &ctl.src_tty_path, &ctl.src_tty_name, NULL) == 0) {
+	if (get_terminal_name(&ctl.src_tty_path, &ctl.src_tty_name, NULL) == 0) {
+		/* check that sender has write enabled */
 		if (check_tty(ctl.src_tty_path, &tty_writeable, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (!tty_writeable)
-- 
2.9.0

Re: [PATCH 11/15] write: remove PUTC macro

[Sami Kerola]

On 3 July 2016 at 22:23, Sami Kerola <kerolasa@iki.fi> wrote:
> +               if (c == '\n' && fputc_careful('\r', stdout, '^') == EOF)
> +                       goto fail;
> +               if (fputc_careful(c, stdout, '^') == EOF)
> + fail:
> +                       err(EXIT_FAILURE, _("carefulputc failed"));

The two if statements can be merged, and goto becomes unnecessary. Fixed in.

https://github.com/kerolasa/lelux-utiliteetit/commit/a8031743eaea418a9512c426f5678ea53e943ceb

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/15] pull: write(1) improvements

[Karel Zak]

On Sun, Jul 03, 2016 at 10:23:43PM +0100, Sami Kerola wrote:
> are available in the git repository at:
>   git://github.com/kerolasa/lelux-utiliteetit.git write-improvements

 Merged. Thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com