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

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.

I only got as far as that before running out of time today. Can you fix
those please and go through the rest of the xfree/safe_free changes and
make sure that the other files are similarly optimized.
As a guide:
  * xfree() is faster and should be preferred over safe_free() when
possible.
  * but safe_free() is required if the variable or member is possibly
going to be read later in the code without being set to a new value.

Also, FYI in C++ variables may be declared at point of first use or
inside any {} to increase compiler checks usefulness. We are making use
of that property extensively in new Squid code to harden local variables
and assist with ensuring guarantees like variables with undefined
contents not being re-used accidentally outside their intended scope.
You may want to consider polishing up some of the long functions in
support_*.cc to make use of the sub-scopes.

Amos
Received on Sat Sep 21 2013 - 16:27:51 MDT