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

[Eric Engestrom]

On Thursday, 2016-11-17 11:35:53 +0000, Daniel Stone wrote:
> 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));

Isn't that a bit of overkill? Clear all the bytes just before
overwriting almost all of them, just so the last one is null?
I know it's only 12 bytes, but it still looks weird to me :)

> +
>  		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';

Is there any situation where this `if` isn't going to be true?

>  
>  		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

Nit: newline before "Another subtlety" to make it more prominent.

> +	 * 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) {

Why aren't we just doing this here?
	size = dprintf(fd, "%10d\n", getpid());
	if (size != 11) {

Other than that, big +1 on documenting things :)

Cheers,
  Eric

>  		unlink(lockfile);
>  		close(fd);
>  		return -1;
> -- 
> 2.9.3
>