[PATCH] xauth: improve to handle FamilyWild necessary for GDM/XDMCP/SSH. #43425

[walter harms]

Am 19.12.2011 10:23, schrieb Dr. Tilmann Bubeck:
> Hello xorg,
> 
> I sent you the mail below a few days ago with a patch for xauth.
> According to
> http://www.x.org/wiki/Development/Documentation/SubmittingPatches I am
> now pinging the list again. I do not know any maintainer to CC, so I
> only wrote to this list.
> 
> I would be glad if this patch gets included.
> 
> Also feel free to contact me, if anything has to be changed.
> 
> Kind regards,
>   Till
> 
> 
> 
> 
> On Thu, 01 Dec 2011 17:44:02 +0100, Dr. Tilmann Bubeck
> <t.bubeck at reinform.de> wrote:
> 
>> From: "Dr. Tilmann Bubeck" <t.bubeck at reinform.de>
>>
>> xauth is currently unable to handle FamilyWild. This gives problems
>> with GDM receiving XDMCP request which used FamilyWild. More details
>> in the referenced freedesktop bugzilla entry.
>>
>> The patch improves xauth to handle that Family:
>>   * allow "dump_entry" to deal with that Family and output
>>     such entries correctly.
>>   * allow "list $DISPLAY" to match against an entry in
>>     XAUTHORITY of type FamilyWild.
>>
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=43425
>>
>> Signed-off-by: Dr. Tilmann Bubeck <t.bubeck at reinform.de>
>> ---
>>  process.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 files changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/process.c b/process.c
>> index 283b4a1..651bea0 100644
>> --- a/process.c
>> +++ b/process.c
>> @@ -462,6 +462,9 @@ read_auth_entries(FILE *fp, Bool numeric, AuthList
>> **headp, AuthList **tailp)
>>      return n;
>>  }
>> +/**
>> + * Parse the given displayname and build a corresponding AuthList.
>> + */
>>  static Bool
>>  get_displayname_auth(const char *displayname, AuthList **authl)
>>  {
>> @@ -991,6 +994,9 @@ dump_entry(const char *inputfilename, int lineno,
>> Xauth *auth, char *data)
>>          fwrite (auth->address, sizeof (char), auth->address_length, fp);
>>          fprintf (fp, "/unix");
>>          break;
>> +        case FamilyWild:
>> +        fwrite (auth->address, sizeof (char), auth->address_length, fp);
>> +        break;
>>        case FamilyInternet:
>>  #if defined(IPv6) && defined(AF_INET6)
>>        case FamilyInternet6:
>> @@ -1073,6 +1079,32 @@ match_auth_dpy(register Xauth *a, register
>> Xauth *b)
>>           memcmp(a->number, b->number, a->number_length) == 0) ? 1 : 0);
>>  }
>> +static int
>> +match_authwild_dpy(register Xauth *a, const char *displayname)
>> +{
>> +    int family;
>> +    char *host = NULL, *rest = NULL;
>> +    int dpynum, scrnum;
>> +    char dpynumbuf[40];            /* want to hold largest display
>> num */
>> +
>> +    if ( a->family != FamilyWild ) {
>> +    return False;
>> +    }
>> +
>> +    if (!parse_displayname (displayname,
>> +                &family, &host, &dpynum, &scrnum, &rest)) {
>> +    return False;
>> +    }

 i do not know parse_displayname() but i guess it will allocate
 memory for host and rest. I do not see a free()
 memory leak ?

>> +
>> +    dpynumbuf[0] = '\0';
>> +    sprintf (dpynumbuf, "%d", dpynum);
>> +
>> +    return ((a->address_length == strlen(host) &&
>> +         a->number_length == strlen(dpynumbuf) &&
>> +         memcmp(a->address, host, a->address_length) == 0 &&
>> +         memcmp(a->number, dpynumbuf, a->number_length) == 0) ? 1 : 0);
>> +}
>> +

i have a problem with these memcp() block.
could you entangle it a bit to improve readability ?
like

 if ( a->address_length != strlen(host) || a->number_length == strlen(dpynumbuf) )
	return False;

 if (	memcmp(a->address, host, a->address_length) == 0 &&
        memcmp(a->number, dpynumbuf, a->number_length) == 0) )
 	return True;
 else
	return False;

	

hope that helps,

re,
 wh

>>  /* return non-zero iff display and authorization type are the same */
>> static int
>> @@ -1236,13 +1268,22 @@ iterdpy (const char *inputfilename, int
>> lineno, int start,
>>          /* l may be freed by remove_entry below. so save its contents */
>>          next = l->next;
>>          tmp_auth = copyAuth(l->auth);
>> -        for (proto = proto_head; proto; proto = proto->next) {
>> -        if (match_auth_dpy (proto->auth, tmp_auth)) {
>> -            matched = True;
>> -            if (yfunc) {
>> -            status = (*yfunc) (inputfilename, lineno,
>> -                       tmp_auth, data);
>> -            if (status < 0) break;
>> +
>> +        if ( match_authwild_dpy(tmp_auth, displayname) ) {
>> +            matched = True;
>> +        if (yfunc) {
>> +            status = (*yfunc) (inputfilename, lineno,
>> +                       tmp_auth, data);
>> +        }
>> +        } else {
>> +            for (proto = proto_head; proto; proto = proto->next) {
>> +            if (match_auth_dpy (proto->auth, tmp_auth)) {
>> +                matched = True;
>> +                if (yfunc) {
>> +                status = (*yfunc) (inputfilename, lineno,
>> +                              tmp_auth, data);
>> +                if (status < 0) break;
>> +            }
>>              }
>>          }
>>          }
> 
>