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

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

[Sami Kerola]

Hello,

Recent write(1) regression inspired me to have a look what is going on with
this source file.  Here are the results of the inspection.  I am pretty
confident most of the changes are quick and easy to review, with exception
of one - the last change.  Let me explain a little bit closer.

Current messaging is fine assuming group ownership and setgid bit are set,
but if they are not then one will get misleading message about oneself not
having mesg(1) enabled (even if they would be).  The difficult bit is how
should be fixed.  I can think of three different options:

  1) Match process effective gid with tty device file gid, and complain when
  not matching.  This is what I propose.

  2) Use faccessat(3) to determine whether run time has effivice access to
  tty path.

  3) Perform the check at the time of stdout freopen(2), and fail with
  permission denied when necessary.

Second option feels a bit pointless as the third option does exactly the
same, and one only has to delete code to make that to work.  Main reason why
I like first option is that it gives hint how one ought to fix an issue. 
Simple 'permission denied' is less obvious.  Unfortunately the option 1 can
cause failure when write would have been possible due ownership of the tty
file - so in that sense option 2 is better.  So where I'm getting at is that
there's good and bad sides in each of these options and it would be nice to
hear what others think is the best thing to do.

----------------------------------------------------------------
The following changes since commit 94af67f2aaaafce58f495b2415e534a0c2508332:
  Merge branch 'no-fork' of https://github.com/terryburton/util-linux (2016-05-09 13:28:52 +0200)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git write-improvements
for you to fetch changes up to 354f2ee527fe9d6f5d6dcd111af49b47469eaad6:
  write: tell when effective gid and tty path group mismatch (2016-05-09 22:03:14 +0100)
----------------------------------------------------------------

Sami Kerola (12):
  write: remove unused variable
  write: get rid of function prototypes
  write: remove pointless fileno(3) calls
  write: set atime value in term_chk() only if 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 variable names
  write: remove unnecessary utmp variables
  write: make timestamp to be obviously just a clock time
  write: improve coding style
  write: tell when effective gid and tty path group mismatch

 term-utils/write.c | 395 +++++++++++++++++++++++++++--------------------------
 1 file changed, 199 insertions(+), 196 deletions(-)

-- 
2.8.2

[PATCH 01/12] 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 14594f5..42f279b 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.8.2

[PATCH 02/12] 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 42f279b..e8532f6 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.8.2

[PATCH 03/12] 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 e8532f6..4c6a25f 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.8.2

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

[Sami Kerola]

The atime comparison is done only in search_utmp(), the main does not use
this variable.

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 4c6a25f..dd31f59 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.8.2

[PATCH 05/12] 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.  In practise this is a
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 dd31f59..ed508d4 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.8.2

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

[Sami Kerola]

The use of _exit() at the end of earlier signal handler, that was called at
end of execution, caused atexit() standard file descriptor close checks not
to be done.  So make the signal handler to set flag to exit, and perform
expected exit workflow.

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 ed508d4..021e943 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.8.2

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

[Sami Kerola]

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

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

diff --git a/term-utils/write.c b/term-utils/write.c
index 021e943..cdcc0af 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;
+	char *src_login;
+	char *src_tty;
+	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,42 @@ 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))
+			memmove(ctl.src_tty, ctl.src_tty + 5, strlen(ctl.src_tty + 5) + 1);
+		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:
-		if (!strncmp(argv[2], "/dev/", 5))
-			argv[2] += 5;
-		if (utmp_chk(argv[1], argv[2]))
+		ctl.dst_login = argv[1];
+		xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
+		if (!strncmp(ctl.dst_tty, "/dev/", 5))
+			memmove(ctl.dst_tty, ctl.dst_tty + 5, strlen(ctl.dst_tty + 5) + 1);
+		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.8.2

[PATCH 08/12] write: improve 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 cdcc0af..1ae3c82 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 *mesg_allowed, 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;
+		*mesg_allowed = 1;
 	else
-		*msgsokP = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
-	if (atimeP)
-		*atimeP = s.st_atime;
+		*mesg_allowed = (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, mesg_allowed = 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, &mesg_allowed, &tty_atime, 0))
 				/* bad term? skip */
 				continue;
