On Wed, Mar 11, 2015 at 09:46:42AM +0100, Lukas Slebodnik wrote:
On (10/03/15 15:59), Pavel Reichl wrote:
>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(-)
>
ACK
warnings are gone.
LS
* master: 6ccda8691123bb27f5f2a88a0c80174af3e0fd0a
* sssd-1-12: 6ccda8691123bb27f5f2a88a0c80174af3e0fd0a