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

Lukas Slebodnik lslebodn at redhat.com
Wed Aug 12 11:06:54 UTC 2015


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



More information about the sssd-devel mailing list