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
