[PATCH xserver v2.1] os: Treat ssh as a non-local client (v2)

[Martin Peres]

On 11/12/15 11:30, Michel Dänzer wrote:
> On 11.12.2015 18:17, Martin Peres wrote:
>> On 11/12/15 04:34, Michel Dänzer wrote:
>>> From: Adam Jackson <ajax at redhat.com>
>>>
>>> By the time we get to ComputeLocalClient, we've already done
>>> NextAvailableClient → ReserveClientIds → DetermineClientCmd (assuming
>>> we're built with #define CLIENTIDS), so we can look up the name of the
>>> client process and refuse to treat ssh's X forwarding as if it were
>>> local.
>>>
>>> v2: (Michel Dänzer)
>>>       * Only match "ssh" itself, not other executable names starting with
>>>         that prefix.
>>>       * Ignore executable path for the match.
>>>
>>> Signed-off-by: Adam Jackson <ajax at redhat.com>
>>> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
>>> ---
>>>
>>> v2.1: Slightly extended code comment
>>>
>>>    os/access.c | 29 ++++++++++++++++++++++++++---
>>>    1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/os/access.c b/os/access.c
>>> index 10a48c3..3e32128 100644
>>> --- a/os/access.c
>>> +++ b/os/access.c
>>> @@ -101,6 +101,7 @@ SOFTWARE.
>>>    #include <sys/socket.h>
>>>    #include <sys/ioctl.h>
>>>    #include <ctype.h>
>>> +#include <libgen.h>
>>>      #ifndef NO_LOCAL_CLIENT_CRED
>>>    #include <pwd.h>
>>> @@ -1081,9 +1082,8 @@ ResetHosts(const char *display)
>>>        }
>>>    }
>>>    -/* Is client on the local host */
>>> -Bool
>>> -ComputeLocalClient(ClientPtr client)
>>> +static Bool
>>> +xtransLocalClient(ClientPtr client)
>>>    {
>>>        int alen, family, notused;
>>>        Xtransaddr *from = NULL;
>>> @@ -1116,6 +1116,29 @@ ComputeLocalClient(ClientPtr client)
>>>        return FALSE;
>>>    }
>>>    +/* Is client on the local host */
>>> +Bool
>>> +ComputeLocalClient(ClientPtr client)
>>> +{
>>> +    if (!xtransLocalClient(client))
>>> +        return FALSE;
>>> +
>>> +#ifndef WIN32
>> Why add this ifndef?
> Because libgen.h and basename() aren't available on Windows AFAICT. I
> figured this use case probably isn't common enough on Windows anyway to
> bother making it work there as well.

You are right, basename is a POSIX function and mingw does expose it, 
but it turns out msvc does not. As you said, it is probably not worth 
the effort to add msvc support that no-one will be able to test anyway.

>
>> And why all clients of an xserver running on windows should be
>> considered local?
> You misread the patch. There's no change in behaviour on Windows,
> because xtransLocalClient and by extension ComputeLocalClient returns
> FALSE in all the same cases ComputeLocalClient did before the patch.

Yes, indeed. The added code is just filtering one more case.

Reviewed-by: Martin Peres <martin.peres at linux.intel.com>

>
>
>>> +    /* If the executable name is "ssh", assume that this client
>>> connection
>>> +     * is forwarded from another host via SSH
>>> +     */
>>> +    if (client->clientIds->cmdname) {
>>> +        char *cmdname = strdup(client->clientIds->cmdname);
>>> +        Bool ret = strcmp(basename(cmdname), "ssh") != 0;
>>> +
>>> +        free(cmdname);
>>> +        return ret;
>>> +    }
>>> +#endif
>>> +
>>> +    return TRUE;
>>> +}
>>> +
>>>    /*
>>>     * Return the uid and all gids of a connected local client
>>>     * Allocates a LocalClientCredRec - caller must call
>>> FreeLocalClientCreds
>