Probable boolean logic error

[Michael McConville]

Robbie Harwood wrote:
> Michael McConville <mmcco at mykolab.com> writes:
> 
> > The below if condition is obviously always true in its current state.
> > It seems that the author meant to use && rather than ||.
> >
> > diff --git a/src/windows/leashdll/krb5routines.c b/src/windows/leashdll/krb5routines.c
> > index 3911720ae..ff5eb4980 100644
> > --- a/src/windows/leashdll/krb5routines.c
> > +++ b/src/windows/leashdll/krb5routines.c
> > @@ -255,7 +255,7 @@ LeashKRB5_renew(void)
> >      pkrb5_cc_set_flags(ctx, cc, KRB5_TC_NOTICKET);
> >  #endif
> >      if (code) {
> > -        if ( code != KRB5KDC_ERR_ETYPE_NOSUPP ||
> > +        if ( code != KRB5KDC_ERR_ETYPE_NOSUPP &&
> >               code != KRB5_KDC_UNREACH)
> >              Leash_krb5_error(code, "krb5_get_renewed_creds()", 0, &ctx, &cc);
> >          goto cleanup;
> 
> That doesn't seem correct to me.  On line 253, we call into
> pkrb5_get_renewed_creds(), which is what's being checked by this call -
> it's a wrapper around get_valrenewed_creds(), which can return a lot
> more than KRB5KDC_ERR_ETYPE_NOSUPP and KRB5_KDC_UNREACH - including
> EINVAL, ENOMEM, KRB5_CC_NOTFOUND, to name a few.

I've never looked at Kerberos's code before; I'm scanning port/package repos en
masse for coding errors. Sorry for the oversight in my patch, but I think I
still found a bug. 

My point is that the if condition tests whether a variable is non-equal to two
different constants, which is always true. This pretty clearly indicates a
mistake in the code logic.

However, I was moving too fast and misunderstood the code. This condition is
checking for failure, not success. It isn't that the author meant to use &&
rather than ||, it's that he meant to use == rather than !=.

Thanks for your time,
Michael McConville
University of Utah