On Thu, Aug 22, 2013 at 07:27:07PM +0200, Jakub Hrozek wrote:
On Thu, Aug 22, 2013 at 10:18:36AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> On 08/22/2013 10:11 AM, Stephen Gallagher wrote:
> > On 08/15/2013 11:50 AM, Stephen Gallagher wrote:
> >> There was duplicated code in cc_file_check_existing() and in
> >> cc_dir_check_existing(). I pulled them into the same function.
> >> There are two changes made to the original code here: 1) Fixes a
> >> use-after-free bug in cc_file_check_existing(). In the original
> >> code, we called krb5_free_context() and then used that context
> >> immediately after that in krb5_cc_close(). This patch corrects
> >> the ordering
> >> 2) The krb5_cc_resolve() call handles KRB5_FCC_NOFILE for all
> >> cache types. Previously, this was only handled for DIR caches.
> >> This second part I need someone with Kerberos knowledge to
> >> verify. Is there a risk of receiving this error for the FILE or
> >> KEYRING types, and if so is this handling still acceptable or
> >> should they be special-cased?
> > Self-nack. New patch attached (I was never actually returning the
> > validity result).
> > Interdiff:
> > diff --git a/src/providers/krb5/krb5_utils.c
> > b/src/providers/krb5/krb5_utils.c index 47c45d0..8166435 100644 ---
> > a/src/providers/krb5/krb5_utils.c +++
> > b/src/providers/krb5/krb5_utils.c @@ -750,6 +750,7 @@
> > check_cc_validity(const char *location, }
> > ret = EOK; + *_valid = valid;
> > done: if (ccache) krb5_cc_close(context, ccache);
> Adding another consistency patch that goes along with this:
> Only set active and valid on success
> The FILE cache only sets the return values of _active and _bool if the
> entire function succeeds. The DIR cache was setting it even on failure.
> This patch makes both consistent. This will benefit static analysis
> tools which would be able to detect if the variable is ever used
> uninitialized anywhere.
> Both patches are attached for simplicity.
ACK to both. I tested that the ccache is reused if exists and recreated
if doesn't exist for both DIR and FILE ccaches.
Pushed to master and sssd-1-10