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!
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@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
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@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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:
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@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:
From 3343172c483589386585b1e3675ec90e990e1b22 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
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@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:
From 3343172c483589386585b1e3675ec90e990e1b22 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl