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

[Daniel Stone]

Hi,

On 17 November 2016 at 14:41, Pekka Paalanen
 wrote:
> On Thu, 17 Nov 2016 12:17:59 +
> Daniel Stone  wrote:
>> @@ -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.

Oh, too clever by half. I just left your explicit termination at byte
11 and have pushed it so we can all move on with our lives.

Thanks for the thorough review all.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Pekka Paalanen]

On Thu, 17 Nov 2016 12:17:59 +
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.
> 
> 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 
> Cc: Pekka Paalanen 
> Cc: Eric Engestrom 
> ---
>  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, )) {
>   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;

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Eric Engestrom]

On Thursday, 2016-11-17 12:17:59 +, 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.
> 
> 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 
> Cc: Pekka Paalanen 
> Cc: Eric Engestrom 

Reviewed-by: Eric Engestrom 

> ---
>  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];
>   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, )) {
>   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;
> -- 
> 2.9.3
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2] 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.

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 
Cc: Pekka Paalanen 
Cc: Eric Engestrom 
---
 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];
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, )) {
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;
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel