More memory leaks (1.6.4-beta1 release)

[Marcus Watts]

Dan writes:
> Date:    Thu, 12 Nov 2009 15:18:57 GMT
> To:      Markus Moeller <huaraz at moeller.plus.com>
> cc:      krbdev at mit.edu
> From:    Dan Searle <dan.searle at censornet.com>
> Subject: Re: More memory leaks (1.6.4-beta1 release)
> 
> Hi guys,
> 
> I've managed to come up with a patch to the krb5-1.6.4-beta1 release 
> that fixes the critical still reachable memory leaks, please see the 
> attached file. I would appreciate comments as my "fixes" may have other 
> side effects I'm unaware of.
> 
> Regards, Dan...

At first blush, your patch has the right kind of logic in the right kinds
of places.  It's exactly the sort of patch to send in - most localized
fix & all. (except I'd omit the "#warning"s.)  But this does make a nice
example of how resource leakage happens.

src/lib/gssapi/krb5/acquire_cred.c
acquire_accept_cred()
Your code to fix the return at 196 is very like the logic
at lines 182-187.  Except you call kt_close in a different
order.  (At the least, I'd move that just for the sake
	of sanity).

<start coding style rant>

As a general result, I would try to code it this way:

	krb5_keytab kt = NULL;
	OM_uint32 result = GSS_S_FAILURE;
...
	if (((code = krb5_kt_get_entry(...
		*minor_status = ...
		result = GSS_S_CRED_UNAVAIL;
		goto fail;
...
	/* hooray.  we made it */
	cred->keytab = kt;
	kt = 0;
	result = GSS_S_COMPLETE;
fail:
	if (kt) krb5_kt_close(context, kt);
	return result;

By making sure all the code goes through the "fail" logic
at the end, and "hiding" resources that are passed elsewhere,
I can make sure that I only need one copy of "postlogue" code,
and I get greater assurance that my code won't leak in some
odd wonderful way I hadn't tested.

And then there's
src/lib/gssapi/krb5/accept_sec_context.c
krb5_gss_accept_sec_context()

This logic does have the common exit strategy - but as you found it
didn't help.  This code very likely has other problems - trivial example:
the return at 982 fails to free scratch like at line 989.
It could benefit from having separate "error" and "cleanup" labels,
and having fewer returns at the end.  This would not be a simple task.

<end coding style rant>

				-Marcus Watts