On 04/04/2014 07:26 PM, Lukas Slebodnik wrote:
On (04/04/14 17:08), Michal Židek wrote:
Reported by Clang. See patch description for more info.
Patch is in attachment.
Thanks,
Michal
From a0c94bc4c5df2955e82a7dc0852a86019286577e Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Fri, 4 Apr 2014 16:53:48 +0200
Subject: [PATCH] Passing NULL as map_order in create_order_array is invalid.
Reported by Clang. This is actually false positive because
a NULL pointer in map_order would cause the function above
(sysdb_store_selinux_config) to fail and create_order_array
would be skipped. So this patch is another line of
defense that just makes the code more readable and silences
the message from Clang.
---
src/providers/ipa/ipa_selinux.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
index e0d7a00..5ee1e74 100644
--- a/src/providers/ipa/ipa_selinux.c
+++ b/src/providers/ipa/ipa_selinux.c
@@ -595,6 +595,10 @@ static errno_t create_order_array(TALLOC_CTX *mem_ctx,
const char *map_order,
int len;
size_t order_count;
+ if (map_order == NULL) {
+ return EINVAL;
+ }
+
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) {
ret = ENOMEM;
--
1.7.11.2
This patch fixed warning from clang static analyser.
But as you wrote, it is false positive because sssd would crash few lines
before calling create_order_array in function sysdb_store_selinux_config.
Why I think it is false positive:
-because map_order cannot be null.
static errno_t
ipa_get_selinux_recv(struct tevent_req *req,
//snip
if (state->defaults != NULL) {
//snip
*map_order = talloc_strdup(mem_ctx, tmp_str);
if (*map_order == NULL) {
talloc_zfree(*default_user);
return ENOMEM;
}
} else {
*map_order = NULL;
*default_user = NULL;
}
^^^^^
else branch is dead code.
state->defaults could be NULL in offline mode, but only in the first version
of file src/providers/ipa/ipa_selinux.c The behaviour was rewritten in commit
commit a004ce714c20a7a5324393ea47f5dc115eb20713
Author: Jakub Hrozek <jhro...@redhat.com>
SELINUX: Process maps even when offline
We can ignore this warning and function ipa_get_selinux_recv should be
refactored.
LS
I think you are right. The same case is with all the 'else' branches in
ipa_get_selinux_recv. So these 'else' branches can be IMO removed (patch
attached).
I tested both online and offline modes with the attached patch and
nothing seem to be broken.
Michal
>From 940b711bf28c9eb6dd994e1c18049470f01ae6d6 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Fri, 4 Apr 2014 20:04:46 +0200
Subject: [PATCH] Remove dead code from ipa_get_selinux_recv
The 'else' branches in ipa_get_selinux_recv are never
executed (and even if they were, the result would be
the same as if the true branches were taken).
---
src/providers/ipa/ipa_selinux.c | 61 ++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 38 deletions(-)
diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
index 5ee1e74..c731d9e 100644
--- a/src/providers/ipa/ipa_selinux.c
+++ b/src/providers/ipa/ipa_selinux.c
@@ -1318,53 +1318,38 @@ ipa_get_selinux_recv(struct tevent_req *req,
TEVENT_REQ_RETURN_ON_ERROR(req);
- if (state->defaults != NULL) {
- ret = sysdb_attrs_get_string(state->defaults,
- IPA_CONFIG_SELINUX_DEFAULT_USER_CTX,
- &tmp_str);
- if (ret != EOK && ret != ENOENT) {
- return ret;
- }
-
- if (ret == EOK) {
- *default_user = talloc_strdup(mem_ctx, tmp_str);
- if (*default_user == NULL) {
- return ENOMEM;
- }
- }
-
- ret = sysdb_attrs_get_string(state->defaults, IPA_CONFIG_SELINUX_MAP_ORDER,
- &tmp_str);
- if (ret != EOK) {
- return ret;
- }
+ ret = sysdb_attrs_get_string(state->defaults,
+ IPA_CONFIG_SELINUX_DEFAULT_USER_CTX,
+ &tmp_str);
+ if (ret != EOK && ret != ENOENT) {
+ return ret;
+ }
- *map_order = talloc_strdup(mem_ctx, tmp_str);
- if (*map_order == NULL) {
- talloc_zfree(*default_user);
+ if (ret == EOK) {
+ *default_user = talloc_strdup(mem_ctx, tmp_str);
+ if (*default_user == NULL) {
return ENOMEM;
}
- } else {
- *map_order = NULL;
- *default_user = NULL;
}
- if (state->selinuxmaps != NULL && state->nmaps != 0) {
- *count = state->nmaps;
- *maps = talloc_steal(mem_ctx, state->selinuxmaps);
- } else {
- *count = 0;
- *maps = NULL;
+ ret = sysdb_attrs_get_string(state->defaults, IPA_CONFIG_SELINUX_MAP_ORDER,
+ &tmp_str);
+ if (ret != EOK) {
+ return ret;
}
- if (state->hbac_rules != NULL) {
- *hbac_count = state->hbac_rule_count;
- *hbac_rules = talloc_steal(mem_ctx, state->hbac_rules);
- } else {
- *hbac_count = 0;
- *hbac_rules = NULL;
+ *map_order = talloc_strdup(mem_ctx, tmp_str);
+ if (*map_order == NULL) {
+ talloc_zfree(*default_user);
+ return ENOMEM;
}
+ *count = state->nmaps;
+ *maps = talloc_steal(mem_ctx, state->selinuxmaps);
+
+ *hbac_count = state->hbac_rule_count;
+ *hbac_rules = talloc_steal(mem_ctx, state->hbac_rules);
+
return EOK;
}
--
1.7.11.2
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