-			if (ctl->src_uid && !msgsok)
+			if (ctl->src_uid && !mesg_allowed)
 				/* 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 mesg_allowed = 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))
 			memmove(ctl.src_tty, ctl.src_tty + 5, strlen(ctl.src_tty + 5) + 1);
-		if (term_chk(ctl.src_tty, &msgsok, NULL, 1))
+		if (check_tty(ctl.src_tty, &mesg_allowed, NULL, 1))
 			exit(EXIT_FAILURE);
-		if (!msgsok)
+		if (!mesg_allowed)
 			errx(EXIT_FAILURE,
 			     _("you have write permission turned off"));
-		msgsok = 0;
+		mesg_allowed = 0;
 	} else
 		ctl.src_tty = "<no tty>";
 
@@ -372,13 +369,13 @@ int main(int argc, char **argv)
 		xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
 		if (!strncmp(ctl.dst_tty, "/dev/", 5))
 			memmove(ctl.dst_tty, ctl.dst_tty + 5, strlen(ctl.dst_tty + 5) + 1);
-		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, &mesg_allowed, NULL, 1))
 			exit(EXIT_FAILURE);
-		if (ctl.src_uid && !msgsok)
+		if (ctl.src_uid && !mesg_allowed)
 			errx(EXIT_FAILURE,
 			     _("%s has messages disabled on %s"),
 			     ctl.dst_login, ctl.dst_tty);
-- 
2.8.2

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

[Sami Kerola]

The getutent() calls are not thread safe, and reference documentation is
recommending to copy the context of the structures if information is wished
to be stored.  But excessive copying does not seem to be relevant for single
threaded application.

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 1ae3c82..403d4a7 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -126,17 +126,15 @@ static int check_tty(char *tty, int *mesg_allowed, time_t *tty_atime, int shower
  */
 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, mesg_allowed = 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, &mesg_allowed, &tty_atime, 0))
+			if (check_tty(u->ut_line, &mesg_allowed, &tty_atime, 0))
 				/* bad term? skip */
 				continue;
 			if (ctl->src_uid && !mesg_allowed)
 				/* 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.8.2

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

[Sami Kerola]

Just by looking the code one will have hard time knowing that a slice of
ctime() from characters 11 to 16 is HH:MM time format.  Use of strftime()
tells exactly what one can expect without additional comments.

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 403d4a7..b10c129 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, time_stamp[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(time_stamp, sizeof(time_stamp), "%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, time_stamp);
 	else
 		printf(_("Message from %s@%s on %s at %s ..."),
-		       login, host, ctl->src_tty, time_stamp + 11);
+		       login, host, ctl->src_tty, time_stamp);
 	free(host);
 	printf("\r\n");
 
-- 
2.8.2

[PATCH 11/12] write: improve coding style

[Sami Kerola]

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

diff --git a/term-utils/write.c b/term-utils/write.c
index b10c129..e3e749e 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -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,
@@ -199,11 +199,9 @@ static void search_utmp(struct write_control *ctl)
 			return;
 		}
 		errx(EXIT_FAILURE, _("%s has messages disabled"), ctl->dst_login);
-	} else if (valid_ttys > 1) {
+	} else if (valid_ttys > 1)
 		warnx(_("%s is logged in more than once; writing to %s"),
 		      ctl->dst_login, ctl->dst_tty);
-	}
-
 }
 
 /*
@@ -215,6 +213,15 @@ static void signal_handler(int signo)
 }
 
 /*
+ * PUTC - fputc_careful wrapper
+ */
+static inline void PUTC(char c)
+{
+	if (fputc_careful(c, stdout, '^') == EOF)
+		err(EXIT_FAILURE, _("carefulputc failed"));
+}
+
+/*
  * write_line - like fputs(), but makes control characters visible and
  *     turns \n into \r\n.
  */
@@ -222,8 +229,6 @@ 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')
@@ -231,7 +236,6 @@ static void write_line(char *s)
 		PUTC(c);
 	}
 	return;
-#undef PUTC
 }
 
 /*
@@ -333,7 +337,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.8.2

[PATCH 12/12] 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.

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 e3e749e..58d3839 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -113,8 +113,13 @@ static int check_tty(char *tty, int *mesg_allowed, time_t *tty_atime, int shower
 	}
 	if (getuid() == 0)	/* root can always write */
 		*mesg_allowed = 1;
