On Fri, Apr 04, 2014 at 08:42:53PM +0200, Michal Židek wrote:
> 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
>
I think you should also test the patch with an IPA server that doesn't
support SELinux at all. I'll try to do some testing with an SELinux
search base that points nowhere to simulate the behaviour.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
