Probable boolean logic error

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Probable boolean logic error

Michael McConville
Hi guys.

The below if condition is obviously always true in its current state.
It seems that the author meant to use && rather than ||.

Thanks for your time,
Michael McConville
University of Utah

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;
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Probable boolean logic error

Robbie Harwood
Michael McConville <[hidden email]> writes:

> Hi guys.
>
> The below if condition is obviously always true in its current state.
> It seems that the author meant to use && rather than ||.
>
> Thanks for your time,
> Michael McConville
> University of Utah
>
> 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.

Thanks,
--Robbie

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Probable boolean logic error

Michael McConville
Robbie Harwood wrote:

> Michael McConville <[hidden email]> 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             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: Probable boolean logic error

Greg Hudson
On 12/07/2017 12:11 AM, Michael McConville wrote:

> Robbie Harwood wrote:
>> Michael McConville <[hidden email]> writes:
>>>      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.

Yes, it's definitely a bug.  I have filed an issue here:

  http://krbdev.mit.edu/rt/Ticket/Display.html?id=8628

Since we don't spin up a development and testing environment for the
Windows code frequently, I don't expect to fix the bug right away.  The
current behavior has been around for over a decade and doesn't seem
especially harmful.

> 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 !=.

I think you were right the first time; the apparent intent is to
suppress reporting of two specific errors, not to report two specific
errors.  But with context from the version control history, it's not
clear to me why the second reporting suppression (for KRB5_KDC_UNREACH)
is desirable.  There have also been some changes to the calling code.
So the best fix might not be as simple as correcting the apparent intent
of the boolean expression.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev