[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Lukas Slebodnik lslebodn at redhat.com
Mon Dec 15 09:12:04 UTC 2014


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

Summary: your solution is partially done therefore NACK

LS



More information about the sssd-devel mailing list