-	else
-		*mesg_allowed = (s.st_mode & S_IWGRP) && (getegid() == s.st_gid);
+	else {
+		if (getegid() != s.st_gid) {
+			warnx(_("effective gid and %s group does not match"), path);
+			return 1;
+		}
+		*mesg_allowed = s.st_mode & S_IWGRP;
+	}
 	if (tty_atime)
 		*tty_atime = s.st_atime;
 	return 0;
-- 
2.8.2

Re: [PATCH 12/12] write: tell when effective gid and tty path group mismatch

[Benno Schulenberg]

On Mon, May 9, 2016, at 23:27, Sami Kerola wrote:
> +		if (getegid() != s.st_gid) {
> +			warnx(_("effective gid and %s group does not match"), path);

For translation purposes, better make this:

  "effective gid does not match group of %s"

otherwise a translator might think that %s is the name of a group.
(And the grammar of the original is incorrect.)

Benno

-- 
http://www.fastmail.com - Does exactly what it says on the tin

Re: [PATCH 12/12] write: tell when effective gid and tty path group mismatch

[Sami Kerola]

On 10 May 2016 at 15:24, Benno Schulenberg <bensberg@justemail.net> wrote:
>
> On Mon, May 9, 2016, at 23:27, Sami Kerola wrote:
>> +             if (getegid() != s.st_gid) {
>> +                     warnx(_("effective gid and %s group does not match"), path);
>
> For translation purposes, better make this:
>
>   "effective gid does not match group of %s"
>
> otherwise a translator might think that %s is the name of a group.
> (And the grammar of the original is incorrect.)

Thank you for review Benno, fixed in

https://github.com/kerolasa/lelux-utiliteetit/commit/8e8c57e69985a22260c30793080a9776f47081a0

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

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

[Karel Zak]

On Mon, May 09, 2016 at 10:27:23PM +0100, Sami Kerola wrote:
> @@ -339,39 +347,42 @@ 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))
> +			memmove(ctl.src_tty, ctl.src_tty + 5, strlen(ctl.src_tty + 5) + 1);
> +		if (term_chk(ctl.src_tty, &msgsok, NULL, 1))

The patch promises that it replaces variables with control struct, but
here it does something more. Don't do that. There is no memmove() in
the original code. 

Please, if you want to optimize the code than do it in another patch.

and btw: we have lib/ttyutils.c: get_terminal_name(), it seems usable
for this use-case, but...

> -		if (!strncmp(argv[2], "/dev/", 5))
> -			argv[2] += 5;
> -		if (utmp_chk(argv[1], argv[2]))
> +		ctl.dst_login = argv[1];
> +		xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
> +		if (!strncmp(ctl.dst_tty, "/dev/", 5))
> +			memmove(ctl.dst_tty, ctl.dst_tty + 5, strlen(ctl.dst_tty + 5) + 1);

... maybe we need xttyname() to return a name (no path) and in
allocated string for programs where we call ttyname() more times.

    Karel

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

Re: [PATCH 11/12] write: improve coding style

[Karel Zak]

On Mon, May 09, 2016 at 10:27:27PM +0100, Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  term-utils/write.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/term-utils/write.c b/term-utils/write.c
> index b10c129..e3e749e 100644
> --- a/term-utils/write.c
> +++ b/term-utils/write.c
> @@ -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,
> @@ -199,11 +199,9 @@ static void search_utmp(struct write_control *ctl)
>  			return;
>  		}
>  		errx(EXIT_FAILURE, _("%s has messages disabled"), ctl->dst_login);
> -	} else if (valid_ttys > 1) {
> +	} else if (valid_ttys > 1)
>  		warnx(_("%s is logged in more than once; writing to %s"),
>  		      ctl->dst_login, ctl->dst_tty);
> -	}
> -
>  }
>  
>  /*
> @@ -215,6 +213,15 @@ static void signal_handler(int signo)
>  }
>  
>  /*
> + * PUTC - fputc_careful wrapper
> + */
> +static inline void PUTC(char c)
> +{
> +	if (fputc_careful(c, stdout, '^') == EOF)
> +		err(EXIT_FAILURE, _("carefulputc failed"));
> +}
> +
> +/*
>   * write_line - like fputs(), but makes control characters visible and
>   *     turns \n into \r\n.
>   */
> @@ -222,8 +229,6 @@ 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')
> @@ -231,7 +236,6 @@ static void write_line(char *s)
>  		PUTC(c);
>  	}
>  	return;
> -#undef PUTC
>  }

