From 75fcdf156390c802a966a8d5a1be2681989256ac Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Thu, 6 Mar 2025 19:56:33 +0100 Subject: [PATCH 01/16] SYSDB: update in sysdb_add_group_member_overrides() Skip function if group->memberUid is empty. In this case there are no user objects in the cache that would have memberOf == group->dn anyway. --- src/db/sysdb_views.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index 28bf64c41b..605ec25a13 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1670,6 +1670,15 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, return EOK; } + if (ldb_msg_find_element(obj, SYSDB_MEMBERUID) == NULL) { + /* empty memberUid list means there are no user objects in + * the cache that would have 'memberOf = obj->dn', + * so get_user_members_recursively() will return an empty list + * anyway (but may consume a lot of CPU in case of a large cache) + */ + return EOK; + } + tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n"); From 48ccf4495390b7df6eb84ef9216fcb03d4023b33 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Thu, 6 Mar 2025 20:57:01 +0100 Subject: [PATCH 02/16] SYSDB: update in sysdb_add_group_member_overrides() Ensure that `get_user_members_recursively()` returns only POSIX users via search filter. This avoids the need to populate and later check SYSDB_UIDNUM attr. --- src/db/sysdb_views.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index 605ec25a13..a3c500271b 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1547,7 +1547,6 @@ static errno_t get_user_members_recursively(TALLOC_CTX *mem_ctx, char *sanitized_name; const char *attrs[] = { - SYSDB_UIDNUM, SYSDB_OVERRIDE_DN, SYSDB_NAME, SYSDB_DEFAULT_OVERRIDE_NAME, @@ -1576,7 +1575,8 @@ static errno_t get_user_members_recursively(TALLOC_CTX *mem_ctx, goto done; } - filter = talloc_asprintf(tmp_ctx, "(&("SYSDB_UC")("SYSDB_MEMBEROF"=%s))", + filter = talloc_asprintf(tmp_ctx, + "(&("SYSDB_UC")("SYSDB_MEMBEROF"=%s)("SYSDB_UIDNUM"=*))", sanitized_name); if (filter == NULL) { DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); @@ -1697,12 +1697,6 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, } for (c = 0; c < res_members->count; c++) { - if (ldb_msg_find_attr_as_uint64(res_members->msgs[c], - SYSDB_UIDNUM, 0) == 0) { - /* Skip non-POSIX-user members i.e. groups and non-POSIX users */ - continue; - } - if (expect_override_dn) { /* Creates new DN object. */ override_dn = ldb_msg_find_attr_as_dn(domain->sysdb->ldb, tmp_ctx, From 4c8e01cad9d28b7a0bde6e4c98cd358d7227a040 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Thu, 6 Mar 2025 21:01:23 +0100 Subject: [PATCH 03/16] SYSDB: debug message fixed --- src/db/sysdb_views.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index a3c500271b..ea8882fdc0 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1773,7 +1773,7 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, val = talloc_steal(obj, memberuid); if (val == NULL) { - DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n"); + DEBUG(SSSDBG_OP_FAILURE, "talloc_steal() failed.\n"); ret = ENOMEM; goto done; } From 1b695174651cf34ba70d99ae3c44c07b7d9720fa Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Thu, 6 Mar 2025 21:03:09 +0100 Subject: [PATCH 04/16] SYSDB: update in sysdb_add_group_member_overrides() Don't read unneeded attributes from override_dn. --- src/db/sysdb_views.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index ea8882fdc0..b590b93315 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1661,7 +1661,7 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, struct ldb_result *res_members; TALLOC_CTX *tmp_ctx; struct ldb_result *override_obj; - static const char *member_attrs[] = SYSDB_PW_ATTRS; + static const char *member_attrs[] = { SYSDB_NAME, NULL }; struct ldb_dn *override_dn = NULL; const char *memberuid; const char *val; From 8dc4a54187d7d67a33de9c7ded49dd09b553e69b Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 7 Mar 2025 20:40:49 +0100 Subject: [PATCH 05/16] SYSDB: update in get_user_members_recursively() Replaced `sysdb_search_entry()` with `sysdb_cache_search_entry()` to avoid `sysdb_merge_msg_list_ts_attrs()` that isn't needed here (timestamps aren't used anyway). --- src/db/sysdb_views.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index b590b93315..f36270b85f 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1584,8 +1584,8 @@ static errno_t get_user_members_recursively(TALLOC_CTX *mem_ctx, goto done; } - ret = sysdb_search_entry(tmp_ctx, dom->sysdb, base_dn, LDB_SCOPE_SUBTREE, - filter, attrs, &count, &msgs); + ret = sysdb_cache_search_entry(tmp_ctx, dom->sysdb->ldb, base_dn, LDB_SCOPE_SUBTREE, + filter, attrs, &count, &msgs); if (ret != EOK) { goto done; } From 594c072dee28c242b309473b44251a78091a2cc5 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 21 Mar 2025 11:22:03 +0100 Subject: [PATCH 06/16] DEBUG: a new helper that skips backtrace if requested debug level isn't set. Meant to be used in hot (performance sensitive) code paths only. --- src/util/debug.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/util/debug.h b/src/util/debug.h index 2b4bf85107..f785d9c262 100644 --- a/src/util/debug.h +++ b/src/util/debug.h @@ -194,6 +194,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 58e274925637b9217e9b94c49c21b7630c039623 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Sat, 8 Mar 2025 14:16:26 +0100 Subject: [PATCH 07/16] Avoid logging to the backtrace unconditionally in hot paths. In case of reading a large group (comparable to entire cache) it accounts for some non trivial CPU time (cca ~6..7%) --- src/db/sysdb_views.c | 9 +++++---- src/responder/common/negcache.c | 2 +- src/util/domain_info_utils.c | 4 ++-- src/util/usertools.c | 2 -- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index f36270b85f..e7e3673ba5 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1760,7 +1760,8 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, } if (memberuid == NULL) { - DEBUG(SSSDBG_TRACE_ALL, "No override name available.\n"); + DEBUG_CONDITIONAL(SSSDBG_TRACE_ALL, "No override name available for %s.\n", + orig_name); memberuid = orig_name; } else { /* add domain name if memberuid is a short name */ @@ -1784,9 +1785,9 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, ret = sysdb_error_to_errno(ret); goto done; } - DEBUG(SSSDBG_TRACE_ALL, "Added [%s] to [%s].\n", memberuid, - OVERRIDE_PREFIX SYSDB_MEMBERUID); - + DEBUG_CONDITIONAL(SSSDBG_TRACE_ALL, + "Added [%s] to ["OVERRIDE_PREFIX SYSDB_MEMBERUID"].\n", + memberuid); } ret = EOK; diff --git a/src/responder/common/negcache.c b/src/responder/common/negcache.c index 469e527963..c299a71276 100644 --- a/src/responder/common/negcache.c +++ b/src/responder/common/negcache.c @@ -96,7 +96,7 @@ static int sss_ncache_check_str(struct sss_nc_ctx *ctx, char *str) char *ep; int ret; - DEBUG(SSSDBG_TRACE_INTERNAL, "Checking negative cache for [%s]\n", str); + DEBUG_CONDITIONAL(SSSDBG_TRACE_INTERNAL, "Checking negative cache for [%s]\n", str); data.dptr = NULL; diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c index 95e2b1af38..677b76ff35 100644 --- a/src/util/domain_info_utils.c +++ b/src/util/domain_info_utils.c @@ -910,8 +910,8 @@ static const char *domain_state_str(struct sss_domain_info *dom) enum sss_domain_state sss_domain_get_state(struct sss_domain_info *dom) { - DEBUG(SSSDBG_TRACE_LIBS, - "Domain %s is %s\n", dom->name, domain_state_str(dom)); + DEBUG_CONDITIONAL(SSSDBG_TRACE_INTERNAL, + "Domain %s is %s\n", dom->name, domain_state_str(dom)); return dom->state; } diff --git a/src/util/usertools.c b/src/util/usertools.c index 0d11a95719..72b0021156 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -508,8 +508,6 @@ calc_flat_name(struct sss_domain_info *domain) s = domain->flat_name; if (s == NULL) { - DEBUG(SSSDBG_FUNC_DATA, "Domain has no flat name set," - "using domain name instead\n"); s = domain->name; } From 5b00a787ecaad8bc71f8430ccf4eea7bf5c98b2c Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 11:42:20 +0100 Subject: [PATCH 08/16] UTIL: sss_parse_internal_fqname() optimization 'tmp_ctx' was removed as it wasn't really used anyway. Code could be changed to make a real use of 'tmp_ctx': to avoid touching '_dom_name' output arg if update of '_shortname' fails. But this is quite unrealistic case and function is in a hot path, so better to avoid unneeded memory manipulations. --- src/util/usertools.c | 37 +++++++++---------------------------- 1 file changed, 9 insertions(+), 28 deletions(-) diff --git a/src/util/usertools.c b/src/util/usertools.c index 72b0021156..85bb0a7fbe 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -608,54 +608,35 @@ errno_t sss_parse_internal_fqname(TALLOC_CTX *mem_ctx, char **_shortname, char **_dom_name) { - errno_t ret; - char *separator; - char *shortname = NULL; - char *dom_name = NULL; + const char *separator; size_t shortname_len; - TALLOC_CTX *tmp_ctx; if (fqname == NULL) { return EINVAL; } - tmp_ctx = talloc_new(NULL); - if (tmp_ctx == NULL) { - return ENOMEM; - } - separator = strrchr(fqname, '@'); if (separator == NULL || *(separator + 1) == '\0' || separator == fqname) { /*The name does not contain name or domain component. */ - ret = ERR_WRONG_NAME_FORMAT; - goto done; + return ERR_WRONG_NAME_FORMAT; } if (_dom_name != NULL) { - dom_name = talloc_strdup(tmp_ctx, separator + 1); - if (dom_name == NULL) { - ret = ENOMEM; - goto done; + *_dom_name = talloc_strdup(mem_ctx, separator + 1); + if (*_dom_name == NULL) { + return ENOMEM; } - - *_dom_name = talloc_steal(mem_ctx, dom_name); } if (_shortname != NULL) { shortname_len = strlen(fqname) - strlen(separator); - shortname = talloc_strndup(tmp_ctx, fqname, shortname_len); - if (shortname == NULL) { - ret = ENOMEM; - goto done; + *_shortname = talloc_strndup(mem_ctx, fqname, shortname_len); + if (*_shortname == NULL) { + return ENOMEM; } - - *_shortname = talloc_steal(mem_ctx, shortname); } - ret = EOK; -done: - talloc_free(tmp_ctx); - return ret; + return EOK; } /* Creates internal fqname in format shortname@domname. From 444fd722f3d33c9364c4df9d735804e06cd3bf47 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 11:47:27 +0100 Subject: [PATCH 09/16] UTIL: sss_parse_internal_fqname() optimization Avoid unneeded strlen()'s --- src/util/usertools.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/usertools.c b/src/util/usertools.c index 85bb0a7fbe..25ae45dd17 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -629,7 +629,7 @@ errno_t sss_parse_internal_fqname(TALLOC_CTX *mem_ctx, } if (_shortname != NULL) { - shortname_len = strlen(fqname) - strlen(separator); + shortname_len = (size_t)(separator - fqname); *_shortname = talloc_strndup(mem_ctx, fqname, shortname_len); if (*_shortname == NULL) { return ENOMEM; From f217b01a8783ef687b93aaa82c7b5fb582fd7e77 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 12:01:02 +0100 Subject: [PATCH 10/16] UTIL: sized_domain_name() optimization Don't use sss_parse_internal_fqname() as domain name copy isn't needed. --- src/responder/common/responder_common.c | 33 ++++++------------------- src/util/util.h | 16 ++++++++++++ 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index c2f0273873..611006e32c 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -1692,37 +1692,20 @@ int sized_domain_name(TALLOC_CTX *mem_ctx, const char *member_name, struct sized_string **_name) { - TALLOC_CTX *tmp_ctx = NULL; - errno_t ret; - char *domname; + const char *domain; struct sss_domain_info *member_dom; - tmp_ctx = talloc_new(NULL); - if (tmp_ctx == NULL) { - return ENOMEM; - } - - ret = sss_parse_internal_fqname(tmp_ctx, member_name, NULL, &domname); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "sss_parse_internal_fqname failed\n"); - goto done; - } - - if (domname == NULL) { - ret = ERR_WRONG_NAME_FORMAT; - goto done; + domain = sss_get_domain_internal_fqname(member_name); + if (domain == NULL) { + return ERR_WRONG_NAME_FORMAT; } member_dom = find_domain_by_name(get_domains_head(rctx->domains), - domname, true); + domain, true); if (member_dom == NULL) { - ret = ERR_DOMAIN_NOT_FOUND; - goto done; + return ERR_DOMAIN_NOT_FOUND; } - ret = sized_output_name(mem_ctx, rctx, member_name, - member_dom, _name); -done: - talloc_free(tmp_ctx); - return ret; + return sized_output_name(mem_ctx, rctx, member_name, + member_dom, _name); } diff --git a/src/util/util.h b/src/util/util.h index 43cddcdbe5..cb8ab6b62d 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -319,6 +319,22 @@ errno_t sss_parse_internal_fqname(TALLOC_CTX *mem_ctx, char **_shortname, char **_dom_name); +/* Accepts fqname in the format shortname@domname only + * and returns a pointer to domain part or NULL if not found. + */ +__attribute__((always_inline)) +static inline const char *sss_get_domain_internal_fqname(const char *fqname) +{ + const char *separator = strrchr(fqname, '@'); + + if (separator == NULL || *(separator + 1) == '\0' || separator == fqname) { + /*The name does not contain name or domain component. */ + return NULL; + } + + return (separator + 1); +} + /* Creates internal fqname in format shortname@domname. * The domain portion is lowercased. */ char *sss_create_internal_fqname(TALLOC_CTX *mem_ctx, From 6f9abf85e07b7ac5656f0383b8d1611c3792b569 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 12:23:25 +0100 Subject: [PATCH 11/16] RESPONDER: sized_output_name() optimization Avoid alloc/free tmp_ctx. Not much benefits but a function is in a hot path. --- src/responder/common/responder_common.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c index 611006e32c..4d04a70e3a 100644 --- a/src/responder/common/responder_common.c +++ b/src/responder/common/responder_common.c @@ -1657,20 +1657,13 @@ int sized_output_name(TALLOC_CTX *mem_ctx, struct sss_domain_info *name_dom, struct sized_string **_name) { - TALLOC_CTX *tmp_ctx = NULL; errno_t ret; char *name_str; struct sized_string *name; - tmp_ctx = talloc_new(NULL); - if (tmp_ctx == NULL) { - return ENOMEM; - } - - name = talloc_zero(tmp_ctx, struct sized_string); + name = talloc_zero(mem_ctx, struct sized_string); if (name == NULL) { - ret = ENOMEM; - goto done; + return ENOMEM; } ret = sss_output_fqname(name, name_dom, orig_name, @@ -1680,10 +1673,15 @@ int sized_output_name(TALLOC_CTX *mem_ctx, } to_sized_string(name, name_str); - *_name = talloc_steal(mem_ctx, name); ret = EOK; + done: - talloc_zfree(tmp_ctx); + if (ret == EOK) { + *_name = name; + } else { + talloc_free(name); + } + return ret; } From e6c8294edb88b6fba00aa60938cfa364dad13443 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 17:35:39 +0100 Subject: [PATCH 12/16] UTIL: sss_output_name() optimization Avoid unnecessary string copy. --- src/util/sss_tc_utf8.c | 2 +- src/util/usertools.c | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/util/sss_tc_utf8.c b/src/util/sss_tc_utf8.c index 75d5c7136a..345083a713 100644 --- a/src/util/sss_tc_utf8.c +++ b/src/util/sss_tc_utf8.c @@ -28,7 +28,7 @@ #include "util/util.h" /* Expects and returns NULL-terminated string; - * result must be freed with sss_utf8_free() + * result must be freed with free() */ static inline char *sss_utf8_tolower(const char *s) { diff --git a/src/util/usertools.c b/src/util/usertools.c index 25ae45dd17..bce59a7eaf 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -723,11 +723,15 @@ char *sss_output_name(TALLOC_CTX *mem_ctx, goto done; } - outname = sss_get_cased_name(tmp_ctx, shortname, case_sensitive); - if (outname == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, - "sss_get_cased_name failed, skipping\n"); - goto done; + if (!case_sensitive) { + outname = sss_get_cased_name(tmp_ctx, shortname, false); + if (outname == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, + "sss_get_cased_name failed, skipping\n"); + goto done; + } + } else { /* avoid talloc_strdup() inside sss_get_cased_name() */ + outname = shortname; } outname = sss_replace_space(tmp_ctx, outname, replace_space); From 2b4937946909aea50fe4620c1274f30a2d0cf313 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 20:13:12 +0100 Subject: [PATCH 13/16] RESPONDER: delete sss_resp_create_fqname() Function wasn't used since ed891c0c55985cd25de05f65e82debf4452987e1 --- src/responder/common/responder.h | 6 ---- src/responder/common/responder_utils.c | 43 -------------------------- 2 files changed, 49 deletions(-) diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h index ad61ae61fc..e2dacc289f 100644 --- a/src/responder/common/responder.h +++ b/src/responder/common/responder.h @@ -362,12 +362,6 @@ errno_t sss_parse_inp_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx, const char **parse_attr_list_ex(TALLOC_CTX *mem_ctx, const char *conf_str, const char **defaults); -char *sss_resp_create_fqname(TALLOC_CTX *mem_ctx, - struct resp_ctx *rctx, - struct sss_domain_info *dom, - bool name_is_upn, - const char *orig_name); - errno_t sss_resp_populate_cr_domains(struct resp_ctx *rctx); const char * diff --git a/src/responder/common/responder_utils.c b/src/responder/common/responder_utils.c index f20482ad35..926cbbe6e8 100644 --- a/src/responder/common/responder_utils.c +++ b/src/responder/common/responder_utils.c @@ -155,49 +155,6 @@ const char **parse_attr_list_ex(TALLOC_CTX *mem_ctx, const char *conf_str, return res; } -char *sss_resp_create_fqname(TALLOC_CTX *mem_ctx, - struct resp_ctx *rctx, - struct sss_domain_info *dom, - bool name_is_upn, - const char *orig_name) -{ - TALLOC_CTX *tmp_ctx; - char *name; - - tmp_ctx = talloc_new(NULL); - if (tmp_ctx == NULL) { - return NULL; - } - - name = sss_get_cased_name(tmp_ctx, orig_name, dom->case_sensitive); - if (name == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "sss_get_cased_name failed\n"); - talloc_free(tmp_ctx); - return NULL; - } - - name = sss_reverse_replace_space(tmp_ctx, name, rctx->override_space); - if (name == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "sss_reverse_replace_space failed\n"); - talloc_free(tmp_ctx); - return NULL; - } - - - if (name_is_upn == false) { - name = sss_create_internal_fqname(tmp_ctx, name, dom->name); - if (name == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname failed\n"); - talloc_free(tmp_ctx); - return NULL; - } - } - - name = talloc_steal(mem_ctx, name); - talloc_free(tmp_ctx); - return name; -} - struct resp_resolve_group_names_state { struct tevent_context *ev; struct resp_ctx *rctx; From 88900ab85469edf7617600b2f5f7214e736c765c Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Mon, 10 Mar 2025 21:27:55 +0100 Subject: [PATCH 14/16] UTIL: remake sss_*replace_space() to inplace version There were no users of those functions that would need a new copy. --- .../plugins/cache_req_group_by_filter.c | 8 +-- .../plugins/cache_req_group_by_name.c | 8 +-- .../plugins/cache_req_initgroups_by_name.c | 8 +-- .../plugins/cache_req_object_by_name.c | 8 +-- .../plugins/cache_req_subid_ranges_by_name.c | 8 +-- .../plugins/cache_req_user_by_filter.c | 8 +-- .../plugins/cache_req_user_by_name.c | 8 +-- src/tests/cmocka/test_string_utils.c | 57 ++++--------------- src/util/string_utils.c | 38 +++++++------ src/util/usertools.c | 6 +- src/util/util.h | 10 ++-- 11 files changed, 49 insertions(+), 118 deletions(-) diff --git a/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c b/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c index 1233d248a8..d92468bf60 100644 --- a/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c +++ b/src/responder/common/cache_req/plugins/cache_req_group_by_filter.c @@ -32,7 +32,7 @@ cache_req_group_by_filter_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -52,11 +52,7 @@ cache_req_group_by_filter_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); talloc_zfree(data->name.lookup); data->name.lookup = talloc_steal(data, name); diff --git a/src/responder/common/cache_req/plugins/cache_req_group_by_name.c b/src/responder/common/cache_req/plugins/cache_req_group_by_name.c index 3be0d5ea55..00202f35f3 100644 --- a/src/responder/common/cache_req/plugins/cache_req_group_by_name.c +++ b/src/responder/common/cache_req/plugins/cache_req_group_by_name.c @@ -32,7 +32,7 @@ cache_req_group_by_name_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -52,11 +52,7 @@ cache_req_group_by_name_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); name = sss_create_internal_fqname(tmp_ctx, name, domain->name); if (name == NULL) { diff --git a/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c b/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c index c5bea9d849..56d52aaf1c 100644 --- a/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c +++ b/src/responder/common/cache_req/plugins/cache_req_initgroups_by_name.c @@ -32,7 +32,7 @@ cache_req_initgroups_by_name_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -52,11 +52,7 @@ cache_req_initgroups_by_name_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); name = sss_create_internal_fqname(tmp_ctx, name, domain->name); if (name == NULL) { diff --git a/src/responder/common/cache_req/plugins/cache_req_object_by_name.c b/src/responder/common/cache_req/plugins/cache_req_object_by_name.c index 907f1f3d50..46002da937 100644 --- a/src/responder/common/cache_req/plugins/cache_req_object_by_name.c +++ b/src/responder/common/cache_req/plugins/cache_req_object_by_name.c @@ -76,7 +76,7 @@ cache_req_object_by_name_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -96,11 +96,7 @@ cache_req_object_by_name_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); name = sss_create_internal_fqname(tmp_ctx, name, domain->name); if (name == NULL) { diff --git a/src/responder/common/cache_req/plugins/cache_req_subid_ranges_by_name.c b/src/responder/common/cache_req/plugins/cache_req_subid_ranges_by_name.c index 54852711f9..fe31cf2654 100644 --- a/src/responder/common/cache_req/plugins/cache_req_subid_ranges_by_name.c +++ b/src/responder/common/cache_req/plugins/cache_req_subid_ranges_by_name.c @@ -30,7 +30,7 @@ cache_req_subid_ranges_by_name_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -50,11 +50,7 @@ cache_req_subid_ranges_by_name_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); talloc_zfree(data->name.lookup); data->name.lookup = talloc_steal(data, name); diff --git a/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c b/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c index 33ad7c00f3..dd6a33fb22 100644 --- a/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c +++ b/src/responder/common/cache_req/plugins/cache_req_user_by_filter.c @@ -32,7 +32,7 @@ cache_req_user_by_filter_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -52,11 +52,7 @@ cache_req_user_by_filter_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); talloc_zfree(data->name.lookup); data->name.lookup = talloc_steal(data, name); diff --git a/src/responder/common/cache_req/plugins/cache_req_user_by_name.c b/src/responder/common/cache_req/plugins/cache_req_user_by_name.c index d24a2221b2..20f215c9c8 100644 --- a/src/responder/common/cache_req/plugins/cache_req_user_by_name.c +++ b/src/responder/common/cache_req/plugins/cache_req_user_by_name.c @@ -32,7 +32,7 @@ cache_req_user_by_name_prepare_domain_data(struct cache_req *cr, struct sss_domain_info *domain) { TALLOC_CTX *tmp_ctx; - const char *name; + char *name; errno_t ret; if (cr->data->name.name == NULL) { @@ -52,11 +52,7 @@ cache_req_user_by_name_prepare_domain_data(struct cache_req *cr, goto done; } - name = sss_reverse_replace_space(tmp_ctx, name, cr->rctx->override_space); - if (name == NULL) { - ret = ENOMEM; - goto done; - } + sss_reverse_replace_space_inplace(name, cr->rctx->override_space); name = sss_create_internal_fqname(tmp_ctx, name, domain->name); if (name == NULL) { diff --git a/src/tests/cmocka/test_string_utils.c b/src/tests/cmocka/test_string_utils.c index f11fc21a6a..09a61f2d93 100644 --- a/src/tests/cmocka/test_string_utils.c +++ b/src/tests/cmocka/test_string_utils.c @@ -23,10 +23,8 @@ void test_replace_whitespaces(void **state) { - TALLOC_CTX *mem_ctx; - const char *input_str = "Lorem ipsum dolor sit amet"; - const char *res; size_t i; + char *input; struct { const char *input; @@ -51,36 +49,18 @@ void test_replace_whitespaces(void **state) { NULL, NULL, '\0' }, }; - mem_ctx = talloc_new(NULL); - assert_non_null(mem_ctx); - check_leaks_push(mem_ctx); - - res = sss_replace_space(mem_ctx, input_str, '\0'); - assert_string_equal(res, input_str); - talloc_zfree(res); - - res = sss_replace_space(mem_ctx, input_str, '\0'); - assert_string_equal(res, input_str); - talloc_zfree(res); - for (i=0; data_set[i].input != NULL; ++i) { - res = sss_replace_space(mem_ctx, data_set[i].input, - data_set[i].replace_char); - assert_non_null(res); - assert_string_equal(res, data_set[i].output); - talloc_zfree(res); + input = strdup(data_set[i].input); + sss_replace_space_inplace(input, data_set[i].replace_char); + assert_string_equal(input, data_set[i].output); + free(input); } - - assert_true(check_leaks_pop(mem_ctx) == true); - talloc_free(mem_ctx); } void test_reverse_replace_whitespaces(void **state) { - TALLOC_CTX *mem_ctx; - char *input_str = discard_const_p(char, "Lorem ipsum dolor sit amet"); - char *res; size_t i; + char *input; struct { const char *input; @@ -109,29 +89,12 @@ void test_reverse_replace_whitespaces(void **state) { NULL, NULL, '\0' }, }; - mem_ctx = talloc_new(NULL); - assert_non_null(mem_ctx); - check_leaks_push(mem_ctx); - - res = sss_reverse_replace_space(mem_ctx, input_str, '\0'); - assert_string_equal(res, input_str); - talloc_free(res); - - res = sss_reverse_replace_space(mem_ctx, input_str, '\0'); - assert_string_equal(res, input_str); - talloc_free(res); - for (i=0; data_set[i].input != NULL; ++i) { - input_str = discard_const_p(char, data_set[i].input); - res = sss_reverse_replace_space(mem_ctx, input_str, - data_set[i].replace_char); - assert_non_null(res); - assert_string_equal(res, data_set[i].output); - talloc_zfree(res); + input = strdup(data_set[i].input); + sss_reverse_replace_space_inplace(input, data_set[i].replace_char); + assert_string_equal(input, data_set[i].output); + free(input); } - - assert_true(check_leaks_pop(mem_ctx) == true); - talloc_free(mem_ctx); } void test_guid_blob_to_string_buf(void **state) diff --git a/src/util/string_utils.c b/src/util/string_utils.c index 78966e3093..fa3bffdb76 100644 --- a/src/util/string_utils.c +++ b/src/util/string_utils.c @@ -22,12 +22,20 @@ #include "util/util.h" +static inline void replace_char_inplace(char *p, char match, char sub) +{ + for (; *p != '\0'; ++p) { + if (*p == match) { + *p = sub; + } + } +} + char *sss_replace_char(TALLOC_CTX *mem_ctx, const char *in, const char match, const char sub) { - char *p; char *out; out = talloc_strdup(mem_ctx, in); @@ -35,21 +43,16 @@ char *sss_replace_char(TALLOC_CTX *mem_ctx, return NULL; } - for (p = out; *p != '\0'; ++p) { - if (*p == match) { - *p = sub; - } - } + replace_char_inplace(out, match, sub); return out; } -char * sss_replace_space(TALLOC_CTX *mem_ctx, - const char *orig_name, - const char subst) +void sss_replace_space_inplace(char *orig_name, + const char subst) { if (subst == '\0' || subst == ' ') { - return talloc_strdup(mem_ctx, orig_name); + return; } if (strchr(orig_name, subst) != NULL) { @@ -60,28 +63,27 @@ char * sss_replace_space(TALLOC_CTX *mem_ctx, "Name [%s] already contains replacement character [%c]. " \ "No replacement will be done.\n", orig_name, subst); - return talloc_strdup(mem_ctx, orig_name); + return; } - return sss_replace_char(mem_ctx, orig_name, ' ', subst); + replace_char_inplace(orig_name, ' ', subst); } -char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx, - const char *orig_name, - const char subst) +void sss_reverse_replace_space_inplace(char *orig_name, + const char subst) { if (subst == '\0' || subst == ' ') { - return talloc_strdup(mem_ctx, orig_name); + return; } if (strchr(orig_name, subst) != NULL && strchr(orig_name, ' ') != NULL) { DEBUG(SSSDBG_TRACE_FUNC, "Input [%s] contains replacement character [%c] and space.\n", orig_name, subst); - return talloc_strdup(mem_ctx, orig_name); + return; } - return sss_replace_char(mem_ctx, orig_name, subst, ' '); + replace_char_inplace(orig_name, subst, ' '); } errno_t guid_blob_to_string_buf(const uint8_t *blob, char *str_buf, diff --git a/src/util/usertools.c b/src/util/usertools.c index bce59a7eaf..a52194accd 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -734,11 +734,7 @@ char *sss_output_name(TALLOC_CTX *mem_ctx, outname = shortname; } - outname = sss_replace_space(tmp_ctx, outname, replace_space); - if (outname == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "sss_replace_space failed\n"); - goto done; - } + sss_replace_space_inplace(outname, replace_space); outname = talloc_steal(mem_ctx, outname); done: diff --git a/src/util/util.h b/src/util/util.h index cb8ab6b62d..d8de5673d1 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -712,12 +712,10 @@ char *sss_replace_char(TALLOC_CTX *mem_ctx, const char match, const char sub); -char * sss_replace_space(TALLOC_CTX *mem_ctx, - const char *orig_name, - const char replace_char); -char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx, - const char *orig_name, - const char replace_char); +void sss_replace_space_inplace(char *orig_name, + const char replace_char); +void sss_reverse_replace_space_inplace(char *orig_name, + const char replace_char); #define GUID_BIN_LENGTH 16 /* 16 2-digit hex values + 4 dashes + terminating 0 */ From a67cba7f85452a9a547611b0f88c26229ede6ff1 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Tue, 11 Mar 2025 12:26:07 +0100 Subject: [PATCH 15/16] UTIL: delete sss_fqname() Function is unused since 26c722d568b0061e0f1edb8d07093bf051d76083 --- src/tests/cmocka/test_fqnames.c | 24 ------------------------ src/util/usertools.c | 10 ---------- 2 files changed, 34 deletions(-) diff --git a/src/tests/cmocka/test_fqnames.c b/src/tests/cmocka/test_fqnames.c index be6a27ae81..6a9bed7519 100644 --- a/src/tests/cmocka/test_fqnames.c +++ b/src/tests/cmocka/test_fqnames.c @@ -110,8 +110,6 @@ void test_default(void **state) errno_t ret; char *fqdn; - const int fqdn_size = 255; - char fqdn_s[fqdn_size]; if (test_ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Type mismatch\n"); @@ -128,10 +126,6 @@ void test_default(void **state) assert_string_equal(fqdn, NAME"@"DOMNAME); talloc_free(fqdn); - ret = sss_fqname(fqdn_s, fqdn_size, test_ctx->nctx, test_ctx->dom, NAME); - assert_int_equal(ret + 1, sizeof(NAME"@"DOMNAME)); - assert_string_equal(fqdn_s, NAME"@"DOMNAME); - talloc_free(test_ctx->nctx); } @@ -142,8 +136,6 @@ void test_all(void **state) errno_t ret; char *fqdn; - const int fqdn_size = 255; - char fqdn_s[fqdn_size]; if (test_ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Type mismatch\n"); @@ -160,10 +152,6 @@ void test_all(void **state) assert_string_equal(fqdn, NAME"@"DOMNAME"@"FLATNAME); talloc_free(fqdn); - ret = sss_fqname(fqdn_s, fqdn_size, test_ctx->nctx, test_ctx->dom, NAME); - assert_int_equal(ret + 1, sizeof(NAME"@"DOMNAME"@"FLATNAME)); - assert_string_equal(fqdn_s, NAME"@"DOMNAME"@"FLATNAME); - talloc_free(test_ctx->nctx); } @@ -174,8 +162,6 @@ void test_flat(void **state) errno_t ret; char *fqdn; - const int fqdn_size = 255; - char fqdn_s[fqdn_size]; if (test_ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Type mismatch\n"); @@ -192,10 +178,6 @@ void test_flat(void **state) assert_string_equal(fqdn, NAME"@"FLATNAME); talloc_free(fqdn); - ret = sss_fqname(fqdn_s, fqdn_size, test_ctx->nctx, test_ctx->dom, NAME); - assert_int_equal(ret + 1, sizeof(NAME"@"FLATNAME)); - assert_string_equal(fqdn_s, NAME"@"FLATNAME); - talloc_free(test_ctx->nctx); } @@ -206,8 +188,6 @@ void test_flat_fallback(void **state) errno_t ret; char *fqdn; - const int fqdn_size = 255; - char fqdn_s[fqdn_size]; if (test_ctx == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Type mismatch\n"); @@ -229,10 +209,6 @@ void test_flat_fallback(void **state) assert_string_equal(fqdn, NAME"@"DOMNAME); talloc_free(fqdn); - ret = sss_fqname(fqdn_s, fqdn_size, test_ctx->nctx, test_ctx->dom, NAME); - assert_int_equal(ret + 1, sizeof(NAME"@"DOMNAME)); - assert_string_equal(fqdn_s, NAME"@"DOMNAME); - talloc_free(test_ctx->nctx); } diff --git a/src/util/usertools.c b/src/util/usertools.c index a52194accd..57f1dd669a 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -552,16 +552,6 @@ sss_tc_fqname2(TALLOC_CTX *mem_ctx, struct sss_names_ctx *nctx, return output; } -int -sss_fqname(char *str, size_t size, struct sss_names_ctx *nctx, - struct sss_domain_info *domain, const char *name) -{ - if (domain == NULL || nctx == NULL) return -EINVAL; - - return safe_format_string(str, size, nctx->fq_fmt, - name, domain->name, calc_flat_name (domain), NULL); -} - errno_t sss_user_by_name_or_uid(const char *input, uid_t *_uid, gid_t *_gid) { uid_t uid; From 3fbeeee236bd1ddacdc81fa8270fc763d28b42bf Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Tue, 11 Mar 2025 13:17:37 +0100 Subject: [PATCH 16/16] UTIL: sss_tc_fqname2() optimization Scan format and alloc string once instead of talloc_strndup_append() for every chunk. --- src/util/usertools.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/src/util/usertools.c b/src/util/usertools.c index 57f1dd669a..bfcaeecf29 100644 --- a/src/util/usertools.c +++ b/src/util/usertools.c @@ -518,37 +518,46 @@ char * sss_tc_fqname(TALLOC_CTX *mem_ctx, struct sss_names_ctx *nctx, struct sss_domain_info *domain, const char *name) { - if (domain == NULL || nctx == NULL) return NULL; + if (domain == NULL || nctx == NULL) { + errno = EINVAL; + return NULL; + } return sss_tc_fqname2 (mem_ctx, nctx, domain->name, calc_flat_name (domain), name); } -static void -safe_talloc_callback (void *data, - const char *piece, - size_t len) -{ - char **output = data; - if (*output != NULL) - *output = talloc_strndup_append(*output, piece, len); -} - char * sss_tc_fqname2(TALLOC_CTX *mem_ctx, struct sss_names_ctx *nctx, const char *domain_name, const char *flat_dom_name, const char *name) { - const char *args[] = { name, domain_name, flat_dom_name, NULL }; char *output; + int fqdn_len, len; - if (nctx == NULL) return NULL; + if (nctx == NULL) { + errno = EINVAL; + return NULL; + } - output = talloc_strdup(mem_ctx, ""); - if (safe_format_string_cb(safe_talloc_callback, &output, nctx->fq_fmt, args, 3) < 0) - output = NULL; - else if (output == NULL) + fqdn_len = safe_format_string(NULL, 0, nctx->fq_fmt, + name, domain_name, flat_dom_name, NULL); + if (fqdn_len <= 0) { + errno = EINVAL; + return NULL; + } + output = talloc_size(mem_ctx, fqdn_len + 1); + if (output == NULL) { errno = ENOMEM; + return NULL; + } + len = safe_format_string(output, fqdn_len + 1, nctx->fq_fmt, + name, domain_name, flat_dom_name, NULL); + if (len != fqdn_len) { + talloc_free(output); + errno = EINVAL; + return NULL; + } return output; }