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 ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Robbie Harwood]

Post by 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;

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

[Michael McConville]

Post by Robbie Harwood

Post by Michael McConville
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 ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[Greg Hudson]

Post by Michael McConville

Post by Robbie Harwood

Post by Michael McConville
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.

Post by Michael McConville
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 ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev