[Freeipa-devel] [PATCH 0005] Fixed some of the issues reported in FreeIPA code by covscan
Alexander Bokovoy
abokovoy at redhat.com
Tue Jan 27 09:10:21 UTC 2015
On Tue, 27 Jan 2015, Martin Babinsky wrote:
>The attached patch is related to
>https://fedorahosted.org/freeipa/ticket/4795 and fixes (hopefully)
>some of the defects reported by subsequent scans.
NACK overall. If you want to provide fixes, make them separate of each
other and explain each fix. See below for details.
>
>There are also 21 defects reported in asn1/asn1c/*.c files (http://cov01.lab.eng.brq.redhat.com/covscanhub/task/16545/log/freeipa-4.1.99.201501261541GIT871f9bb-0.fc21/scan-results.html).
>Since this code is (semi)-automatically generated by asn1c software,
>we should decide what to do with them.
>
>Should I try to fix them by hand and/or report them upstream?
No manual fixes, we'll need to find out a way to fix them upstream.
>
>Martin^3
>From 4732626ed0fb8ec0fb2074c55955ab570eac58fa Mon Sep 17 00:00:00 2001
>From: Martin Babinsky <mbabinsk at redhat.com>
>Date: Fri, 16 Jan 2015 15:43:17 +0100
>Subject: [PATCH] Fixed issues reported in FreeIPA code by covscan
>
>This patch is related to https://fedorahosted.org/freeipa/ticket/4795.
>Performed another scan and fixed/waived some defects reported by Coverity in
>IPA C code.
>---
> daemons/ipa-kdb/ipa_kdb_audit_as.c | 5 +++++
> daemons/ipa-kdb/ipa_kdb_mspac.c | 5 +----
> daemons/ipa-kdb/ipa_kdb_principals.c | 11 ++++-------
> daemons/ipa-sam/ipa_sam.c | 2 +-
> .../ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c | 8 ++++++--
> daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 4 +++-
> daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c | 3 ++-
> daemons/ipa-slapi-plugins/libotp/Makefile.am | 4 +++-
> daemons/ipa-slapi-plugins/libotp/otp_config.c | 9 ++++++++-
> util/ipa_krb5.h | 2 ++
> 10 files changed, 35 insertions(+), 18 deletions(-)
>
>diff --git a/daemons/ipa-kdb/ipa_kdb_audit_as.c b/daemons/ipa-kdb/ipa_kdb_audit_as.c
>index 52c165442bde61d3ce88843b122aae7fe0fae50b..81ccbc2de28681c9c90b932fb14831965e0b246c 100644
>--- a/daemons/ipa-kdb/ipa_kdb_audit_as.c
>+++ b/daemons/ipa-kdb/ipa_kdb_audit_as.c
>@@ -20,6 +20,7 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
>+#include <syslog.h>
> #include "ipa_kdb.h"
> #include "ipa_pwd.h"
NACK, why do you need syslog.h here?
>
>@@ -120,7 +121,11 @@ void ipadb_audit_as_req(krb5_context kcontext,
> client->last_failed = authtime;
> client->mask |= KMASK_LAST_FAILED;
> break;
>+ /*coverity[dead_error_begin]*/
> default:
>+ krb5_klog_syslog(LOG_ERR,
>+ "Got an unexpected value of error_code: %d\n",
>+ error_code);
> return;
> }
>
>diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
>index a4500070760e83994c8155a12ee6414b5ebee9e0..0f47d1f4bd536e24b9d46a35232ad558b33b4b26 100644
>--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
>+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
>@@ -54,9 +54,6 @@ struct ipadb_mspac {
> time_t last_update;
> };
>
>-
>-int krb5_klog_syslog(int, const char *, ...);
>-
> static char *user_pac_attrs[] = {
> "objectClass",
> "uid",
What does this fix have to do with coverity?
Please separate patches and submit one by one.
>@@ -2074,7 +2071,7 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
> }
> }
>
>- kerr = ipadb_get_pac(context, client_entry ? client_entry : client, &pac);
>+ kerr = ipadb_get_pac(context, client, &pac);
> if (kerr != 0 && kerr != ENOENT) {
> goto done;
> }
NACK, this change is wrong and breaks conditional delegation.
>diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
>index e158c236eab5c7c5a7c12664dbde5d51cc55406d..760faeef224701c3dc4ee69d16df043e9c622d9a 100644
>--- a/daemons/ipa-kdb/ipa_kdb_principals.c
>+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
>@@ -1894,19 +1894,16 @@ static krb5_error_code ipadb_modify_principal(krb5_context kcontext,
> if (!ipactx) {
> return KRB5_KDB_DBNOTINITED;
> }
>-
>+ kerr = krb5_unparse_name(kcontext, entry->princ, &principal);
>+ if (kerr != 0) {
>+ goto done;
>+ }
> ied = (struct ipadb_e_data *)entry->e_data;
> if (!ied || !ied->entry_dn) {
>- kerr = krb5_unparse_name(kcontext, entry->princ, &principal);
>- if (kerr != 0) {
>- goto done;
>- }
>-
> kerr = ipadb_fetch_principals(ipactx, 0, principal, &res);
> if (kerr != 0) {
> goto done;
> }
>-
> /* FIXME: no alias allowed for now, should we allow modifies
> * by alias name ? */
> kerr = ipadb_find_principal(kcontext, 0, res, &principal, &lentry);
NACK, we only want to touch entry->princ and fetch it if it does not
exist. The cost of these operations is sufficient enough to postpone.
>diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
>index 07249fd27b362ed6499e372d651192dfc31b5173..ea9c18888503f40bfc288703d985530a66539b7d 100644
>--- a/daemons/ipa-sam/ipa_sam.c
>+++ b/daemons/ipa-sam/ipa_sam.c
>@@ -1487,7 +1487,7 @@ static bool ldapgroup2displayentry(struct ldap_search_state *state,
> return false;
> }
> break;
>-
>+ /*coverity[dead_error_begin]*/
> default:
> DEBUG(0,("unknown group type: %d\n", group_type));
> talloc_free(sid);
>diff --git a/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c b/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
>index 233813745795344f31a7dcf1931cf74a09f1e552..2990fba51452fcbe1c67572b0d1a64d5565e6eba 100644
>--- a/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
>+++ b/daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c
>@@ -111,13 +111,17 @@ static bool is_pwd_enabled(const char *user_dn)
> Slapi_Entry *entry = NULL;
> uint32_t authtypes;
> Slapi_DN *sdn;
>+ int search_result = 0;
>
> sdn = slapi_sdn_new_dn_byval(user_dn);
> if (sdn == NULL)
> return false;
>
>- slapi_search_internal_get_entry(sdn, attrs, &entry,
>- otp_config_plugin_id(otp_config));
>+ search_result = slapi_search_internal_get_entry(sdn, attrs, &entry,
>+ otp_config_plugin_id(otp_config));
>+ if (search_result != LDAP_SUCCESS) {
>+ LOG_TRACE("Entry not found. Error code: %d\n", search_result);
>+ }
> slapi_sdn_free(&sdn);
> if (entry == NULL)
> return false;
What does this fix have to do with Coverity?
>diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>index 84eff17013d2742d1b5e5c4ea5f4e22ee290d785..b28e2d220a41628277dbdce84dfdbc5952476190 100644
>--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
>@@ -634,7 +634,9 @@ static int ipapwd_pre_mod(Slapi_PBlock *pb)
> is_krb = 0;
> is_smb = 0;
> is_ipant = 0;
>-
>+ /* coverity[fallthrough]
>+ * After examining the output of covscan, we think that this
>+ * fallthrough is intentional.*/
> case LDAP_MOD_ADD:
> if (!lmod->mod_bvalues ||
> !lmod->mod_bvalues[0]) {
>diff --git a/daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c b/daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c
>index 2b07de45b63dab36a0b7167e3583e88ebd07f6f7..061fd12521f072498ecc72858dfe79ba46624a51 100644
>--- a/daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c
>+++ b/daemons/ipa-slapi-plugins/ipa-uuid/ipa_uuid.c
>@@ -1027,9 +1027,10 @@ static int ipauuid_pre_op(Slapi_PBlock *pb, int modtype)
>
> slapi_mod_free(&next_mod);
> break;
>-
>+ /*coverity[dead_error_begin]*/
> default:
> /* never reached, just silence compiler */
>+ LOG_TRACE("Got unexpected value of modtype: %d\n", modtype);
> break;
> }
>
>diff --git a/daemons/ipa-slapi-plugins/libotp/Makefile.am b/daemons/ipa-slapi-plugins/libotp/Makefile.am
>index 4428f6bdc38a4e4ec224d1fa70744d8381f7e0b1..71b9c19f40379ba6c61858984f9de0253020e00d 100644
>--- a/daemons/ipa-slapi-plugins/libotp/Makefile.am
>+++ b/daemons/ipa-slapi-plugins/libotp/Makefile.am
>@@ -1,5 +1,7 @@
> MAINTAINERCLEANFILES = *~ Makefile.in
>-AM_CPPFLAGS = -I/usr/include/dirsrv
>+PLUGIN_COMMON_DIR = ../common
>+AM_CPPFLAGS = -I/usr/include/dirsrv \
>+ -I$(PLUGIN_COMMON_DIR)
>
> noinst_LTLIBRARIES = libhotp.la libotp.la
> libhotp_la_SOURCES = hotp.c hotp.h
>diff --git a/daemons/ipa-slapi-plugins/libotp/otp_config.c b/daemons/ipa-slapi-plugins/libotp/otp_config.c
>index ac2cfc72aa9f72af8eb5b5c565650325ac8bf714..0d87ac0cdf35fd0d457ee5f2ee22d6cf4b1297cd 100644
>--- a/daemons/ipa-slapi-plugins/libotp/otp_config.c
>+++ b/daemons/ipa-slapi-plugins/libotp/otp_config.c
>@@ -38,6 +38,7 @@
> * END COPYRIGHT BLOCK **/
>
> #include "otp_config.h"
>+#include "util.h"
>
> #include <pratom.h>
> #include <plstr.h>
>@@ -214,6 +215,7 @@ struct otp_config *otp_config_init(Slapi_ComponentId *plugin_id)
>
> struct otp_config *cfg = NULL;
> void *node = NULL;
>+ int search_result = 0;
>
> cfg = (typeof(cfg)) slapi_ch_calloc(1, sizeof(*cfg));
> cfg->plugin_id = plugin_id;
>@@ -236,7 +238,12 @@ struct otp_config *otp_config_init(Slapi_ComponentId *plugin_id)
> cfg->records = rec;
>
> /* Load the specified entry. */
>- slapi_search_internal_get_entry(rec->sdn, NULL, &entry, plugin_id);
>+ search_result = slapi_search_internal_get_entry(rec->sdn,
>+ NULL, &entry, plugin_id);
>+ if (search_result != LDAP_SUCCESS) {
>+ LOG_TRACE("Entry not found. Error code: %d\n",
>+ search_result);
>+ }
> update(cfg, rec->sdn, entry);
> slapi_entry_free(entry);
> }
What does this fix have to do with coverity?
>diff --git a/util/ipa_krb5.h b/util/ipa_krb5.h
>index 7b877aa665dd6cb4e0c1cf9d8153319cc8f61a20..2153bd57142d1468031d0aa4b5d3f59ef5c890b5 100644
>--- a/util/ipa_krb5.h
>+++ b/util/ipa_krb5.h
>@@ -30,6 +30,8 @@ struct keys_container {
> #define KEYTAB_RET_OID "2.16.840.1.113730.3.8.10.2"
> #define KEYTAB_GET_OID "2.16.840.1.113730.3.8.10.5"
>
>+int krb5_klog_syslog(int, const char *, ...);
>+
> void
> ipa_krb5_free_ktypes(krb5_context context, krb5_enctype *val);
>
>--
>2.1.0
>
>_______________________________________________
>Freeipa-devel mailing list
>Freeipa-devel at redhat.com
>https://www.redhat.com/mailman/listinfo/freeipa-devel
--
/ Alexander Bokovoy
More information about the Freeipa-devel
mailing list