On Thu, 17 Nov 2016 11:35:53 +0000 Daniel Stone <[email protected]> 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 <[email protected]> > Cc: Pekka Paalanen <[email protected]> > Cc: Eric Engestrom <[email protected]> > --- > 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; Reviewed-by: Pekka Paalanen <[email protected]> Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
