From 73ed43ba85b3635b251ef2bf5ebc71f5d0b9a9b0 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 21 Mar 2025 11:22:03 +0100 Subject: [PATCH 01/11] DEBUG: a new helper that skips backtrace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit if requested debug level isn't set. Meant to be used in hot (performance sensitive) code paths only. Reviewed-by: Alejandro López Reviewed-by: Sumit Bose (cherry picked from commit ca76b7c8f6029ccbf8fc1737fe328cfbb2c5e4b2) (cherry picked from commit 03cef2f42c86e4ea448fd13f7aecc3a802ed89d2) --- src/util/debug.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/util/debug.h b/src/util/debug.h index 54f9d1124ce..f3cfe7b0c3f 100644 --- a/src/util/debug.h +++ b/src/util/debug.h @@ -196,6 +196,18 @@ void sss_debug_fn(const char *file, (level & (SSSDBG_FATAL_FAILURE | \ SSSDBG_CRIT_FAILURE)))) +/* The same as DEBUG but does nothing if requested debug level isn't set, + * thus avoiding logging to the backtrace in this case. + * Meant to be used in hot (performance sensitive) code paths only. + */ +#define DEBUG_CONDITIONAL(level, format, ...) do { \ + if (DEBUG_IS_SET(level)) { \ + sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \ + level, \ + format, ##__VA_ARGS__); \ + } \ +} while (0) + /* not to be used explictly, use 'DEBUG_INIT' instead */ void _sss_debug_init(int dbg_lvl, const char *logger); From 1cb5ea7fd66f0adfb7028e839ebbbc099ea6d988 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Wed, 11 Feb 2026 10:46:46 +0100 Subject: [PATCH 02/11] SDAP: use `DEBUG_CONDITIONAL` in hot path Both `perf` and manual measurement confirms ~6..8% perf gain in the test case: - INITGROUPS lookup for a user that is a member of 5k groups, no groups were cached; - debug_level = 3 - debug_microseconds = true Note `debug_microseconds = true` - without this setting impact isn't that dramatic. Reviewed-by: Justin Stephenson Reviewed-by: Sumit Bose (cherry picked from commit 09e283e22c7ea23b520538e948e3bcb1178dd617) (cherry picked from commit 5596fe54a8ac574ad2ec68b6b55dd12cb4488f5f) --- src/providers/ldap/sdap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index 956eba93a22..e0d197acdf3 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -1824,7 +1824,7 @@ errno_t sdap_get_primary_name(const char *attr_name, } talloc_free(tmp_ctx); - DEBUG(SSSDBG_TRACE_FUNC, "Processing object %s\n", orig_name); + DEBUG_CONDITIONAL(SSSDBG_TRACE_FUNC, "Processing object %s\n", orig_name); *_primary_name = orig_name; From 8dff009c92eddaf587b072159e55e8e785f119ba Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Wed, 11 Feb 2026 13:46:09 +0100 Subject: [PATCH 03/11] UTIL: `sss_tc_utf8_str_tolower()` optimization In vast majority of cases strings are ascii and lowercase. In other cases overhead added should be negligible. Reviewed-by: Justin Stephenson Reviewed-by: Sumit Bose (cherry picked from commit 9a2cf21228b126016f3118c53dc7b47d94a1664d) (cherry picked from commit d3634ccc9ae24a27d70c608d4d72afdda75fdc9d) --- src/util/sss_tc_utf8.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/util/sss_tc_utf8.c b/src/util/sss_tc_utf8.c index 75d5c7136a7..3f6ace06317 100644 --- a/src/util/sss_tc_utf8.c +++ b/src/util/sss_tc_utf8.c @@ -21,12 +21,29 @@ #include "config.h" #include +#include #include #include #include #include "util/util.h" +static inline bool sss_is_ascii_lowercase(const char *s) +{ + if (s == NULL) { + return true; + } + + while (*s != 0) { + if (((unsigned char)*s > 0x7F) || ((*s >= 'A') && (*s <= 'Z'))) { + return false; + } + s++; + } + + return true; +} + /* Expects and returns NULL-terminated string; * result must be freed with sss_utf8_free() */ @@ -41,10 +58,15 @@ char *sss_tc_utf8_str_tolower(TALLOC_CTX *mem_ctx, const char *s) char *lower; char *ret = NULL; - lower = sss_utf8_tolower(s); - if (lower) { - ret = talloc_strdup(mem_ctx, lower); - free(lower); + if (sss_is_ascii_lowercase(s)) { + ret = talloc_strdup(mem_ctx, s); + } + else { + lower = sss_utf8_tolower(s); + if (lower) { + ret = talloc_strdup(mem_ctx, lower); + free(lower); + } } return ret; From 8ddb0ebbb4b493ddf4232fb281bf0bb1b9893ed7 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Wed, 11 Feb 2026 21:02:08 +0100 Subject: [PATCH 04/11] UTIL: `sss_create_internal_fqname()` optimization (caching) This helper is heavily used, including in hot paths. Since number of domains used is very limited, hash table used for caching should be very small and lookup much more efficient as compared with `sss_tc_utf8_str_tolower()` Assisted-by: Claude Code (Opus 4.6) Reviewed-by: Justin Stephenson Reviewed-by: Sumit Bose (cherry picked from commit a5b77e42916bcaebb898d01932c9247ca129bbbf) (cherry picked from commit 8679758d4e9631dcc6ce35115e2cfd4b593bc55c) --- src/util/usertools.c | 66 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/src/util/usertools.c b/src/util/usertools.c index 8084760a03e..1b2a7645c6a 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -660,29 +660,79 @@ errno_t sss_parse_internal_fqname(TALLOC_CTX *mem_ctx, return ret; } +/* This is a wrapper around `sss_tc_utf8_str_tolower()` that + * maintains run time cache. + */ +static const char *sss_get_lc_dom_name(const char *dom_name) +{ + static TALLOC_CTX *cache_ctx; + static hash_table_t *lc_dom_name_cache; + hash_key_t key; + hash_value_t value; + char *lc_dom_name; + int hret; + + key.type = HASH_KEY_CONST_STRING; + key.c_str = dom_name; + + if (lc_dom_name_cache != NULL) { + hret = hash_lookup(lc_dom_name_cache, &key, &value); + if (hret == HASH_SUCCESS) { + return (const char *)value.ptr; + } + } else { + cache_ctx = talloc_new(NULL); + if (cache_ctx == NULL) { + return NULL; + } + hret = hash_create(0, &lc_dom_name_cache, NULL, NULL); + if (hret != HASH_SUCCESS) { + DEBUG(SSSDBG_CRIT_FAILURE, + "hash_create() failed [%s]\n", hash_error_string(hret)); + lc_dom_name_cache = NULL; + talloc_zfree(cache_ctx); + return NULL; + } + } + + lc_dom_name = sss_tc_utf8_str_tolower(cache_ctx, dom_name); + if (lc_dom_name == NULL) { + return NULL; + } + + value.type = HASH_VALUE_PTR; + value.ptr = lc_dom_name; + + hret = hash_enter(lc_dom_name_cache, &key, &value); + if (hret != HASH_SUCCESS) { + DEBUG(SSSDBG_CRIT_FAILURE, + "hash_enter() failed [%s]\n", hash_error_string(hret)); + talloc_free(lc_dom_name); + return NULL; + } + + return lc_dom_name; +} + /* Creates internal fqname in format shortname@domname. * The domain portion is lowercased. */ char *sss_create_internal_fqname(TALLOC_CTX *mem_ctx, const char *shortname, const char *dom_name) { - char *lc_dom_name; - char *fqname = NULL; + const char *lc_dom_name; if (shortname == NULL || dom_name == NULL) { /* Avoid allocating null@null */ return NULL; } - lc_dom_name = sss_tc_utf8_str_tolower(mem_ctx, dom_name); + lc_dom_name = sss_get_lc_dom_name(dom_name); if (lc_dom_name == NULL) { - goto done; + return NULL; } - fqname = talloc_asprintf(mem_ctx, "%s@%s", shortname, lc_dom_name); - talloc_free(lc_dom_name); -done: - return fqname; + return talloc_asprintf(mem_ctx, "%s@%s", shortname, lc_dom_name); } /* Creates a list of internal fqnames in format shortname@domname. From d7e1cebe14867e3ca733f830c015d579f09866a3 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 16 Feb 2026 18:38:27 +0100 Subject: [PATCH 05/11] sdap: eliminate O(N^2) loop in `sdap_add_incomplete_groups()` `sdap_add_incomplete_groups()` had two separate steps: first it iterated the group name list checking each against sysdb to build a 'missing' list, then for each missing group it scanned the entire 'ldap_groups' array calling to find matching LDAP attributes. This resulted in O(N^2) behavior when all groups were missing (i.e. empty cache). Replace this with a single O(N) loop that iterates 'ldap_groups' directly: check sysdb, and if missing create the incomplete entry immediately. The 'sysdb_groupnames' parameter is removed as it is not used anymore. This patch also has an interesting side effect: it also makes `sysdb_update_members()` executed in the `sdap_initgr_common_store()` after `sdap_add_incomplete_groups()` faster. Most probably this is because previosuly O(N^2) allocations of `groupname` (by `sdap_get_group_primary_name()`) trashed memory, purging ldb/tdb data from the cache. Implementation assisted-by: Claude Code (Opus 4.6) Reviewed-by: Justin Stephenson Reviewed-by: Sumit Bose (cherry picked from commit f91c7bbc38e41eeb31f2132acc7263bd4ac9d47c) (cherry picked from commit 281ee45f86d4a59dfced076dd0081acded3d2bd1) --- src/providers/ldap/sdap_async_groups.c | 18 +- src/providers/ldap/sdap_async_initgroups.c | 346 +++++++++------------ src/providers/ldap/sdap_async_private.h | 1 - 3 files changed, 147 insertions(+), 218 deletions(-) diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index f36e5c5837a..e2ca9328fe1 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -1943,7 +1943,7 @@ static void sdap_get_groups_process(struct tevent_req *subreq) bool next_base = false; size_t count; struct sysdb_attrs **groups; - char **sysdb_groupnamelist; + ret = sdap_get_and_parse_generic_recv(subreq, state, &count, &groups); @@ -1999,22 +1999,8 @@ static void sdap_get_groups_process(struct tevent_req *subreq) } if (state->no_members) { - ret = sdap_get_primary_fqdn_list(state->dom, state, - state->groups, state->count, - state->opts->group_map[SDAP_AT_GROUP_NAME].name, - state->opts->group_map[SDAP_AT_GROUP_OBJECTSID].name, - state->opts->idmap_ctx, - &sysdb_groupnamelist); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "sysdb_attrs_primary_name_list failed.\n"); - tevent_req_error(req, ret); - return; - } - ret = sdap_add_incomplete_groups(state->sysdb, state->dom, state->opts, - sysdb_groupnamelist, state->groups, - state->count); + state->groups, state->count); if (ret == EOK) { DEBUG(SSSDBG_TRACE_LIBS, "Writing only group data without members was successful.\n"); diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c index 8ce1f6cd484..25d2a29e0c3 100644 --- a/src/providers/ldap/sdap_async_initgroups.c +++ b/src/providers/ldap/sdap_async_initgroups.c @@ -33,18 +33,16 @@ errno_t sdap_add_incomplete_groups(struct sysdb_ctx *sysdb, struct sss_domain_info *domain, struct sdap_options *opts, - char **sysdb_groupnames, struct sysdb_attrs **ldap_groups, int ldap_groups_count) { TALLOC_CTX *tmp_ctx; struct ldb_message *msg; - int i, mi, ai; - const char *groupname; - const char *original_dn; + int i; + const char *groupname = NULL; + const char *original_dn = NULL; const char *uuid = NULL; - char **missing; - gid_t gid; + gid_t gid = 0; int ret; errno_t sret; bool in_transaction = false; @@ -61,203 +59,172 @@ errno_t sdap_add_incomplete_groups(struct sysdb_ctx *sysdb, tmp_ctx = talloc_new(NULL); if (!tmp_ctx) return ENOMEM; - missing = talloc_array(tmp_ctx, char *, ldap_groups_count+1); - if (!missing) { - ret = ENOMEM; + use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(opts->idmap_ctx, + domain->name, + domain->domain_id); + + ret = sysdb_transaction_start(sysdb); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Cannot start sysdb transaction [%d]: %s\n", + ret, strerror(ret)); goto done; } - mi = 0; + in_transaction = true; - for (i=0; sysdb_groupnames[i]; i++) { - subdomain = find_domain_by_object_name(domain, sysdb_groupnames[i]); + now = time(NULL); + for (i = 0; i < ldap_groups_count; i++) { + gid = 0; + talloc_zfree(sid_str); + talloc_zfree(groupname); + original_dn = NULL; /* don't free - this points to 'ldap_groups' internals */ + uuid = NULL; + ret = sdap_get_group_primary_name(tmp_ctx, opts, ldap_groups[i], + domain, &groupname); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "The group has no name attribute\n"); + goto done; + } + + subdomain = find_domain_by_object_name(domain, groupname); if (subdomain == NULL) { subdomain = domain; } - ret = sysdb_search_group_by_name(tmp_ctx, subdomain, sysdb_groupnames[i], NULL, - &msg); + + ret = sysdb_search_group_by_name(tmp_ctx, subdomain, groupname, + NULL, &msg); if (ret == EOK) { continue; - } else if (ret == ENOENT) { - missing[mi] = talloc_strdup(missing, sysdb_groupnames[i]); - DEBUG(SSSDBG_TRACE_LIBS, "Group #%d [%s][%s] is not cached, " \ - "need to add a fake entry\n", - i, sysdb_groupnames[i], missing[mi]); - mi++; - continue; } else if (ret != ENOENT) { - DEBUG(SSSDBG_CRIT_FAILURE, "search for group failed [%d]: %s\n", - ret, strerror(ret)); + DEBUG(SSSDBG_CRIT_FAILURE, + "search for group failed [%d]: %s\n", + ret, strerror(ret)); goto done; } - } - missing[mi] = NULL; - /* All groups are cached, nothing to do */ - if (mi == 0) { - ret = EOK; - goto done; - } + DEBUG(SSSDBG_TRACE_LIBS, "Group #%d [%s] is not cached, " + "need to add a fake entry\n", i, groupname); - use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(opts->idmap_ctx, - domain->name, - domain->domain_id); + posix = true; - ret = sysdb_transaction_start(sysdb); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Cannot start sysdb transaction [%d]: %s\n", - ret, strerror(ret)); - goto done; - } - in_transaction = true; + ret = sdap_attrs_get_sid_str( + tmp_ctx, opts->idmap_ctx, ldap_groups[i], + opts->group_map[SDAP_AT_GROUP_OBJECTSID].sys_name, + &sid_str); + if (ret != EOK && ret != ENOENT) goto done; + + if (use_id_mapping) { + if (sid_str == NULL) { + DEBUG(SSSDBG_MINOR_FAILURE, "No SID for group [%s] " + "while id-mapping.\n", + groupname); + ret = EINVAL; + goto done; + } + DEBUG(SSSDBG_TRACE_LIBS, + "Mapping group [%s] objectSID to unix ID\n", groupname); - now = time(NULL); - for (i=0; missing[i]; i++) { - /* The group is not in sysdb, need to add a fake entry */ - for (ai=0; ai < ldap_groups_count; ai++) { - ret = sdap_get_group_primary_name(tmp_ctx, opts, ldap_groups[ai], - domain, &groupname); - if (ret != EOK) { + DEBUG(SSSDBG_TRACE_INTERNAL, + "Group [%s] has objectSID [%s]\n", + groupname, sid_str); + + /* Convert the SID into a UNIX group ID */ + ret = sdap_idmap_sid_to_unix(opts->idmap_ctx, sid_str, + &gid); + if (ret == EOK) { + DEBUG(SSSDBG_TRACE_INTERNAL, + "Group [%s] has mapped gid [%lu]\n", + groupname, (unsigned long)gid); + } else { + posix = false; + gid = 0; + + DEBUG(SSSDBG_TRACE_INTERNAL, + "Group [%s] cannot be mapped. " + "Treating as a non-POSIX group\n", + groupname); + } + + } else { + ret = sysdb_attrs_get_uint32_t(ldap_groups[i], + SYSDB_GIDNUM, + &gid); + if (ret == ENOENT || (ret == EOK && gid == 0)) { + DEBUG(SSSDBG_TRACE_LIBS, "The group %s gid was %s\n", + groupname, ret == ENOENT ? "missing" : "zero"); + DEBUG(SSSDBG_TRACE_FUNC, + "Marking group %s as non-POSIX and setting GID=0!\n", + groupname); + gid = 0; + posix = false; + } else if (ret) { DEBUG(SSSDBG_CRIT_FAILURE, - "The group has no name attribute\n"); + "The GID attribute is malformed\n"); goto done; } + } - if (strcmp(groupname, missing[i]) == 0) { - posix = true; - - ret = sdap_attrs_get_sid_str( - tmp_ctx, opts->idmap_ctx, ldap_groups[ai], - opts->group_map[SDAP_AT_GROUP_OBJECTSID].sys_name, - &sid_str); - if (ret != EOK && ret != ENOENT) goto done; - - if (use_id_mapping) { - if (sid_str == NULL) { - DEBUG(SSSDBG_MINOR_FAILURE, "No SID for group [%s] " \ - "while id-mapping.\n", - groupname); - ret = EINVAL; - goto done; - } - - DEBUG(SSSDBG_TRACE_LIBS, - "Mapping group [%s] objectSID to unix ID\n", groupname); - - DEBUG(SSSDBG_TRACE_INTERNAL, - "Group [%s] has objectSID [%s]\n", - groupname, sid_str); - - /* Convert the SID into a UNIX group ID */ - ret = sdap_idmap_sid_to_unix(opts->idmap_ctx, sid_str, - &gid); - if (ret == EOK) { - DEBUG(SSSDBG_TRACE_INTERNAL, - "Group [%s] has mapped gid [%lu]\n", - groupname, (unsigned long)gid); - } else { - posix = false; - gid = 0; - - DEBUG(SSSDBG_TRACE_INTERNAL, - "Group [%s] cannot be mapped. " - "Treating as a non-POSIX group\n", - groupname); - } - - } else { - ret = sysdb_attrs_get_uint32_t(ldap_groups[ai], - SYSDB_GIDNUM, - &gid); - if (ret == ENOENT || (ret == EOK && gid == 0)) { - DEBUG(SSSDBG_TRACE_LIBS, "The group %s gid was %s\n", - groupname, ret == ENOENT ? "missing" : "zero"); - DEBUG(SSSDBG_TRACE_FUNC, - "Marking group %s as non-POSIX and setting GID=0!\n", - groupname); - gid = 0; - posix = false; - } else if (ret) { - DEBUG(SSSDBG_CRIT_FAILURE, - "The GID attribute is malformed\n"); - goto done; - } - } - - ret = sysdb_attrs_get_string(ldap_groups[ai], - SYSDB_ORIG_DN, - &original_dn); - if (ret) { - DEBUG(SSSDBG_FUNC_DATA, - "The group has no original DN\n"); - original_dn = NULL; - } - - ret = sysdb_handle_original_uuid( - opts->group_map[SDAP_AT_GROUP_UUID].def_name, - ldap_groups[ai], - opts->group_map[SDAP_AT_GROUP_UUID].sys_name, - ldap_groups[ai], "uniqueIDstr"); - if (ret != EOK) { - DEBUG((ret == ENOENT) ? SSSDBG_TRACE_ALL : SSSDBG_MINOR_FAILURE, - "Failed to retrieve UUID [%d][%s].\n", - ret, sss_strerror(ret)); - } + ret = sysdb_attrs_get_string(ldap_groups[i], + SYSDB_ORIG_DN, + &original_dn); + if (ret) { + DEBUG(SSSDBG_FUNC_DATA, + "The group has no original DN\n"); + original_dn = NULL; + } - ret = sysdb_attrs_get_string(ldap_groups[ai], - "uniqueIDstr", - &uuid); - if (ret) { - DEBUG(SSSDBG_FUNC_DATA, - "The group has no UUID\n"); - uuid = NULL; - } + ret = sysdb_handle_original_uuid( + opts->group_map[SDAP_AT_GROUP_UUID].def_name, + ldap_groups[i], + opts->group_map[SDAP_AT_GROUP_UUID].sys_name, + ldap_groups[i], "uniqueIDstr"); + if (ret != EOK) { + DEBUG((ret == ENOENT) ? SSSDBG_TRACE_ALL : SSSDBG_MINOR_FAILURE, + "Failed to retrieve UUID [%d][%s].\n", + ret, sss_strerror(ret)); + } - ret = sdap_check_ad_group_type(domain, opts, ldap_groups[ai], - groupname, &need_filter); - if (ret != EOK) { - goto done; - } + ret = sysdb_attrs_get_string(ldap_groups[i], + "uniqueIDstr", + &uuid); + if (ret) { + DEBUG(SSSDBG_FUNC_DATA, + "The group has no UUID\n"); + uuid = NULL; + } - if (need_filter) { - posix = false; - gid = 0; - } + ret = sdap_check_ad_group_type(domain, opts, ldap_groups[i], + groupname, &need_filter); + if (ret != EOK) { + goto done; + } - DEBUG(SSSDBG_TRACE_INTERNAL, - "Adding fake group %s to sysdb\n", groupname); - subdomain = find_domain_by_object_name(domain, groupname); - if (subdomain == NULL) { - subdomain = domain; - } - ret = sysdb_add_incomplete_group(subdomain, groupname, gid, - original_dn, sid_str, - uuid, posix, now); - if (ret == ERR_GID_DUPLICATED) { - /* In case o group id-collision, do: - * - Delete the group from sysdb - * - Add the new incomplete group - * - Notify the NSS responder that the entry has also to be - * removed from the memory cache - */ - ret = sdap_handle_id_collision_for_incomplete_groups( - opts->dp, subdomain, groupname, gid, - original_dn, sid_str, uuid, posix, - now); - } + if (need_filter) { + posix = false; + gid = 0; + } - if (ret != EOK) { - goto done; - } - break; - } + DEBUG(SSSDBG_TRACE_INTERNAL, + "Adding fake group %s to sysdb\n", groupname); + ret = sysdb_add_incomplete_group(subdomain, groupname, gid, + original_dn, sid_str, + uuid, posix, now); + if (ret == ERR_GID_DUPLICATED) { + /* In case of group id-collision, do: + * - Delete the group from sysdb + * - Add the new incomplete group + * - Notify the NSS responder that the entry has also to be + * removed from the memory cache + */ + ret = sdap_handle_id_collision_for_incomplete_groups( + opts->dp, subdomain, groupname, gid, + original_dn, sid_str, uuid, posix, + now); } - if (ai == ldap_groups_count) { - DEBUG(SSSDBG_OP_FAILURE, - "Group %s not present in LDAP\n", missing[i]); - ret = EINVAL; + if (ret != EOK) { goto done; } } @@ -350,8 +317,7 @@ int sdap_initgr_common_store(struct sysdb_ctx *sysdb, */ if (add_groups && add_groups[0]) { ret = sdap_add_incomplete_groups(sysdb, domain, opts, - add_groups, ldap_groups, - ldap_groups_count); + ldap_groups, ldap_groups_count); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Adding incomplete groups failed\n"); goto done; @@ -666,27 +632,8 @@ sdap_nested_groups_store(struct sysdb_ctx *sysdb, unsigned long count) { errno_t ret, tret; - TALLOC_CTX *tmp_ctx; - char **groupnamelist = NULL; bool in_transaction = false; - tmp_ctx = talloc_new(NULL); - if (!tmp_ctx) return ENOMEM; - - if (count > 0) { - ret = sdap_get_primary_fqdn_list(domain, tmp_ctx, groups, count, - opts->group_map[SDAP_AT_GROUP_NAME].name, - opts->group_map[SDAP_AT_GROUP_OBJECTSID].name, - opts->idmap_ctx, - &groupnamelist); - if (ret != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, - "sysdb_attrs_primary_name_list failed [%d]: %s\n", - ret, strerror(ret)); - goto done; - } - } - ret = sysdb_transaction_start(sysdb); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to start transaction\n"); @@ -694,8 +641,7 @@ sdap_nested_groups_store(struct sysdb_ctx *sysdb, } in_transaction = true; - ret = sdap_add_incomplete_groups(sysdb, domain, opts, groupnamelist, - groups, count); + ret = sdap_add_incomplete_groups(sysdb, domain, opts, groups, count); if (ret != EOK) { DEBUG(SSSDBG_TRACE_FUNC, "Could not add incomplete groups [%d]: %s\n", ret, strerror(ret)); @@ -717,8 +663,6 @@ sdap_nested_groups_store(struct sysdb_ctx *sysdb, DEBUG(SSSDBG_CRIT_FAILURE, "Failed to cancel transaction\n"); } } - - talloc_free(tmp_ctx); return ret; } diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h index 90ed3656735..bc17b31a2b5 100644 --- a/src/providers/ldap/sdap_async_private.h +++ b/src/providers/ldap/sdap_async_private.h @@ -157,7 +157,6 @@ sdap_nested_group_lookup_external_recv(TALLOC_CTX *mem_ctx, errno_t sdap_add_incomplete_groups(struct sysdb_ctx *sysdb, struct sss_domain_info *domain, struct sdap_options *opts, - char **sysdb_groupnames, struct sysdb_attrs **ldap_groups, int ldap_groups_count); From 07c584cb513fbed371a54078ad63b2e20f3c7328 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Wed, 25 Feb 2026 18:40:26 +0100 Subject: [PATCH 06/11] LDAP: free tmp var within the loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit inside `sdap_add_incomplete_groups()` to avoid memory pressure / cache trashing if handling large groups set. Reviewed-by: Tomáš Halman (cherry picked from commit 8c1e20b23ce0412a7cf7e452b20db6896492a0a7) (cherry picked from commit e830df8e5bda898f9f677ff655a36a47f53784a1) --- src/providers/ldap/sdap_async_initgroups.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c index 25d2a29e0c3..c4000a7fd8f 100644 --- a/src/providers/ldap/sdap_async_initgroups.c +++ b/src/providers/ldap/sdap_async_initgroups.c @@ -37,7 +37,7 @@ errno_t sdap_add_incomplete_groups(struct sysdb_ctx *sysdb, int ldap_groups_count) { TALLOC_CTX *tmp_ctx; - struct ldb_message *msg; + struct ldb_message *msg = NULL; int i; const char *groupname = NULL; const char *original_dn = NULL; @@ -77,6 +77,7 @@ errno_t sdap_add_incomplete_groups(struct sysdb_ctx *sysdb, gid = 0; talloc_zfree(sid_str); talloc_zfree(groupname); + talloc_zfree(msg); original_dn = NULL; /* don't free - this points to 'ldap_groups' internals */ uuid = NULL; ret = sdap_get_group_primary_name(tmp_ctx, opts, ldap_groups[i], From 16c2f0a62bf97424b94392d35814ed25f867238a Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Wed, 18 Feb 2026 13:37:29 +0100 Subject: [PATCH 07/11] memberOf plugin: redundant comparison removed 'msg->dn' (== 'addop->entry_dn') is already filtered out from 'parents->dns[i]' at the beginning of `mbof_add_operation()` Reviewed-by: Iker Pedrosa Reviewed-by: Sumit Bose (cherry picked from commit c1eced627e3d6edb33133f3d52557d257605870b) (cherry picked from commit a0c67ac805092f9fbcd9aa92105264ce369a0bc9) --- src/ldb_modules/memberof.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index 21a175cb353..51606c7866d 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -938,7 +938,6 @@ static int mbof_add_operation(struct mbof_add_operation *addop) return LDB_ERR_OPERATIONS_ERROR; } for (i = 0, j = 0; i < parents->num; i++) { - if (ldb_dn_compare(parents->dns[i], msg->dn) == 0) continue; val = ldb_dn_get_linearized(parents->dns[i]); el->values[j].length = strlen(val); el->values[j].data = (uint8_t *)talloc_strdup(el->values, val); From 81bf68d99bce7ea835a173bdebfb802a11d6c207 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Wed, 18 Feb 2026 14:53:57 +0100 Subject: [PATCH 08/11] memberOf plugin: swap instead of a shift when removing a duplicate. DNs order doesn't matter. Reviewed-by: Iker Pedrosa Reviewed-by: Sumit Bose (cherry picked from commit 7a7480e841ebcbf054d8c8a23c2bf3b9faf30b47) (cherry picked from commit 52c71afbdaa0cb3f946b5b225ec40dae17ca9342) --- src/ldb_modules/memberof.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index 51606c7866d..8bb43abe9c3 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -829,9 +829,7 @@ static int mbof_add_operation(struct mbof_add_operation *addop) } if (j < parents->num) { /* remove duplicate */ - for (;j+1 < parents->num; j++) { - parents->dns[j] = parents->dns[j+1]; - } + parents->dns[j] = parents->dns[parents->num - 1]; parents->num--; } } From 8f2321b13c0406c5c344ebe1b7ada66928a8793a Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 27 Feb 2026 16:30:53 +0100 Subject: [PATCH 09/11] memberOf plugin: avoid `ldb_dn_compare()` in `mbof_add_operation()` `ldb_dn_compare()` here is heavy because when DNs are not equal (vast majority of cases), it performs `ldb_dn_casefold_internal()` to return -1 or 1, but it's not important in this context. Note that in general using `str*cmp()` instead of `ldb_dn_get_linearized()` might yield incorrect results due to different DN representations. But since all 'memberof' DNs originate from sysdb cache / memberof plugin itself, it should be safe replacement in this context. Reviewed-by: Iker Pedrosa Reviewed-by: Sumit Bose (cherry picked from commit 704c31dbcb86266a9ad5cb02c96fc73ca6a2fb95) (cherry picked from commit 999afa26418b428a683849fb7ab576345b110d7d) --- src/ldb_modules/memberof.c | 85 ++++++++++++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 12 deletions(-) diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index 8bb43abe9c3..2a4ef566ce4 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -17,6 +17,9 @@ along with this program. If not, see . */ +#ifndef _GNU_SOURCE +#define _GNU_SOURCE /* For strchrnul() */ +#endif #include #include @@ -761,20 +764,77 @@ static int mbof_next_add_callback(struct ldb_request *req, return LDB_SUCCESS; } +/* based on `ldb_dn_from_ldb_val()` but avoids memcpy */ +static const char *sss_get_linearized_dn_from_ldb_val(const struct ldb_val *strdn) +{ + const char *data; + + if (strdn == NULL || strdn->data == NULL || strdn->length == 0) { + return NULL; + } + + data = (const char *)strdn->data; + + if (data[0] == '<') { + const char *p_save = data; + const char *p = data; + do { + p_save = p; + p = strstr(p, ">;"); + if (p) { + p = p + 2; + } + } while (p); + + if (p_save == data) { + /* Only extended components, no linearized DN */ + return NULL; + } + return p_save; + } + + return data; +} + +__attribute__((always_inline)) +static inline bool sss_linearized_dn_match(const char *dn1, const char *dn2) +{ + const char *comma = NULL; + size_t name_len; + + if ((dn1 == NULL) || (dn2 == NULL)) { + return false; + } + + if (strcasecmp(dn1, dn2) != 0) { + return false; + } + + /* Since sysdb cache treats 'name' case-sensitive, + * perform additional check to be on a safe side. + */ + if (strncasecmp(dn1, "name=", 5) != 0) { + return true; + } + + comma = strchrnul(dn1+5, ','); + name_len = comma - (dn1 + 5); + return (strncmp(dn1+5, dn2+5, name_len) == 0); +} + /* if it is a group, add all members for cascade effect * add memberof attribute to this entry */ static int mbof_add_operation(struct mbof_add_operation *addop) { - TALLOC_CTX *tmp_ctx; struct mbof_ctx *ctx; struct mbof_add_ctx *add_ctx; struct ldb_context *ldb; struct ldb_message_element *el; struct ldb_request *mod_req; struct ldb_message *msg; - struct ldb_dn *elval_dn; + const char *elval_dn; struct ldb_dn *valdn; struct mbof_dn_array *parents; int i, j, ret; @@ -799,7 +859,8 @@ static int mbof_add_operation(struct mbof_add_operation *addop) /* create new parent set for this entry */ for (i = 0; i < addop->parents->num; i++) { /* never add yourself as memberof */ - if (ldb_dn_compare(addop->parents->dns[i], addop->entry_dn) == 0) { + if (sss_linearized_dn_match(ldb_dn_get_linearized(addop->parents->dns[i]), + ldb_dn_get_linearized(addop->entry_dn))) { continue; } parents->dns[parents->num] = addop->parents->dns[i]; @@ -810,19 +871,21 @@ static int mbof_add_operation(struct mbof_add_operation *addop) el = ldb_msg_find_element(addop->entry, DB_MEMBEROF); if (el) { - tmp_ctx = talloc_new(addop); - if (!tmp_ctx) return LDB_ERR_OPERATIONS_ERROR; - for (i = 0; i < el->num_values; i++) { - elval_dn = ldb_dn_from_ldb_val(tmp_ctx, ldb, &el->values[i]); - if (!elval_dn) { + elval_dn = sss_get_linearized_dn_from_ldb_val(&el->values[i]); + if (elval_dn == NULL) { ldb_debug(ldb, LDB_DEBUG_TRACE, "Invalid DN in memberof [%s]", (const char *)el->values[i].data); - talloc_free(tmp_ctx); return LDB_ERR_OPERATIONS_ERROR; } for (j = 0; j < parents->num; j++) { - if (ldb_dn_compare(parents->dns[j], elval_dn) == 0) { + /* Don't use `ldb_dn_compare()` here - + * it is heavy because when DNs are not equal (vast majority of cases) + * it performs `ldb_dn_casefold_internal()` to return -1 or 1, + * but it's not important in this context. + */ + if (sss_linearized_dn_match(ldb_dn_get_linearized(parents->dns[j]), + elval_dn)) { /* duplicate found */ break; } @@ -836,7 +899,6 @@ static int mbof_add_operation(struct mbof_add_operation *addop) if (parents->num == 0) { /* already contains all parents as memberof, skip to next */ - talloc_free(tmp_ctx); talloc_free(addop->entry); addop->entry = NULL; @@ -854,7 +916,6 @@ static int mbof_add_operation(struct mbof_add_operation *addop) LDB_SUCCESS); } } - talloc_free(tmp_ctx); } /* if it is a group add all members */ From 7fe7fb3437bbf24d680f7cdaa6c44c6630a8e3a1 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 20 Mar 2026 13:41:15 +0100 Subject: [PATCH 10/11] SDAP: reduce logger load in the hot path This patch reduced number of *sprintf() keeping the same level of details in the resulting log. Besides, list of attrs being requested was excluded from the backtrace. Reviewed-by: Iker Pedrosa Reviewed-by: Sumit Bose (cherry picked from commit feca02838a8889ceded54fb794e63d7257e0625f) (cherry picked from commit 4c183fbb82562348846bb7714b18c021f3c4b1e9) --- src/providers/ldap/sdap_async.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index ab3572d1d4f..f60e977247a 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -1629,17 +1629,6 @@ static errno_t sdap_get_generic_ext_step(struct tevent_req *req) */ talloc_zfree(state->op); - DEBUG(SSSDBG_TRACE_FUNC, - "calling ldap_search_ext with [%s][%s].\n", - state->filter ? state->filter : "no filter", - state->search_base); - if (state->attrs) { - for (int i = 0; state->attrs[i]; i++) { - DEBUG(SSSDBG_TRACE_LIBS, - "Requesting attrs: [%s]\n", state->attrs[i]); - } - } - disable_paging = dp_opt_get_bool(state->opts->basic, SDAP_DISABLE_PAGING); if (!disable_paging @@ -1690,7 +1679,6 @@ static errno_t sdap_get_generic_ext_step(struct tevent_req *req) } goto done; } - DEBUG(SSSDBG_TRACE_INTERNAL, "ldap_search_ext called, msgid = %d\n", msgid); stat_info = talloc_asprintf(state, "server: [%s] filter: [%s] base: [%s]", sdap_get_server_peer_str_safe(state->sh), @@ -1699,6 +1687,15 @@ static errno_t sdap_get_generic_ext_step(struct tevent_req *req) DEBUG(SSSDBG_OP_FAILURE, "Failed to create info string, ignored.\n"); } + DEBUG(SSSDBG_TRACE_FUNC, "ldap_search_ext called: %s; msgid = %d\n", + (stat_info ? stat_info : "N/A"), msgid); + if (state->attrs) { + for (int i = 0; state->attrs[i]; i++) { + DEBUG_CONDITIONAL(SSSDBG_TRACE_ALL, "Requesting attrs: [%s]\n", + state->attrs[i]); + } + } + ret = sdap_op_add(state, state->ev, state->sh, msgid, stat_info, sdap_get_generic_op_finished, req, state->timeout, From d516674a2f614af55495df7a5ffde9abae2f88b8 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 20 Mar 2026 14:54:18 +0100 Subject: [PATCH 11/11] SDAP: use DEBUG_CONDITIONAL in the hot paths Reviewed-by: Iker Pedrosa Reviewed-by: Sumit Bose (cherry picked from commit 87c7bce1552aec2da064b506aec391c40f365705) (cherry picked from commit d294a73e366ebcfaf14c55a446f99df20f5260e5) --- src/providers/ldap/sdap.c | 2 +- src/providers/ldap/sdap_async.c | 9 +++++---- src/providers/ldap/sdap_range.c | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c index e0d197acdf3..5729651bce7 100644 --- a/src/providers/ldap/sdap.c +++ b/src/providers/ldap/sdap.c @@ -445,7 +445,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx, goto done; } - DEBUG(SSSDBG_TRACE_LIBS, "OriginalDN: [%s].\n", str); + DEBUG_CONDITIONAL(SSSDBG_TRACE_LIBS, "OriginalDN: [%s].\n", str); PROBE(SDAP_PARSE_ENTRY, "OriginalDN", str, strlen(str)); ret = sysdb_attrs_add_string(attrs, SYSDB_ORIG_DN, str); ldap_memfree(str); diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index f60e977247a..9b4b0f162e3 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -220,7 +220,7 @@ static void sdap_process_result(struct tevent_context *ev, void *pvt) * later in this function once we can match the reply with an operation. */ old_chain_id = sss_chain_id_set(0); - DEBUG(SSSDBG_TRACE_INTERNAL, + DEBUG_CONDITIONAL(SSSDBG_TRACE_INTERNAL, "Trace: sh[%p], connected[%d], ops[%p], ldap[%p]\n", sh, (int)sh->connected, sh->ops, sh->ldap); @@ -234,7 +234,7 @@ static void sdap_process_result(struct tevent_context *ev, void *pvt) if (ret == 0) { /* this almost always means we have reached the end of * the list of received messages */ - DEBUG(SSSDBG_TRACE_INTERNAL, "Trace: end of ldap_result list\n"); + DEBUG_CONDITIONAL(SSSDBG_TRACE_INTERNAL, "Trace: end of ldap_result list\n"); return; } @@ -360,7 +360,7 @@ static void sdap_process_message(struct tevent_context *ev, return; } - DEBUG(SSSDBG_TRACE_ALL, + DEBUG_CONDITIONAL(SSSDBG_TRACE_ALL, "Message type: [%s]\n", sdap_ldap_result_str(msgtype)); switch (msgtype) { @@ -471,7 +471,8 @@ static int sdap_op_destructor(void *mem) DLIST_REMOVE(op->sh->ops, op); if (op->done) { - DEBUG(SSSDBG_TRACE_INTERNAL, "Operation %d finished\n", op->msgid); + DEBUG_CONDITIONAL(SSSDBG_TRACE_INTERNAL, + "Operation %d finished\n", op->msgid); return 0; } diff --git a/src/providers/ldap/sdap_range.c b/src/providers/ldap/sdap_range.c index 44c3350db1e..b3337eb227a 100644 --- a/src/providers/ldap/sdap_range.c +++ b/src/providers/ldap/sdap_range.c @@ -54,7 +54,7 @@ errno_t sdap_parse_range(TALLOC_CTX *mem_ctx, } else { ret = EOK; } - DEBUG(SSSDBG_TRACE_INTERNAL, + DEBUG_CONDITIONAL(SSSDBG_TRACE_ALL, "No sub-attributes for [%s]\n", attr_desc); goto done; }