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 <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.
Pushed to master.