[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Pavel Reichl preichl at redhat.com
Mon Dec 15 11:15:57 UTC 2014


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




More information about the sssd-devel mailing list