[PATCH xserver] os: Fix strtok/free crash in ComputeLocalClient

[Tomasz Śniatowski]

On 8 December 2017 at 16:46, Michel Dänzer <michel at daenzer.net> wrote:
> On 2017-12-07 11:19 AM, walter harms wrote:
>> Am 06.12.2017 12:16, schrieb Tomasz Śniatowski:
>>> Don't reuse cmd for strtok output to ensure the proper pointer is
>>> freed afterwards.
>>>
>>> The code incorrectly assumed the pointer returned by strtok(cmd, ":")
>>> would always point to cmd. However, strtok(str, sep) != str if str
>>> begins with sep. This caused an invalid-free crash when running
>>> a program under X with a name beginning with a colon.
>>>
>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=104123
>>>
>>
>> Yes, strtok returns a pointer to its arguments.
>> btw: strtok is not thread-safe, could that be an issue with that function ?

Not really -- the crash I'm getting is 100% reproducible with no races
or thread issues in sight. Equivalent crash call stacks can be obtained
in a toy program that does a
    free(strtok(strdup(":a"), ":");
which is the same incorrect use of strtok's return value in free.

> Hmm. My initial answer was no, since ComputeLocalClient should only be
> called on the main thread. But if there can be a race with strtok
> getting called from other places, there might be an issue. Anyway,
> that's not something introduced by this patch but could be addressed in
> another patch. This patch is
>
> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>

Forgive my ignorance, am I supposed to do something now to get this merged?

>>> Signed-off-by: Tomasz Śniatowski <kailoran at gmail.com>
>>> ---
>>>  os/access.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/os/access.c b/os/access.c
>>> index 8828e0834..97246160c 100644
>>> --- a/os/access.c
>>> +++ b/os/access.c
>>> @@ -1137,12 +1137,12 @@ ComputeLocalClient(ClientPtr client)
>>>          /* Cut off any colon and whatever comes after it, see
>>>           * https://lists.freedesktop.org/archives/xorg-devel/2015-December/048164.html
>>>           */
>>> -        cmd = strtok(cmd, ":");
>>> +        char *tok = strtok(cmd, ":");
>>>
>>>  #if !defined(WIN32) || defined(__CYGWIN__)
>>> -        ret = strcmp(basename(cmd), "ssh") != 0;
>>> +        ret = strcmp(basename(tok), "ssh") != 0;
>>>  #else
>>> -        ret = strcmp(cmd, "ssh") != 0;
>>> +        ret = strcmp(tok, "ssh") != 0;
>>>  #endif
>>>
>>>          free(cmd);
>> _______________________________________________
>> xorg-devel at lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>
>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer