Re: [PATCH] Check for AD style machine principal name

Re: [PATCH] Check for AD style machine principal name

[Timo Aaltonen]

CC:ing the "new" list.

On Tue, 27 Apr 2010, J. Bruce Fields wrote:

> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org wrote:
>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context, krb5_keytab kt,
>>  }
>>
>>  /*
>> + * Convert the hostname to machine principal name as created
>> + * by MS Active Directory.
>> +*/
>> +
>> +static char *
>> +hostname_to_adprinc(char *name)
>> +{
>> +	int i = 0;
>> +	char *buf = strdup(name);
>
> We should handle the possible NULL return.

How about not calling hostname_to_adprinc from find_keytab_entry unless 
myhostname is != NULL?

>> +	int len = strlen(name);
>> +	while(i < len) {
>> +		buf[i] = toupper(buf[i]);
>> +		i++;
>> +	}
>> +	buf[i++] = '$';
>> +	buf[i] = 0;
>
> This is past the end of the buffer.  Maybe you want buf to be
> strlen(name)+1?

Ok, so:
         char *buf = malloc(sizeof(name)+1);
 	buf = strdup(name);
 	.
 	.
works, and should be ok?

>> @@ -812,6 +836,35 @@ find_keytab_entry(krb5_context context, krb5_keytab kt, const char *hostname,
>>  			break;
>>  		if (strcmp(realm, default_realm) == 0)
>>  			tried_default = 1;
>> +		/* First try the Active Directory style machine principal */
>> +		code = krb5_build_principal_ext(context, &princ,
>> +						strlen(realm),
>> +						realm,
>> +						strlen(adprinc),
>> +						adprinc,
>> +						NULL);
>> +		if (code) {
>> +			k5err = gssd_k5_err_msg(context, code);
>> +			printerr(1, "%s while building principal for "
>> +				 "'%s@%s'\n", k5err,
>> +				 adprinc, realm);
>> +			continue;
>
> Is there an infinite loop here?

Not to my knowledge, it's basically the same chunk of code copied from 
a few lines below, modified to suit the adprinc.


-- 
Timo Aaltonen
Systems Specialist
IT Services, Aalto University

Re: [PATCH] Check for AD style machine principal name

[J. Bruce Fields]

On Wed, May 05, 2010 at 07:53:33PM +0300, Timo Aaltonen wrote:
>
> CC:ing the "new" list.
>
> On Tue, 27 Apr 2010, J. Bruce Fields wrote:
>
>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org wrote:
>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context, krb5_keytab kt,
>>>  }
>>>
>>>  /*
>>> + * Convert the hostname to machine principal name as created
>>> + * by MS Active Directory.
>>> +*/
>>> +
>>> +static char *
>>> +hostname_to_adprinc(char *name)
>>> +{
>>> +	int i = 0;
>>> +	char *buf = strdup(name);
>>
>> We should handle the possible NULL return.
>
> How about not calling hostname_to_adprinc from find_keytab_entry unless  
> myhostname is != NULL?

I'm not sure what you mean.  I'm just pointing out that strdup() can
return NULL if the memory allocation fails.

>
>>> +	int len = strlen(name);
>>> +	while(i < len) {
>>> +		buf[i] = toupper(buf[i]);
>>> +		i++;
>>> +	}
>>> +	buf[i++] = '$';
>>> +	buf[i] = 0;
>>
>> This is past the end of the buffer.  Maybe you want buf to be
>> strlen(name)+1?
>
> Ok, so:
>         char *buf = malloc(sizeof(name)+1);
> 	buf = strdup(name);
> 	.
> 	.
> works, and should be ok?

The strdup should be a strcpy.

>
>>> @@ -812,6 +836,35 @@ find_keytab_entry(krb5_context context, krb5_keytab kt, const char *hostname,
>>>  			break;
>>>  		if (strcmp(realm, default_realm) == 0)
>>>  			tried_default = 1;
>>> +		/* First try the Active Directory style machine principal */
>>> +		code = krb5_build_principal_ext(context, &princ,
>>> +						strlen(realm),
>>> +						realm,
>>> +						strlen(adprinc),
>>> +						adprinc,
>>> +						NULL);
>>> +		if (code) {
>>> +			k5err = gssd_k5_err_msg(context, code);
>>> +			printerr(1, "%s while building principal for "
>>> +				 "'%s@%s'\n", k5err,
>>> +				 adprinc, realm);
>>> +			continue;
>>
>> Is there an infinite loop here?
>
> Not to my knowledge,

We need a more convincing argument than that, unless someone else has
the cycles to take this over!

--b.

> it's basically the same chunk of code copied from a 
> few lines below, modified to suit the adprinc.
>
>
> -- 
> Timo Aaltonen
> Systems Specialist
> IT Services, Aalto University

Re: [PATCH] Check for AD style machine principal name

[Kevin Coffman]

On Wed, May 5, 2010 at 12:53 PM, Timo Aaltonen <timo.aaltonen-uOixanVlb7U@public.gmane.org>=
 wrote:
>
> CC:ing the "new" list.
>
> On Tue, 27 Apr 2010, J. Bruce Fields wrote:
>
>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org wro=
te:
>>>
>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context,
>>> krb5_keytab kt,
>>> =A0}
>>>
>>> =A0/*
>>> + * Convert the hostname to machine principal name as created
>>> + * by MS Active Directory.
>>> +*/
>>> +
>>> +static char *
>>> +hostname_to_adprinc(char *name)
>>> +{
>>> + =A0 =A0 =A0 int i =3D 0;
>>> + =A0 =A0 =A0 char *buf =3D strdup(name);
>>
>> We should handle the possible NULL return.
>
> How about not calling hostname_to_adprinc from find_keytab_entry unle=
ss
> myhostname is !=3D NULL?

I think he meant that strdup() may fail, and return NULL.  That has to
be handled here, and a possible resulting NULL return from
host_to_adprinc() should also be handled below.

>>> + =A0 =A0 =A0 int len =3D strlen(name);
>>> + =A0 =A0 =A0 while(i < len) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 buf[i] =3D toupper(buf[i]);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 i++;
>>> + =A0 =A0 =A0 }
>>> + =A0 =A0 =A0 buf[i++] =3D '$';
>>> + =A0 =A0 =A0 buf[i] =3D 0;
>>
>> This is past the end of the buffer. =A0Maybe you want buf to be
>> strlen(name)+1?
>
> Ok, so:
> =A0 =A0 =A0 =A0char *buf =3D malloc(sizeof(name)+1);
> =A0 =A0 =A0 =A0buf =3D strdup(name);
> =A0 =A0 =A0 =A0.
> =A0 =A0 =A0 =A0.
> works, and should be ok?
>
>>> @@ -812,6 +836,35 @@ find_keytab_entry(krb5_context context, krb5_k=
eytab
>>> kt, const char *hostname,
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (strcmp(realm, default_realm) =3D=
=3D 0)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tried_default =3D 1;
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* First try the Active Directory sty=
le machine principal
>>> */
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 code =3D krb5_build_principal_ext(con=
text, &princ,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 strlen(realm),
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 realm,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 strlen(adprinc),
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 adprinc,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 NULL);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (code) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 k5err =3D gssd_k5_err=
_msg(context, code);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printerr(1, "%s while=
 building principal for "
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"'=
%s@%s'\n", k5err,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ad=
princ, realm);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
>>
>> Is there an infinite loop here?
>
> Not to my knowledge, it's basically the same chunk of code copied fro=
m a few
> lines below, modified to suit the adprinc.

The chunk you copied was within a for() loop.  You are calling
continue within the while(1) loop.

Re: [PATCH] Check for AD style machine principal name

[Timo Aaltonen]

On Wed, 5 May 2010, Kevin Coffman wrote:

> On Wed, May 5, 2010 at 12:53 PM, Timo Aaltonen <timo.aaltonen@aalto.f=
i> wrote:
>>
>> CC:ing the "new" list.
>>
>> On Tue, 27 Apr 2010, J. Bruce Fields wrote:
>>
>>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org wr=
ote:
>>>>
>>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context,
>>>> krb5_keytab kt,
>>>> =A0}
>>>>
>>>> =A0/*
>>>> + * Convert the hostname to machine principal name as created
>>>> + * by MS Active Directory.
>>>> +*/
>>>> +
>>>> +static char *
>>>> +hostname_to_adprinc(char *name)
>>>> +{
>>>> + =A0 =A0 =A0 int i =3D 0;
>>>> + =A0 =A0 =A0 char *buf =3D strdup(name);
>>>
>>> We should handle the possible NULL return.
>>
>> How about not calling hostname_to_adprinc from find_keytab_entry unl=
ess
>> myhostname is !=3D NULL?
>
> I think he meant that strdup() may fail, and return NULL.  That has t=
o
> be handled here, and a possible resulting NULL return from
> host_to_adprinc() should also be handled below.

Ahh.. ok so how about this:

static char *
hostname_to_adprinc(char *name)
{
         int i =3D 0;
         int len =3D strlen(name);
         char *buf;
         if ((buf =3D malloc(len+1))) {
                 strcpy(buf, name);
                 while(i < len) {
                         buf[i] =3D toupper(buf[i]);
                         i++;
                 }
                 buf[i++] =3D '$';
                 buf[i] =3D 0;
                 return buf;
         }
         return NULL;
}

and then adding 'if (adprinc !=3D NULL) { ... }' to the while loop in=20
find_keytab_entry. That passes my tests.


>>>> @@ -812,6 +836,35 @@ find_keytab_entry(krb5_context context, krb5_=
keytab
>>>> kt, const char *hostname,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (strcmp(realm, default_realm) =3D=
=3D 0)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tried_default =3D 1=
;
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* First try the Active Directory st=
yle machine principal
>>>> */
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 code =3D krb5_build_principal_ext(co=
ntext, &princ,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 strlen(realm),
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 realm,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 strlen(adprinc),
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 adprinc,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 NULL);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (code) {
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 k5err =3D gssd_k5_er=
r_msg(context, code);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printerr(1, "%s whil=
e building principal for "
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"=
'%s@%s'\n", k5err,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0a=
dprinc, realm);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
>>>
>>> Is there an infinite loop here?
>>
>> Not to my knowledge, it's basically the same chunk of code copied fr=
om a few
>> lines below, modified to suit the adprinc.
>
> The chunk you copied was within a for() loop.  You are calling
> continue within the while(1) loop.

Ok, so 'continue' was extra, whoops..


--=20
Timo Aaltonen
Systems Specialist
IT Services, Aalto University

RE: [PATCH] Check for AD style machine principal name

[Staubach_Peter]

-----Original Message-----
=46rom: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-owner-fy+rA21nqHI@public.gmane.org=
rnel.org] On Behalf Of Timo Aaltonen
Sent: Wednesday, May 05, 2010 6:16 PM
To: Kevin Coffman
Cc: J. Bruce Fields; linux-nfs@vger.kernel.org
Subject: Re: [PATCH] Check for AD style machine principal name

On Wed, 5 May 2010, Kevin Coffman wrote:

> On Wed, May 5, 2010 at 12:53 PM, Timo Aaltonen <timo.aaltonen@aalto.f=
i> wrote:
>>
>> CC:ing the "new" list.
>>
>> On Tue, 27 Apr 2010, J. Bruce Fields wrote:
>>
>>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org wr=
ote:
>>>>
>>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context,
>>>> krb5_keytab kt,
>>>> =A0}
>>>>
>>>> =A0/*
>>>> + * Convert the hostname to machine principal name as created
>>>> + * by MS Active Directory.
>>>> +*/
>>>> +
>>>> +static char *
>>>> +hostname_to_adprinc(char *name)
>>>> +{
>>>> + =A0 =A0 =A0 int i =3D 0;
>>>> + =A0 =A0 =A0 char *buf =3D strdup(name);
>>>
>>> We should handle the possible NULL return.
>>
>> How about not calling hostname_to_adprinc from find_keytab_entry unl=
ess
>> myhostname is !=3D NULL?
>
> I think he meant that strdup() may fail, and return NULL.  That has t=
o
> be handled here, and a possible resulting NULL return from
> host_to_adprinc() should also be handled below.

Ahh.. ok so how about this:

static char *
hostname_to_adprinc(char *name)
{
         int i =3D 0;
         int len =3D strlen(name);
         char *buf;
         if ((buf =3D malloc(len+1))) {

> Doesn't this need to be "+2" since you are adding a '$' and a '\0' to=
 the end of the string, neither of which is included in the strlen() ca=
lculation?

                 strcpy(buf, name);
                 while(i < len) {
                         buf[i] =3D toupper(buf[i]);
                         i++;
                 }

> Instead of copying name to buf and then converting the characters in =
buf to uppercase, why not copy and convert in one loop?

                 buf[i++] =3D '$';
                 buf[i] =3D 0;
                 return buf;
         }
         return NULL;
}

and then adding 'if (adprinc !=3D NULL) { ... }' to the while loop in=20
find_keytab_entry. That passes my tests.


>>>> @@ -812,6 +836,35 @@ find_keytab_entry(krb5_context context, krb5_=
keytab
>>>> kt, const char *hostname,
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (strcmp(realm, default_realm) =3D=
=3D 0)
>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tried_default =3D 1=
;
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* First try the Active Directory st=
yle machine principal
>>>> */
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 code =3D krb5_build_principal_ext(co=
ntext, &princ,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 strlen(realm),
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 realm,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 strlen(adprinc),
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 adprinc,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 NULL);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (code) {
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 k5err =3D gssd_k5_er=
r_msg(context, code);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printerr(1, "%s whil=
e building principal for "
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"=
'%s@%s'\n", k5err,
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0a=
dprinc, realm);
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
>>>
>>> Is there an infinite loop here?
>>
>> Not to my knowledge, it's basically the same chunk of code copied fr=
om a few
>> lines below, modified to suit the adprinc.
>
> The chunk you copied was within a for() loop.  You are calling
> continue within the while(1) loop.

Ok, so 'continue' was extra, whoops..


--=20
Timo Aaltonen
Systems Specialist
IT Services, Aalto University
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Check for AD style machine principal name

[Timo Aaltonen]

On Thu, 6 May 2010, Staubach_Peter@emc.com wrote:

> -----Original Message-----
> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-owner-fy+rA21nqHI@public.gmane.org=
rnel.org] On Behalf Of Timo Aaltonen
> Sent: Wednesday, May 05, 2010 6:16 PM
> To: Kevin Coffman
> Cc: J. Bruce Fields; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH] Check for AD style machine principal name
>
> On Wed, 5 May 2010, Kevin Coffman wrote:
>
>> On Wed, May 5, 2010 at 12:53 PM, Timo Aaltonen <timo.aaltonen@aalto.=
fi> wrote:
>>>
>>> CC:ing the "new" list.
>>>
>>> On Tue, 27 Apr 2010, J. Bruce Fields wrote:
>>>
>>>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org w=
rote:
>>>>>
>>>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context=
,
>>>>> krb5_keytab kt,
>>>>> =A0}
>>>>>
>>>>> =A0/*
>>>>> + * Convert the hostname to machine principal name as created
>>>>> + * by MS Active Directory.
>>>>> +*/
>>>>> +
>>>>> +static char *
>>>>> +hostname_to_adprinc(char *name)
>>>>> +{
>>>>> + =A0 =A0 =A0 int i =3D 0;
>>>>> + =A0 =A0 =A0 char *buf =3D strdup(name);
>>>>
>>>> We should handle the possible NULL return.
>>>
>>> How about not calling hostname_to_adprinc from find_keytab_entry un=
less
>>> myhostname is !=3D NULL?
>>
>> I think he meant that strdup() may fail, and return NULL.  That has =
to
>> be handled here, and a possible resulting NULL return from
>> host_to_adprinc() should also be handled below.
>
> Ahh.. ok so how about this:
>
> static char *
> hostname_to_adprinc(char *name)
> {
>         int i =3D 0;
>         int len =3D strlen(name);
>         char *buf;
>         if ((buf =3D malloc(len+1))) {
>
>> Doesn't this need to be "+2" since you are adding a '$' and a '\0' t=
o the end of the string, neither of which is included in the strlen() c=
alculation?

Yep, you're right. Shouldn't adding to the buffer fail then, when the=20
malloc leaves the string short?

>                 strcpy(buf, name);
>                 while(i < len) {
>                         buf[i] =3D toupper(buf[i]);
>                         i++;
>                 }
>
>> Instead of copying name to buf and then converting the characters in=
 buf to uppercase, why not copy and convert in one loop?

This
                          buf[i] =3D toupper(name[i]);

works fine, thanks :)

--=20
Timo Aaltonen
Systems Specialist
IT Services, Aalto University

RE: [PATCH] Check for AD style machine principal name

[Staubach_Peter]

-----Original Message-----
=46rom: Timo Aaltonen [mailto:timo.aaltonen-uOixanVlb7U@public.gmane.org]=20
Sent: Wednesday, May 05, 2010 6:34 PM
To: Staubach, Peter
Cc: timo.aaltonen-uOixanVlb7U@public.gmane.org; kwc@citi.umich.edu; bfields@fieldses.org; l=
inux-nfs@vger.kernel.org
Subject: RE: [PATCH] Check for AD style machine principal name

On Thu, 6 May 2010, Staubach_Peter@emc.com wrote:

> -----Original Message-----
> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-owner-fy+rA21nqHI@public.gmane.org=
rnel.org] On Behalf Of Timo Aaltonen
> Sent: Wednesday, May 05, 2010 6:16 PM
> To: Kevin Coffman
> Cc: J. Bruce Fields; linux-nfs@vger.kernel.org
> Subject: Re: [PATCH] Check for AD style machine principal name
>
> On Wed, 5 May 2010, Kevin Coffman wrote:
>
>> On Wed, May 5, 2010 at 12:53 PM, Timo Aaltonen <timo.aaltonen@aalto.=
fi> wrote:
>>>
>>> CC:ing the "new" list.
>>>
>>> On Tue, 27 Apr 2010, J. Bruce Fields wrote:
>>>
>>>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen-uOixanVlb7U@public.gmane.org w=
rote:
>>>>>
>>>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context=
,
>>>>> krb5_keytab kt,
>>>>> =A0}
>>>>>
>>>>> =A0/*
>>>>> + * Convert the hostname to machine principal name as created
>>>>> + * by MS Active Directory.
>>>>> +*/
>>>>> +
>>>>> +static char *
>>>>> +hostname_to_adprinc(char *name)
>>>>> +{
>>>>> + =A0 =A0 =A0 int i =3D 0;
>>>>> + =A0 =A0 =A0 char *buf =3D strdup(name);
>>>>
>>>> We should handle the possible NULL return.
>>>
>>> How about not calling hostname_to_adprinc from find_keytab_entry un=
less
>>> myhostname is !=3D NULL?
>>
>> I think he meant that strdup() may fail, and return NULL.  That has =
to
>> be handled here, and a possible resulting NULL return from
>> host_to_adprinc() should also be handled below.
>
> Ahh.. ok so how about this:
>
> static char *
> hostname_to_adprinc(char *name)
> {
>         int i =3D 0;
>         int len =3D strlen(name);
>         char *buf;
>         if ((buf =3D malloc(len+1))) {
>
>> Doesn't this need to be "+2" since you are adding a '$' and a '\0' t=
o the end of the string, neither of which is included in the strlen() c=
alculation?

Yep, you're right. Shouldn't adding to the buffer fail then, when the=20
malloc leaves the string short?

>> Nope, you just step on memory that potentially belongs to something =
else, causing corruption.

		ps


>                 strcpy(buf, name);
>                 while(i < len) {
>                         buf[i] =3D toupper(buf[i]);
>                         i++;
>                 }
>
>> Instead of copying name to buf and then converting the characters in=
 buf to uppercase, why not copy and convert in one loop?

This
                          buf[i] =3D toupper(name[i]);

works fine, thanks :)

--=20
Timo Aaltonen
Systems Specialist
IT Services, Aalto University