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 <mzidek(a)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 <jhrozek(a)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.