[SSSD] [PATCH] SYSDB: add more debug msgs. to group membership code

Lukas Slebodnik lslebodn at redhat.com
Thu Aug 13 06:08:25 UTC 2015


On (12/08/15 13:49), Pavel Reichl wrote:
>
>On 08/12/2015 01:06 PM, Lukas Slebodnik wrote:
>>On (12/08/15 10:28), Pavel Reichl wrote:
>>>On 08/12/2015 10:11 AM, Lukas Slebodnik wrote:
>>>>On (12/08/15 09:42), Pavel Reichl wrote:
>>>>>On 08/12/2015 06:18 AM, Lukas Slebodnik wrote:
>>>>>>On (11/08/15 18:36), Pavel Reichl wrote:
>>>>>>>Hello,
>>>>>>>
>>>>>>>I'm investigating log file with debug_level 9 that contains following lines
>>>>>>>
>>>>>>>>[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member
>>>>>>>>[user at dom] to group [somedn]. Skipping.
>>>>>>>>[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member
>>>>>>>>[user at dom] to group [somedn2]. Skipping.
>>>>>>>>[sssd[be[dom]]] [sysdb_update_members_ex] (0x0020): Could not add member
>>>>>>>>[user at dom] to group [somedn3]. Skipping.
>>>>>>>It's hard to see what went wrong. Attached patch should add more information.
>>>>>>>
>>>>>>>From 9ea7017e0244ce3db56e0bedd8b1ca0b8b206d78 Mon Sep 17 00:00:00 2001
>>>>>>>From: Pavel Reichl <preichl at redhat.com>
>>>>>>>Date: Tue, 11 Aug 2015 12:25:22 -0400
>>>>>>>Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
>>>>>>>
>>>>>>>---
>>>>>>>src/db/sysdb_ops.c | 14 ++++++++++++--
>>>>>>>1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
>>>>>>>index d1d43ebe6c71611f3371b2f4ccf5f7911909c9de..c551418094311be76acad2882f6a44ec20dedfb9 100644
>>>>>>>--- a/src/db/sysdb_ops.c
>>>>>>>+++ b/src/db/sysdb_ops.c
>>>>>>>@@ -2171,11 +2171,13 @@ sysdb_group_membership_mod(struct sss_domain_info *domain,
>>>>>>>     } else if (type == SYSDB_MEMBER_GROUP) {
>>>>>>>         member_dn = sysdb_group_dn(tmp_ctx, domain, member);
>>>>>>>     } else {
>>>>>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unsupported member_type: %d\n", type);
>>>>>>>         ret = EINVAL;
>>>>>>>         goto done;
>>>>>>>     }
>>>>>>>
>>>>>>>     if (!member_dn) {
>>>>>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Failed to create member_dn.\n");
>>>>>>>         ret = ENOMEM;
>>>>>>Here is a small problem.
>>>>>>
>>>>>>Allocation failed but debug_fn might allocate some memory especially
>>>>>>with enabled journald. So debug messge needn't be printed.
>>>>>>It would be good to release all unused resources (tmp_ctx if available)
>>>>>>or do not print such messages at all.
>>>>>I added the freeing of tmp context, but I think that in case we are really
>>>>>out of memory this might not be enough. The more probable scenario to end up
>>>>>in this branch is IMO that sanitizing DN fails or some other failure.
>>>>>
>>>>So it would be better to add debug message directly to sysdb_dn_sanitize.
>>>>instead of after each invocation of sysdb_dn_sanitize. (sysdb_netgroup_dn)
>>>OK, in logs you can lost one layer of calling hierarchy, but that's still
>>>better than nothing at all.
>>>>So you can distinguish there whether it's a memory allocation failure
>>>>or ldb function failed.
>>>>
>>>>LS
>>>>_______________________________________________
>>>>sssd-devel mailing list
>>>>sssd-devel at lists.fedorahosted.org
>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>From e3283791bd2383a47bcffffdb3631554df589c73 Mon Sep 17 00:00:00 2001
>>>From: Pavel Reichl <preichl at redhat.com>
>>>Date: Tue, 11 Aug 2015 12:25:22 -0400
>>>Subject: [PATCH] SYSDB: add more debug msgs. to group membership code
>>>
>>>---
>>>src/db/sysdb.c     |  9 +++++++++
>>>src/db/sysdb_ops.c | 16 ++++++++++++++--
>>>2 files changed, 23 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/src/db/sysdb.c b/src/db/sysdb.c
>>>index 9da655759c0c35d52854b668693195b3360c5f8b..729d03593cc59e2c9f08e9f1fbe07234cc4a38ab 100644
>>>--- a/src/db/sysdb.c
>>>+++ b/src/db/sysdb.c
>>>@@ -84,6 +84,9 @@ errno_t sysdb_dn_sanitize(TALLOC_CTX *mem_ctx, const char *input,
>>>
>>>     *sanitized = ldb_dn_escape_value(mem_ctx, val);
>>>     if (!*sanitized) {
>>>+        talloc_zfree(val.data);
>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "ldb_dn_escape_value failed for: %s.\n",
>>>+              input);
>>>         ret = ENOMEM;
>>>     }
>>>
>>>@@ -162,6 +165,9 @@ struct ldb_dn *sysdb_user_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *dom,
>>>
>>>     ret = sysdb_dn_sanitize(NULL, name, &clean_name);
>>>     if (ret != EOK) {
>>>+        DEBUG(SSSDBG_MINOR_FAILURE,
>>>+              "sysdb_dn_sanitize failed for: '%s' - [%d]: %s.\n",
>>>+              name, ret, sss_strerror(ret));
>>>         return NULL;
>>>     }
>>>
>>>@@ -188,6 +194,9 @@ struct ldb_dn *sysdb_group_dn(TALLOC_CTX *mem_ctx,
>>>
>>>     ret = sysdb_dn_sanitize(NULL, name, &clean_name);
>>>     if (ret != EOK) {
>>>+        DEBUG(SSSDBG_MINOR_FAILURE,
>>>+              "sysdb_dn_sanitize failed for: %s - [%d]: %s.\n",
>>>+              name, ret, sss_strerror(ret));
>>>         return NULL;
>>>     }
>>I'm sorry but my previous was not probably clear.
>>So my prefference was to add debug message to sysdb_dn_sanitize,
>>which you did. The another option was to add debug messages after
>>sysdb_dn_sanitize. So you did it just patrially and it was not necessary.
>>
>>BTW I checked function ldb_dn_escape_value and it can
>>fail only in case of insufficient memory. Your assumption needn't be correct.
>>So debug messages might not help. They will just complicate
>>situation with additional allocations in debug messages after real ENOMEM
>>situation.
>>
>>IMHO the better way would be to check argument "input"
>>for NULL in functions sysdb_dn_sanitize (and also on other places.
>>We had "Out of memory" false positive reports in past. They were caused by
>>fact that we wanted do allocate copy of NULL
>>(inssuficient validation of inputs).
>>
>>LS
>Lukas, I need this messages to debug user case,
So it means that they are not proved it will help to find issue.
Does it make sense to push them to master?
I often provide scratch build for users with many temporary debug messages.
After I find out which debug message help then I will send reduced patch
with useful debug messages to upstream.

>messages are not perfect, and won't be.
I have never claimed writig debug messages are simple task.
I have few patches on the list with debbug messages which
shoudl be changed (request from reviewers)

>Please provide better patch or pick one from the changed versions
>you requested.
I already mentioned in 1st review that logging ENOMEM messages
does not make a sense without releasing some memory.
Releasing temporary context might help because talloc header
is 96 bytes long (IIRC)

I thought that ldb_dn_escape_value can fail in some cases.
But nether you nor me checked documetation or source code
on first time. I'm sorry for a late notice. But it means that the latest
version would print error message after ENOMEM without releasing memory.
   1. after ldb_dn_escape_value
   2. after sysdb_user_dn and sysdb_group_dn

It seems that you do not want to change debug messages
and neither of patches is suitable for merging to upstream.
(I don't want to say it might not help reveal issue for user)
So I will mark patch as rejected in patchwork.

>I don't think we need to spent any more time nitpicking.
It's not about nitpicking.
I also provided and idea which could be reason of issues.
The ENOMEM is not very likely situation but possible.
It's super simple to limit resources with systemd drop-in files.
@see man systemd-exec

LS



More information about the sssd-devel mailing list