You do not need the crazy macro at all

   while (*s) {
        int rc = 0;
        int c = *s++;

        if (c == '\n')
            rc = fputc_careful('\n', stdout, '^');
        if (rc == 0)
            rc = fputc_careful(c, stdout, '^');
        if (rc == EOF)
            err(EXIT_FAILURE, _("carefulputc failed"));
   }

    Karel


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

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

[Sami Kerola]

On Wed, 11 May 2016, Karel Zak wrote:

> On Mon, May 09, 2016 at 10:27:23PM +0100, Sami Kerola wrote:
> > @@ -339,39 +347,42 @@ 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))
> > +			memmove(ctl.src_tty, ctl.src_tty + 5, strlen(ctl.src_tty + 5) + 1);
> > +		if (term_chk(ctl.src_tty, &msgsok, NULL, 1))
> 
> The patch promises that it replaces variables with control struct, but
> here it does something more. Don't do that. There is no memmove() in
> the original code. 

Fair enough. The memmove() is removed from

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

> Please, if you want to optimize the code than do it in another patch.

And what comes to these "/dev/" strings I rethought what would be better. 
That resulted to new change.

https://github.com/kerolasa/lelux-utiliteetit/commit/57b1531faf120aa136064aff2b6e2e0a23c2ba21

-- snip
From: Sami Kerola <kerolasa@iki.fi>
Date: Sat, 14 May 2016 19:39:37 +0100
Subject: [PATCH 14/15] write: stop removing and adding /dev/ in front of tty string

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,
and use of get_terminal_name() from ttyutils.c is possible.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/Makemodule.am |  1 +
 term-utils/write.c       | 81 +++++++++++++++++++++++-------------------------
 2 files changed, 40 insertions(+), 42 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 351a455..6900752 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 *mesg_allowed, time_t *tty_atime, int showerror)
+static int check_tty(const char *tty, int *mesg_allowed, time_t *tty_atime, int showerror)
 {
 	struct stat s;
-	char path[PATH_MAX];
 
-	if (strlen(tty) + 6 > sizeof(path))
-		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 */
 		*mesg_allowed = 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;
 		}
 		*mesg_allowed = 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, mesg_allowed = 0, user_is_me = 0;
+	char path[PATH_MAX];
 
 	utmpname(_PATH_UTMP);
 	setutent();
@@ -172,13 +172,14 @@ static void search_utmp(struct write_control *ctl)
 	while ((u = getutent())) {
 		if (strncmp(ctl->dst_login, u->ut_user, sizeof(u->ut_user)) == 0) {
 			num_ttys++;
-			if (check_tty(u->ut_line, &mesg_allowed, &tty_atime, 0))
+			sprintf(path, "/dev/%s", u->ut_line);
+			if (check_tty(path, &mesg_allowed, &tty_atime, 0))
 				/* bad term? skip */
 				continue;
 			if (ctl->src_uid && !mesg_allowed)
 				/* 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;
@@ -189,7 +190,9 @@ static void search_utmp(struct write_control *ctl)
 			valid_ttys++;
 			if (tty_atime > best_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;
 			}
 		}
 	}
@@ -200,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 (valid_ttys > 1)
 		warnx(_("%s is logged in more than once; writing to %s"),
-		      ctl->dst_login, ctl->dst_tty);
+		      ctl->dst_login, ctl->dst_tty_name);
 }
 
 /*
@@ -246,7 +252,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 */
@@ -257,11 +263,8 @@ static void do_write(const struct write_control *ctl)
 	if ((login = getlogin()) == NULL)
 		login = pwuid;
 
-	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);
+	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);
@@ -280,10 +283,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, time_stamp);
+		       login, host, pwuid, ctl->src_tty_name, time_stamp);
 	else
 		printf(_("Message from %s@%s on %s at %s ..."),
-		       login, host, ctl->src_tty, time_stamp);
+		       login, host, ctl->src_tty_name, time_stamp);
 	free(host);
 	printf("\r\n");
 
