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

[Daniel Stone]

Hi,

On 17 November 2016 at 12:03, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> On Thursday, 2016-11-17 11:35:53 +0000, Daniel Stone wrote:
>> 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 :)

Sure it's overkill, but it's not a particularly performance-sensitive
path, and it seemed that clarity was more important, given the history
of both Weston and Mutter (which copied Weston's implementation)
getting it subtly wrong.

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

Sure, there might be, but on the other hand, if there isn't then this
part of 25a2bdd814 should likely be reverted. Xorg itself uses sscanf,
and Mutter uses bare strtol, so will accept varying degrees of
anything in the file. On the other hand, all known versions of those
two and Weston print either \n or \0 in that position, so probably no
point encouraging people to get creative with what they put there.

Mutter is actually probably the best at parsing, since it uses strtol
and verifies that the end pointer is pid[10]. That seems good to me,
but reverting the safe_strtol change and doing that is a bit more
effort than this deserves.

>> @@ -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) {

dprintf looks good, so I've done that and updated the documentation, thanks.

Cheers,
Daniel