[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Pavel Reichl preichl at redhat.com
Wed Dec 17 14:27:12 UTC 2014


On 12/15/2014 12:15 PM, Pavel Reichl wrote:
>
> On 12/15/2014 10:12 AM, Lukas Slebodnik wrote:
>> On (12/12/14 17:10), Pavel Reichl wrote:
>>> On 12/12/2014 04:10 PM, Lukas Slebodnik wrote:
>>>> On (12/12/14 16:04), Pavel Reichl wrote:
>>>>> On 12/12/2014 03:52 PM, Lukas Slebodnik wrote:
>>>>>> On (12/12/14 15:25), Pavel Reichl wrote:
>>>>>>> On 12/12/2014 03:02 PM, Lukas Slebodnik wrote:
>>>>>>>> On (12/12/14 14:25), Pavel Reichl wrote:
>>>>>>>>> On 12/11/2014 06:35 PM, Michal Židek wrote:
>>>>>>>>>> I think that smaller nesting levels are better.
>>>>>>>>>>
>>>>>>>>>> In general I would do
>>>>>>>>>>
>>>>>>>>>> if (ret == SPECIFIC_CODE) {
>>>>>>>>>>    specific_error_handle_block
>>>>>>>>>> } else (ret != EOK) {
>>>>>>>>>>    general_error_handle_block
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> If there are many specific error codes that need to be 
>>>>>>>>>> addressed,
>>>>>>>>>> use switch, but 1-2 else-ifs are ok to me.
>>>>>>>>>>
>>>>>>>>>> This pattern can be used if common_to_all_erros_action exists 
>>>>>>>>>> (and is
>>>>>>>>>> something more than just goto done):
>>>>>>>>>>
>>>>>>>>>> if (ret != EOK) {
>>>>>>>>>>    if (ret == SPECIFIC_CODE) {
>>>>>>>>>>        specific_error_handle_block
>>>>>>>>>>    }
>>>>>>>>>>    common_to_all_errors_action;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> So I would slightly prefer Lukas's solution over Pavel's.
>>>>>>>>>>
>>>>>>>>>> In this particullar case I would do:
>>>>>>>>>> if (ret == ENOENT) {
>>>>>>>>>>     ....
>>>>>>>>>> } else if (ret != EOK) {
>>>>>>>>>>     ....
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Michal
>>>>>>>>>>
>>>>>>>>> OK, thank you both for opinions and for comments.
>>>>>>>>>
>>>>>>>>> Please see updated patch.
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>> Two questions/comments are inline.
>>>>>>>>
>>>>>>>> >From 94eade7b3c0c0729f5d4a322061c7c9688bd8130 Mon Sep 17 
>>>>>>>> 00:00:00 2001
>>>>>>>>> From: Pavel Reichl <preichl at redhat.com>
>>>>>>>>> Date: Tue, 9 Dec 2014 11:01:13 +0000
>>>>>>>>> Subject: [PATCH 2/2] SYSDB: sysdb_search_object_by_sid returns 
>>>>>>>>> ENOENT
>>>>>>>>>
>>>>>>>>> sysdb_search_object_by_sid returns ENOENT if no results are 
>>>>>>>>> found.
>>>>>>>>>
>>>>>>>>> Part od solution for:
>>>>>>>>> https://fedorahosted.org/sssd/ticket/1991
>>>>>>>>>
>>>>>>>>> Fixes:
>>>>>>>>> https://fedorahosted.org/sssd/ticket/2520
>>>>>>>>> ---
>>>>>>>>> src/db/sysdb.h                 |  2 +-
>>>>>>>>> src/db/sysdb_ops.c             | 68 
>>>>>>>>> ++++++------------------------------------
>>>>>>>>> src/responder/nss/nsssrv_cmd.c | 27 ++++++++---------
>>>>>>>>> src/responder/pac/pacsrv_cmd.c | 29 ++++++++++--------
>>>>>>>>> src/tests/sysdb-tests.c        |  5 +---
>>>>>>>>> 5 files changed, 40 insertions(+), 91 deletions(-)
>>>>>>>>> errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
>>>>>>>>> diff --git a/src/responder/nss/nsssrv_cmd.c 
>>>>>>>>> b/src/responder/nss/nsssrv_cmd.c
>>>>>>>>> index 
>>>>>>>>> ea58920bc3611c84958e8d0aca0e122d90c68e5c..3c5d450714fb3f7655cd32aeef900b4f5e9782c7 
>>>>>>>>> 100644
>>>>>>>>> --- a/src/responder/nss/nsssrv_cmd.c
>>>>>>>>> +++ b/src/responder/nss/nsssrv_cmd.c
>>>>>>>>> @@ -4491,7 +4491,19 @@ static errno_t 
>>>>>>>>> nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
>>>>>>>>>
>>>>>>>>>      ret = sysdb_search_object_by_sid(cmdctx, dom, 
>>>>>>>>> cmdctx->secid, NULL,
>>>>>>>>> &dctx->res);
>>>>>>>>> -    if (ret != EOK) {
>>>>>>>>> +    if (ret == ENOENT) {
>>>>>>>>> +        if (!dctx->check_provider) {
>>>>>>>>> +            DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid 
>>>>>>>>> call.\n");
>>>>>>>>> +
>>>>>>>>> +            /* set negative cache only if not result of cache 
>>>>>>>>> check */
>>>>>>>>> +            ret = sss_ncache_set_sid(nctx->ncache, false, 
>>>>>>>>> cmdctx->secid);
>>>>>>>>> +            if (ret != EOK) {
>>>>>>>>> +                DEBUG(SSSDBG_MINOR_FAILURE,
>>>>>>>>> +                      "Cannot set negative cache for %s\n", 
>>>>>>>>> cmdctx->secid);
>>>>>>>>> +            }
>>>>>>>>> +        }
>>>>>>>>> +        return ENOENT;
>>>>>>>> You changed behaviour with this nesting if statements.
>>>>>>>> ENOENT will be returned every time. Previously it was returned 
>>>>>>>> just in case of
>>>>>>>> true condition (dctx->res->count == 0 && !dctx->check_provider)
>>>>>>>> Could you explain such change?
>>>>>>> Because otherwise 'res' is accessed in case of ENOENT which 
>>>>>>> implies segfault
>>>>>>> in nss_cmd_getbysid_send_reply() or even earlier in 
>>>>>>> check_cache(), if I read
>>>>>>> the code correctly.
>>>>>>>
>>>>>> Thank you for explanation.
>>>>> yw
>>>>>> But it does not change the fact that you change
>>>>>> the behaviour with this refactoring patch.
>>>>>> Please do such change in separate patch.
>>>>> I don't think this a good idea. The change in sysdb is changing 
>>>>> behaviour -
>>>>> no doubt about it and I really think I have to amend the code that 
>>>>> uses it.
>>>>> It is IMO a very bad idea send separate patch that introduces 
>>>>> probable
>>>>> segfault and fix it in another.
>>>>>
>>>> I disagree. The segfault could be even with old code or did I miss 
>>>> something?
>>> I don't see how it could happen with old code, but if it did - I'll 
>>> send the
>>> separate patch as you wish. Otherwise I would prefer to stick with 
>>> my current
>>> patch.
>>> How it could segfault?
>>>
>> Firstly, I would like to apologize that I didn't spend lot of time with
>> a review. I just read the patch
>>
>> Let's summarise some facts.
>> * your match is named: sysdb_search_object_by_sid returns ENOENT
>>    sysdb_search_object_by_sid returns ENOENT if no results are found
>> * It isn't possible to crash with current master.
>> * You conceal a reason why there could be crash.
>>    The function nss_cmd_getbysid_send_reply checks the result count 
>> for 0,
>>    which we wanted to fix .
>> * You had to change behaviour of the function nss_cmd_getbysid_search
>>    because there could be crash.
>> It should be sign for you that something stinks in the code.
>>
>> Let's imagine situation. We have function1 which is called be 
>> function2 which
>> ic called by function3 which id called by function4. All these 
>> functions rely
>> on fact that res->count is 0 they stop execution. You decided to convert
>> function1 to return ENOENT instead of checking res->count to zero. 
>> All other
>> functions should be converted as well because res needn't be 
>> initialized and
>> they could crash. Such change should not be done in single patch. It 
>> would
>> easily become mess a nightmare for reviewer. There is high 
>> probability of
>> overlooking some places which was not converted properly. The right 
>> approach is
>> to convert function from top to down (and not from down to top).
>> Each patch will contain conversion of just one function:
>>      patch1: function4 returns ENOENT
>>      patch2: function3 returns ENOENT
>>      ..
>> Each patch will be simple and will not change the behaviour of other 
>> function.
>>
>> I hope it is clear to you now.
>> Let me know whether you will sent patch ASAP or I should send temporary
>> solution for ticket #2520
> I think it will be best if you send temporally solution.  Acking 
> process for this patch is really cumbersome. :-(
>>
>> Summary: your solution is partially done therefore NACK
>>
>> LS
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Updated patches attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TESTS-sysdb_delete_by_sid-test-return-value.patch
Type: text/x-patch
Size: 2071 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141217/5b789f27/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-NSS-nss_cmd_getbysid_search-return-ENOENT.patch
Type: text/x-patch
Size: 1547 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141217/5b789f27/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SYSDB-sysdb_search_object_by_sid-returns-ENOENT.patch
Type: text/x-patch
Size: 9581 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141217/5b789f27/attachment-0002.bin>


More information about the sssd-devel mailing list