On Thu, Nov 13, 2014 at 07:00:02PM +0100, Michal Židek wrote:
On 11/13/2014 06:45 PM, Jakub Hrozek wrote:
>On Wed, Nov 12, 2014 at 02:53:00PM +0100, Michal Židek wrote:
>>On 11/11/2014 01:37 PM, Jakub Hrozek wrote:
>>>On Thu, Nov 06, 2014 at 07:48:20PM +0100, Michal Židek wrote:
>>>>On 11/06/2014 07:43 PM, Michal Židek wrote:
>>>>>On 11/05/2014 04:53 PM, Michal Židek wrote:
>>>>>>I found this bug while working on
>>>>>>https://fedorahosted.org/sssd/ticket/2461
>>>>>>
>>>>>>It turned out that not only case_sensitive = preserving,
>>>>>>but also case_sensitive = false did not work
>>>>>>with proxy provider.
>>>>>>
>>>>>>But this patch does not solve the issue in the ticket,
>>>>>>so I did not link them.
>>>>>
>>>>>After some investigation I actually think this was
>>>>>the main issue that caused the bug mentioned in
>>>>>the ticket so I added the "Fixes" label to it.
>>>>>
>>>>>I also added second patch that preservers the
>>>>>service name with proxy provider.
>>>>>
>>>>>Michal
>>>>>
>>>>
>>>>And now with patches :)
>>>>
>>>
>>>Coverity found a warning in the patches:
>>>Error: UNUSED_VALUE (CWE-563):
>>>sssd-1.12.3/src/providers/proxy/proxy_id.c:609: value_overwrite: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
overwritten with value "12".
>>>sssd-1.12.3/src/providers/proxy/proxy_id.c:614: value_overwrite: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
overwritten with value from "sysdb_attrs_add_string(attrs, "nameAlias",
cased_alias)".
>>>sssd-1.12.3/src/providers/proxy/proxy_id.c:623: value_overwrite: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is
overwritten with value from "sysdb_store_group(dom, real_name, grp->gr_gid, attrs,
cache_timeout, now)".
>>>sssd-1.12.3/src/providers/proxy/proxy_id.c:603: returned_value: Value from
"sysdb_attrs_add_string(attrs, "nameAlias", lc_gr_name)" is assigned
to "ret" here, but that stored value is not used before it is overwritten.
>>>
>>>Looks like an unchecked call. Apart from that, the code looks good.
>>
>>Thank you Jakub and Pavel for the review. New patches are attached.
>>
>>Michal
>>
>
>> From fbd49da512a60d18dad11a5f1751b45db7ded168 Mon Sep 17 00:00:00 2001
>>From: Michal Zidek <mzidek(a)redhat.com>
>>Date: Fri, 31 Oct 2014 16:39:25 +0100
>>Subject: [PATCH 1/2] proxy: Do not try to store same alias twice
>>
>>LDB does not store attributes if they have the
>>same name and value and errors out instead.
>>
>>Fixes:
>>https://fedorahosted.org/sssd/ticket/2461
>>---
>> src/providers/proxy/proxy_id.c | 76 ++++++++++++++++++++++++++----------------
>> 1 file changed, 48 insertions(+), 28 deletions(-)
>>
>
>[...]
>
>> if (alias) {
>> cased_alias = sss_get_cased_name(attrs, alias, !lowercase);
>> if (!cased_alias) {
>>- talloc_zfree(attrs);
>>- return ENOMEM;
>>+ goto done;
>
>ret is undefined on this jump (probably would be EOK).
Sorry for that. Fixed now.
>
>The rest of the code looks good to me.
New version is attached. I only changed that one line.
Michal
ACK