On 03/10/2015 10:40 AM, Lukas Slebodnik wrote:
>On (09/03/15 11:24), Pavel Reichl wrote:
>>Hello,
>>
>>please see attached trivial patch fixing 2 coverity warnings - ret was
>>overriden without being read first.
>>
>>>done:
>>> if (krberr != 0) KRB5_SYSLOG(krberr);
>>> if (keytab) krb5_kt_close(context, keytab);
>>> if (context) krb5_free_context(context);
>>> if (ccname_file_dummy) {
>>> DEBUG(SSSDBG_TRACE_INTERNAL, "Unlinking [%s]\n",
ccname_file_dummy);
>>> ret = unlink(ccname_file_dummy);
>>> if (ret == -1) {
>>> ret = errno;
>>> DEBUG(SSSDBG_MINOR_FAILURE,
>>> "Unlink failed [%d][%s].\n", ret, strerror(ret));
>>> }
>>> }
>>> talloc_free(tmp_ctx);
>>> return krberr;
>>ci:
>>
>>http://sssd-ci.duckdns.org/logs/job/9/03/summary.html
>>
>>
>>Thanks!
>>From f2faea03742598aa8185db6deeaa0b23b4fa1e52 Mon Sep 17 00:00:00 2001
>>From: Pavel Reichl <preichl(a)redhat.com>
>>Date: Mon, 9 Mar 2015 05:45:20 -0400
>>Subject: [PATCH] ldap_child: fix coverity warning
>>
>>In ldap_child_get_tgt_sync() variable 'ret' got overriden in done
>>section without ever before being read.
>>---
>>src/providers/ldap/ldap_child.c | 6 ++++++
>>1 file changed, 6 insertions(+)
>>
>>diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
>>index
8f034affa48095b6e512c866f8a3c33465e5c595..e9e845f35df53a780f365f90332a04338ad16026 100644
>>--- a/src/providers/ldap/ldap_child.c
>>+++ b/src/providers/ldap/ldap_child.c
>>@@ -399,6 +399,9 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX
*memctx,
>> DB_PATH, realm_name);
>> if (ccname_file_dummy == NULL) {
>> ret = ENOMEM;
>>+ DEBUG(SSSDBG_CRIT_FAILURE,
>>+ "talloc_asprintf failed: %s:[%d].\n",
>>+ strerror(ret), ret);
>> goto done;
>> }
>>
>>@@ -407,6 +410,9 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX
*memctx,
>> umask(old_umask);
>> if (fd == -1) {
>> ret = errno;
>>+ DEBUG(SSSDBG_CRIT_FAILURE,
>>+ "mkstemp failed: %s:[%d].\n",
>>+ strerror(ret), ret);
>> goto done;
>ACK for debug messages.
>NACK for patch.
>
>As you can see ldap_child_get_tgt_sync returns krb5_error_code and not
>int or errnot_t.
>
>It means that function ldap_child_get_tgt_sync might return wrong error code.
>It's very likely that krberr was set to 0 in previous steps.
>
>LS
Thanks, please see updated patch.
Would it make sense to file a ticket to refactor (break into smaller
functions) ldap_child_get_tgt_sync()? I think it is too long and thus
difficult to maintain.
ci:
http://sssd-ci.duckdns.org/logs/job/9/30/summary.html
From 3343172c483589386585b1e3675ec90e990e1b22 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Mon, 9 Mar 2015 05:45:20 -0400
Subject: [PATCH] ldap_child: fix coverity warning
In ldap_child_get_tgt_sync() variable 'ret' got overriden in done
section without ever before being read.
---
src/providers/ldap/ldap_child.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)