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
