[19703] in Kerberos_V5_Development
Re: Probable boolean logic error
daemon@ATHENA.MIT.EDU (Michael McConville)
Thu Dec 7 00:28:23 2017
Date: Wed, 6 Dec 2017 22:11:08 -0700
From: Michael McConville <mmcco@mykolab.com>
To: Robbie Harwood <rharwood@redhat.com>
Message-ID: <20171207051108.6fizcrttee6adasu@t240>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <jlgtvx3yg70.fsf@redhat.com>
Cc: krbdev@mit.edu
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: krbdev-bounces@mit.edu
Robbie Harwood wrote:
> Michael McConville <mmcco@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
_______________________________________________
krbdev mailing list krbdev@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev
