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

[Pekka Paalanen]

On Thu, 17 Nov 2016 12:17:59 +0000
Daniel Stone <daniels at collabora.com> 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.
> 
> Verified manually that it emits lock files with a trailing \n, as Xorg
> does. Also verified manually that it ignores misformatted lock files,
> but accepts either \n or \0 in the trailing position.
> 
> 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 | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/xwayland/launcher.c b/xwayland/launcher.c
> index 97d7c6e..c3114f5 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[11];

Hi,

um, 11? No room for the terminator-for-sure?

Otherwise looks good.


Thanks,
pq


>  	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,12 @@ 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. */
> -	size = snprintf(pid, sizeof pid, "%10d\n", getpid());
> -	if (write(fd, pid, size) != size) {
> +	/* Subtle detail: we use the pid of the wayland compositor, not the
> +	 * xserver in the lock file.
> +	 * Also subtle is that we don't emit a trailing NUL to the file, so
> +	 * our size here is 11 rather than 12. */
> +	size = dprintf(fd, "%10d\n", getpid());
> +	if (size != 11) {
>  		unlink(lockfile);
>  		close(fd);
>  		return -1;