[PATCH weston] xwayland: Fix X11 lock file size confusion

[Daniel Stone]

The X11 lock file was somewhat opaque. Into a sized array of 16
characters, we previously read 11 bytes. 61beda653b fixed the parsing of
this input to ensure that we only considered the first 10 bytes: this
has the effect of culling a LF byte at the end of the string.

This commit more explicitly NULLs the entire string before reading, and
trims trailing LF characters only.

It also adds some documentation by way of resizing pid, an explicit size
check on snprintf's return, and comments.

Related Mutter issue: https://bugzilla.gnome.org/show_bug.cgi?id=774613

Signed-off-by: Daniel Stone <daniels at collabora.com>
Cc: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
Cc: Eric Engestrom <eric at engestrom.ch>
---
 xwayland/launcher.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/xwayland/launcher.c b/xwayland/launcher.c
index 97d7c6e..56b949e 100644
--- a/xwayland/launcher.c
+++ b/xwayland/launcher.c
@@ -148,13 +148,19 @@ bind_to_unix_socket(int display)
 static int
 create_lockfile(int display, char *lockfile, size_t lsize)
 {
-	char pid[16];
+	/* 10 decimal characters, trailing LF and NUL byte; see comment
+	 * at end of function. */
+	char pid[12];
 	int fd, size;
 	pid_t other;
 
 	snprintf(lockfile, lsize, "/tmp/.X%d-lock", display);
 	fd = open(lockfile, O_WRONLY | O_CLOEXEC | O_CREAT | O_EXCL, 0444);
 	if (fd < 0 && errno == EEXIST) {
+		/* Clear our input buffer so we always have a NUL-terminated
+		 * string. */
+		memset(pid, 0, sizeof(pid));
+
 		fd = open(lockfile, O_CLOEXEC | O_RDONLY);
 		if (fd < 0 || read(fd, pid, 11) != 11) {
 			weston_log("can't read lock file %s: %s\n",
@@ -166,8 +172,10 @@ create_lockfile(int display, char *lockfile, size_t lsize)
 			return -1;
 		}
 
-		/* Trim the newline, ensure terminated string. */
-		pid[10] = '\0';
+		/* If the last character is LF, trim it so safe_strtoint
+		 * doesn't explode. */
+		if (pid[10] == '\n')
+			pid[10] = '\0';
 
 		if (!safe_strtoint(pid, &other)) {
 			weston_log("can't parse lock file %s\n",
@@ -199,10 +207,15 @@ create_lockfile(int display, char *lockfile, size_t lsize)
 		return -1;
 	}
 
-	/* Subtle detail: we use the pid of the wayland
-	 * compositor, not the xserver in the lock file. */
+	/* Subtle detail: we use the pid of the wayland compositor, not the
+	 * xserver in the lock file. Another subtlety: snprintf accepts a
+	 * size value which includes the trailing NUL byte, and returns a
+	 * size value which does not (already off by one), which is the
+	 * number of characters it would've hypothetically written, rather
+	 * than the number it actually wrote. Here we don't want to write
+	 * the trailing NUL byte. */
 	size = snprintf(pid, sizeof pid, "%10d\n", getpid());
-	if (write(fd, pid, size) != size) {
+	if (size != 11 || write(fd, pid, size) != size) {
 		unlink(lockfile);
 		close(fd);
 		return -1;
-- 
2.9.3