[SSSD] [PATCH] Passing NULL as map_order in create_order_array is invalid.

Michal Židek mzidek at redhat.com
Fri Apr 4 18:42:53 UTC 2014


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 at 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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-dead-code-from-ipa_get_selinux_recv.patch
Type: text/x-patch
Size: 3077 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140404/e0eea20d/attachment.bin>


More information about the sssd-devel mailing list