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 <[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));
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
>
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
