On Mon, Apr 07, 2014 at 05:30:39PM +0200, Jakub Hrozek wrote: > On Mon, Apr 07, 2014 at 04:51:58PM +0200, Jakub Hrozek wrote: > > 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. > > OK, after some testing I agree with Lukas. If the defaults don't exist > or can't be downloaded we fail earlier, in particular, the > ipa_get_config_recv() request would fail and we'd never reach the part > where we assign the output variables. > > So ACK to this code. > > After some more discussion with Lukas, I no longer think it's necessary > to push this patch to 1.11, master is enough.
Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel