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

[Michel Dänzer]

On 2017-12-13 04:43 PM, Adam Jackson wrote:
> On Tue, 2017-12-12 at 22:18 +0100, Tomasz Śniatowski wrote:
>> On 8 December 2017 at 16:46, Michel Dänzer  wrote:
>>>
>>> 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 
>>
>> Forgive my ignorance, am I supposed to do something now to get this merged?
> 
> Merged to master and 1.19 branch, thanks!

Thanks Adam, sorry, been busy with other stuff.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Adam Jackson]

On Tue, 2017-12-12 at 22:18 +0100, Tomasz Śniatowski wrote:
> On 8 December 2017 at 16:46, Michel Dänzer  wrote:
> > 
> > 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 
> 
> Forgive my ignorance, am I supposed to do something now to get this merged?

Merged to master and 1.19 branch, thanks!

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Tomasz Śniatowski]

On 8 December 2017 at 16:46, Michel Dänzer  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 

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

>>> Signed-off-by: Tomasz Śniatowski 
>>> ---
>>>  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@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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[Michel Dänzer]

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 ?

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 


>> Signed-off-by: Tomasz Śniatowski 
>> ---
>>  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@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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[walter harms]

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 ?

re,
 wh

> Signed-off-by: Tomasz Śniatowski 
> ---
>  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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

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

[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

Signed-off-by: Tomasz Śniatowski 
---
 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);
-- 
2.15.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel