Probable boolean logic error

[Greg Hudson]

On 12/07/2017 12:11 AM, Michael McConville wrote:
> Robbie Harwood wrote:
>> Michael McConville <mmcco at mykolab.com> 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.