@@ -332,25 +335,17 @@ 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, &mesg_allowed, 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, &mesg_allowed, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (!mesg_allowed)
 			errx(EXIT_FAILURE,
 			     _("you have write permission turned off"));
 		mesg_allowed = 0;
 	} else
-		ctl.src_tty = "<no tty>";
+		ctl.src_tty_name = "<no tty>";
 
 	ctl.src_uid = getuid();
 
@@ -364,23 +359,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, &mesg_allowed, NULL, 1))
+			     ctl.dst_login, ctl.dst_tty_name);
+		if (check_tty(ctl.dst_tty_path, &mesg_allowed, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (ctl.src_uid && !mesg_allowed)
 			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.8.2
-- snip

> and btw: we have lib/ttyutils.c: get_terminal_name(), it seems usable
> for this use-case, but...
> 
> > -		if (!strncmp(argv[2], "/dev/", 5))
> > -			argv[2] += 5;
> > -		if (utmp_chk(argv[1], argv[2]))
> > +		ctl.dst_login = argv[1];
> > +		xstrncpy(ctl.dst_tty, argv[2], sizeof(ctl.dst_tty));
> > +		if (!strncmp(ctl.dst_tty, "/dev/", 5))
> > +			memmove(ctl.dst_tty, ctl.dst_tty + 5, strlen(ctl.dst_tty + 5) + 1);
> 
> ... maybe we need xttyname() to return a name (no path) and in
> allocated string for programs where we call ttyname() more times.

The get_terminal_name() seems pretty suitable for write(1) without 
major changes. That said I end up proposing a small change to it.

https://github.com/kerolasa/lelux-utiliteetit/commit/554bfbe07c03bd2cf12910ee53be258dcb95c82e

-- snip
From: Sami Kerola <kerolasa@iki.fi>
Date: Sat, 14 May 2016 19:50:41 +0100
Subject: [PATCH 15/15] lib: try to find tty in get_terminal_name()

Try to use all standard terminal input/output file descriptors to find 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 1a1d8bc..d3ec196 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 6432693..a1176de 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 fe6d0c8..483243f 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 6900752..3580f57 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -300,7 +300,7 @@ static void do_write(const struct write_control *ctl)
 
 int main(int argc, char **argv)
 {
-	int mesg_allowed = 0, src_fd, c;
+	int mesg_allowed = 0, c;
 	struct write_control ctl = { 0 };
 
 	static const struct option longopts[] = {
@@ -325,19 +325,9 @@ 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,
+	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, &mesg_allowed, NULL, 1))
 			exit(EXIT_FAILURE);
 		if (!mesg_allowed)
-- 
2.8.2
-- snip

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

Re: [PATCH 11/12] write: improve coding style

[Sami Kerola]

On Wed, 11 May 2016, Karel Zak wrote:

> You do not need the crazy macro at all
> 
>    while (*s) {
>         int rc = 0;
>         int c = *s++;
> 
>         if (c == '\n')
>             rc = fputc_careful('\n', stdout, '^');
>         if (rc == 0)
>             rc = fputc_careful(c, stdout, '^');
>         if (rc == EOF)
>             err(EXIT_FAILURE, _("carefulputc failed"));
>    }

The commit is amended in spirit of 'no macro at all'. The code is not 
exactly the same as above.

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

-- snip
From: Sami Kerola <kerolasa@iki.fi>
Date: Sat, 14 May 2016 16:40:45 +0100
Subject: [PATCH 11/15] write: remove PUTC macro

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 | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/term-utils/write.c b/term-utils/write.c
index 57b2254..3b86694 100644
--- a/term-utils/write.c
+++ b/term-utils/write.c
@@ -220,18 +220,17 @@ 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') {
+			if (fputc_careful('\r', stdout, '^') == EOF)
+				goto fail;
+		}
+		if (fputc_careful(c, stdout, '^') == EOF)
+ fail:
+			err(EXIT_FAILURE, _("carefulputc failed"));
 	}
-	return;
-#undef PUTC
 }
 
 /*
-- 
2.8.2
-- snip

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