Re: [PATCH] Fix leaks and check for newer libraries in Kerberos related helpers

On 22/09/2013 4:27 a.m., Amos Jeffries wrote:
> On 26/08/2013 3:17 a.m., Markus Moeller wrote:
>> Hi
>> please find a patch for squid 3.4 trunk for:
>>
>> peer_proxy_negotiate_auth.cc
>> negotiate_auth/kerberos
>> external_acl/kerberos_ldap_group
>>
>> Please ignore my previous patch.
>>
>> Thank you
>> Markus
>
> In helpers/external_acl/kerberos_ldap_group/kerberos_ldap_group.cc
> * There are some if-conditions which look like thay are wrongly being
> converted to safe_free().
> The first is:
> - if (p == gdsp) {
> - xfree(gdsp);
> - gdsp = NULL;
> - }
> + safe_free(gdsp);
> p = gdsp;
>
> these will cause the loop to exit after freeing only one entry as
> gdsp gets unconditionally free+NULL'd and p set to NULL via the
> resulting gdsp value.
>
> * The same issue exists in the ndsp and lssp blocks below that.
>
>
> In helpers/external_acl/kerberos_ldap_group/support_group.cc
> * there are still a number of unnecessary safe_free() conversions done
> on local variables before return statements.
>
>
> In helpers/external_acl/kerberos_ldap_group/support_krb5.cc
> * the xfree(service) can stay as xfree(service) but without the
> if(service) conditional.
> * The tgt_creds and creds code for krb5_free*() should look like this
> (note the {} positioning to allow optimized skipping of the z=NULL
> assignment):
>
> + if (tgt_creds) {
> + krb5_free_creds(kparam.context, tgt_creds);
> + tgt_creds = NULL;
> + }
>
> ++ the tgt_creds appears like it can be made local to the "if
> (!principal_name) {" code block and does not need setting to NULL
> after free.
>
> * in the krb5_create_cache() "cleanup:" section most of the xfree()
> were correct, but had unnecessary if() conditions. Now they have
> unnecessary =NULL assignment from the safe_free().
>
>
> In helpers/external_acl/kerberos_ldap_group/support_ldap.cc
> * the xfree(attr_value[j]); in for-loop was correct.

Continuing from there but ignoring the safe_free() for now ...

In helpers/negotiate_auth/kerberos/negotiate_kerberos_auth.cc:
* the new parameter on check_gss_err() is better typed as a "bool"
  NP: we have portability code ensuring that bool type always exists so
you don't have to worry about bool/BOOL/Boolean differences

* use of defined() around HAVE_* macros is not a good idea. They can be
defined to 0 for false on some systems.
  They are however guarateed to always represent a true/false value to
the precompiler so should be used "bare" in the #if conditions.
  - #if (defined(HAVE_GSSKRB5_EXTRACT_AUTHZ_DATA_FROM_SEC_CONTEXT) ||
defined(HAVE_GSS_MAP_NAME_TO_ANY)) && HAVE_KRB5_PAC
+ #if (HAVE_GSSKRB5_EXTRACT_AUTHZ_DATA_FROM_SEC_CONTEXT ||
HAVE_GSS_MAP_NAME_TO_ANY) && HAVE_KRB5_PAC

same defined() problem in src/peer_proxy_negotiate_auth.cc
   -#if defined(HAVE_HEIMDAL_KERBEROS) &&
!defined(HAVE_KRB5_GET_RENEWED_CREDS)
+#if HAVE_HEIMDAL_KERBEROS && !HAVE_KRB5_GET_RENEWED_CREDS

Other than all them styling issues it looks okay to me.

Amos
Received on Sun Sep 22 2013 - 11:26:03 MDT