[Freeipa-devel] [PATCHES] 366-372 Additional Coverity fixes

[Jan Cholasta]

Dne 24.11.2014 v 14:44 Alexander Bokovoy napsal(a):
> On Tue, 18 Nov 2014, Jan Cholasta wrote:
>> Dne 12.11.2014 v 08:58 Petr Spacek napsal(a):
>>> On 11.11.2014 12:27, Jan Cholasta wrote:
>>>> Dne 11.11.2014 v 11:40 Alexander Bokovoy napsal(a):
>>>>> On Tue, 11 Nov 2014, Jan Cholasta wrote:
>>>>>>> From 82d7d37ca310af015018ebb2da2f9a72c4dabcaa Mon Sep 17 00:00:00
>>>>>>> 2001
>>>>>> From: Jan Cholasta <jcholast at redhat.com>
>>>>>> Date: Mon, 10 Nov 2014 18:10:27 +0000
>>>>>> Subject: [PATCH 4/7] Fix unchecked return value in ipa-kdb
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4713
>>>>>> ---
>>>>>> daemons/ipa-kdb/ipa_kdb_mspac.c | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>> b/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>> index c8f6c76..debcd1b 100644
>>>>>> --- a/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>> +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
>>>>>> @@ -2071,6 +2071,9 @@ krb5_error_code
>>>>>> ipadb_sign_authdata(krb5_context
>>>>>> context,
>>>>>>                             ipactx->kdc_hostname,
>>>>>> strlen(ipactx->kdc_hostname),
>>>>>>                             NULL, NULL, &result) == 0) {
>>>>>>                 kerr = ipadb_reinit_mspac(ipactx, true);
>>>>>> +                if (kerr != 0 && kerr != ENOENT) {
>>>>>> +                    goto done;
>>>>>> +                }
>>>>>>             }
>>>>>>         }
>>>>>>
>>>>> I'm not sure we should drop the sign_authdata request here. If we were
>>>>> able to re-initialize our view of trusted domains, we simply cannot
>>>>> re-sign incoming PAC but this is handled in ipadb_verify_pac() and
>>>>> ipadb_sign_pac() and if the former returns NULL value for PAC, we exit
>>>>> with a return code of 0 while this change will fail a cross-realm TGT
>>>>> request unconditionally.
>>>>>
>>>>
>>>> OK, what would be a proper fix? Just ignore the return value of
>>>> ipadb_reinit_mspac here?
>>>
>>> Guys, I did not see the code but all instances of "ignore return
>>> code" I have
>>> seen were wrong, including cases where code comment explicitly said
>>> "we ignore
>>> return code on purpose" :-)
>>>
>>> At least log an error message if you can't think of anything better ...
>>>
>>
>> I don't disagree, if that's the proper fix.
>>
>> Alexander, or someone else, could you please finish the review of the
>> patches? Thanks.
> I've ACKed other patches but I'm not going to accept this patch. Rest is
> fine.

That's fine with me, but I'm still going to need a little hint on how 
this should in fact be fixed.

-- 
Jan Cholasta