URL: https://github.com/SSSD/sssd/pull/5541 Author: sumit-bose Title: #5541: nss client: make innetgr() thread safe Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5541/head:pr5541 git checkout pr5541
From 86f06e4c238faec7d22a7833d99ff73df0d623b3 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 5 Mar 2021 13:57:11 +0100 Subject: [PATCH 1/2] nss client: make innetgr() thread safe The innetgr() call is expected to be thread safe but SSSD's the current implementation isn't. In glibc innetgr() is implementend by calling the setnetgrent(), getnetgrent(), endgrent() sequence with a private context (struct __netgrent) with provides a member where NSS modules can store data between the calls. With this patch setnetgrent() will read all required data from the NSS responder and store it in the data member of the __netgrent struct. Upcoming getnetgrent() calls will only operate on the stored data and not connect to the NSS responder anymore. endgrent() will free the data. Since the netgroup data is read in a single request to the NSS responder protected by a mutex and stored in private context of innetgr() this call is now thread-safe. Resolves: https://github.com/SSSD/sssd/issues/5540 --- src/responder/nss/nss_cmd.c | 106 +++---------------------- src/responder/nss/nss_protocol.h | 6 -- src/responder/nss/nss_protocol_netgr.c | 31 +------- src/sss_client/nss_netgroup.c | 67 +++------------- 4 files changed, 26 insertions(+), 184 deletions(-) diff --git a/src/responder/nss/nss_cmd.c b/src/responder/nss/nss_cmd.c index 709ef9c05f..4698e751c9 100644 --- a/src/responder/nss/nss_cmd.c +++ b/src/responder/nss/nss_cmd.c @@ -826,6 +826,13 @@ static errno_t sss_nss_setnetgrent(struct cli_ctx *cli_ctx, goto done; } + /* enum_limit is not used for setnetgrent, all results will be returned at + * once to allow innetgr() to be implemented in the thread-safe way. */ + cmd_ctx->enum_limit = 0; + cmd_ctx->enumeration = true; + cmd_ctx->enum_ctx = NULL; /* We will determine it later. */ + cmd_ctx->enum_index = &cmd_ctx->state_ctx->netgrent; + subreq = nss_setnetgrent_send(cli_ctx, cli_ctx->ev, cli_ctx, type, nss_ctx->netgrent, state_ctx->netgroup); if (subreq == NULL) { @@ -848,80 +855,6 @@ static errno_t sss_nss_setnetgrent(struct cli_ctx *cli_ctx, } static void sss_nss_setnetgrent_done(struct tevent_req *subreq) -{ - struct nss_cmd_ctx *cmd_ctx; - errno_t ret; - - cmd_ctx = tevent_req_callback_data(subreq, struct nss_cmd_ctx); - - ret = nss_setnetgrent_recv(subreq); - talloc_zfree(subreq); - if (ret != EOK) { - nss_protocol_done(cmd_ctx->cli_ctx, ret); - goto done; - } - - nss_protocol_reply(cmd_ctx->cli_ctx, cmd_ctx->nss_ctx, cmd_ctx, - NULL, cmd_ctx->fill_fn); - -done: - talloc_free(cmd_ctx); -} - -static void nss_getnetgrent_done(struct tevent_req *subreq); - -static errno_t nss_getnetgrent(struct cli_ctx *cli_ctx, - enum cache_req_type type, - nss_protocol_fill_packet_fn fill_fn) -{ - struct nss_cmd_ctx *cmd_ctx; - struct tevent_req *subreq; - errno_t ret; - - cmd_ctx = nss_cmd_ctx_create(cli_ctx, cli_ctx, type, fill_fn); - if (cmd_ctx == NULL) { - ret = ENOMEM; - goto done; - } - - if (cmd_ctx->state_ctx->netgroup == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "State does not contain netgroup name!\n"); - ret = EINVAL; - goto done; - } - - ret = nss_protocol_parse_limit(cli_ctx, &cmd_ctx->enum_limit); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "Invalid request message!\n"); - goto done; - } - - cmd_ctx->enumeration = true; - cmd_ctx->enum_ctx = NULL; /* We will determine it later. */ - cmd_ctx->enum_index = &cmd_ctx->state_ctx->netgrent; - - subreq = nss_setnetgrent_send(cli_ctx, cli_ctx->ev, cli_ctx, type, - cmd_ctx->nss_ctx->netgrent, - cmd_ctx->state_ctx->netgroup); - if (subreq == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "nss_setnetgrent_send() failed\n"); - return ENOMEM; - } - - tevent_req_set_callback(subreq, nss_getnetgrent_done, cmd_ctx); - - ret = EOK; - -done: - if (ret != EOK) { - talloc_free(cmd_ctx); - return nss_protocol_done(cli_ctx, ret); - } - - return EOK; -} - -static void nss_getnetgrent_done(struct tevent_req *subreq) { struct nss_enum_ctx *enum_ctx; struct nss_cmd_ctx *cmd_ctx; @@ -956,6 +889,9 @@ static void nss_getnetgrent_done(struct tevent_req *subreq) nss_protocol_done(cmd_ctx->cli_ctx, ret); } + cmd_ctx->state_ctx->netgrent.domain = 0; + cmd_ctx->state_ctx->netgrent.result = 0; + talloc_free(cmd_ctx); } @@ -1114,23 +1050,7 @@ static errno_t nss_cmd_setnetgrent(struct cli_ctx *cli_ctx) state_ctx->netgrent.result = 0; return sss_nss_setnetgrent(cli_ctx, CACHE_REQ_NETGROUP_BY_NAME, - nss_protocol_fill_setnetgrent); -} - -static errno_t nss_cmd_getnetgrent(struct cli_ctx *cli_ctx) -{ - return nss_getnetgrent(cli_ctx, CACHE_REQ_NETGROUP_BY_NAME, - nss_protocol_fill_netgrent); -} - -static errno_t nss_cmd_endnetgrent(struct cli_ctx *cli_ctx) -{ - struct nss_state_ctx *state_ctx; - - state_ctx = talloc_get_type(cli_ctx->state_ctx, struct nss_state_ctx); - talloc_zfree(state_ctx->netgroup); - - return nss_endent(cli_ctx, &state_ctx->netgrent); + nss_protocol_fill_netgrent); } static errno_t nss_cmd_getservbyname(struct cli_ctx *cli_ctx) @@ -1410,8 +1330,8 @@ struct sss_cmd_table *get_nss_cmds(void) { SSS_NSS_ENDGRENT, nss_cmd_endgrent }, { SSS_NSS_INITGR, nss_cmd_initgroups }, { SSS_NSS_SETNETGRENT, nss_cmd_setnetgrent }, - { SSS_NSS_GETNETGRENT, nss_cmd_getnetgrent }, - { SSS_NSS_ENDNETGRENT, nss_cmd_endnetgrent }, + /* { SSS_NSS_GETNETGRENT, "not needed" }, */ + /* { SSS_NSS_ENDNETGRENT, "not needed" }, */ { SSS_NSS_GETSERVBYNAME, nss_cmd_getservbyname }, { SSS_NSS_GETSERVBYPORT, nss_cmd_getservbyport }, { SSS_NSS_SETSERVENT, nss_cmd_setservent }, diff --git a/src/responder/nss/nss_protocol.h b/src/responder/nss/nss_protocol.h index 357017e768..364f19c835 100644 --- a/src/responder/nss/nss_protocol.h +++ b/src/responder/nss/nss_protocol.h @@ -153,12 +153,6 @@ nss_protocol_fill_netgrent(struct nss_ctx *nss_ctx, struct sss_packet *packet, struct cache_req_result *result); -errno_t -nss_protocol_fill_setnetgrent(struct nss_ctx *nss_ctx, - struct nss_cmd_ctx *cmd_ctx, - struct sss_packet *packet, - struct cache_req_result *result); - errno_t nss_protocol_fill_svcent(struct nss_ctx *nss_ctx, struct nss_cmd_ctx *cmd_ctx, diff --git a/src/responder/nss/nss_protocol_netgr.c b/src/responder/nss/nss_protocol_netgr.c index 274d430073..979b7484ca 100644 --- a/src/responder/nss/nss_protocol_netgr.c +++ b/src/responder/nss/nss_protocol_netgr.c @@ -115,7 +115,6 @@ nss_protocol_fill_netgrent(struct nss_ctx *nss_ctx, size_t body_len; uint8_t *body; errno_t ret; - unsigned int start; idx = cmd_ctx->enum_index; entries = cmd_ctx->enum_ctx->netgroup; @@ -141,13 +140,8 @@ nss_protocol_fill_netgrent(struct nss_ctx *nss_ctx, goto done; } - num_results = 0; - start = idx->result; + num_results = 1; /* group was found */ for (; entries[idx->result] != NULL; idx->result++) { - if ((idx->result - start) >= cmd_ctx->enum_limit) { - /* We have reached result limit. */ - break; - } entry = entries[idx->result]; @@ -185,26 +179,3 @@ nss_protocol_fill_netgrent(struct nss_ctx *nss_ctx, return EOK; } - -errno_t -nss_protocol_fill_setnetgrent(struct nss_ctx *nss_ctx, - struct nss_cmd_ctx *cmd_ctx, - struct sss_packet *packet, - struct cache_req_result *result) -{ - size_t body_len; - uint8_t *body; - errno_t ret; - - /* Two fields (length and reserved). */ - ret = sss_packet_grow(packet, 2 * sizeof(uint32_t)); - if (ret != EOK) { - return ret; - } - - sss_packet_get_body(packet, &body, &body_len); - SAFEALIGN_SET_UINT32(body, 1, NULL); /* Netgroup was found. */ - SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /* reserved */ - - return EOK; -} diff --git a/src/sss_client/nss_netgroup.c b/src/sss_client/nss_netgroup.c index 2fc88f8ae4..dab74046bb 100644 --- a/src/sss_client/nss_netgroup.c +++ b/src/sss_client/nss_netgroup.c @@ -209,7 +209,11 @@ enum nss_status _nss_sss_setnetgrent(const char *netgroup, goto out; } - free(repbuf); + result->data = (char *) repbuf; + result->data_size = replen; + /* skip metadata fields */ + result->idx.position = NETGR_METADATA_COUNT; + nret = NSS_STATUS_SUCCESS; out: @@ -221,19 +225,15 @@ static enum nss_status internal_getnetgrent_r(struct __netgrent *result, char *buffer, size_t buflen, int *errnop) { - struct sss_cli_req_data rd; struct sss_nss_netgr_rep netgrrep; uint8_t *repbuf; size_t replen; - uint32_t num_results; - enum nss_status nret; - uint32_t num_entries; int ret; /* Caught once glibc passing in buffer == 0x0 */ if (!buffer || !buflen) { - *errnop = ERANGE; - return NSS_STATUS_TRYAGAIN; + *errnop = ERANGE; + return NSS_STATUS_TRYAGAIN; } /* If we're already processing result data, continue to @@ -260,36 +260,7 @@ static enum nss_status internal_getnetgrent_r(struct __netgrent *result, return NSS_STATUS_SUCCESS; } - /* Release memory, if any */ - CLEAR_NETGRENT_DATA(result); - - /* retrieve no more than SSS_NSS_MAX_ENTRIES at a time */ - num_entries = SSS_NSS_MAX_ENTRIES; - rd.len = sizeof(uint32_t); - rd.data = &num_entries; - - nret = sss_nss_make_request(SSS_NSS_GETNETGRENT, &rd, - &repbuf, &replen, errnop); - if (nret != NSS_STATUS_SUCCESS) { - return nret; - } - - /* Get number of results from repbuf. */ - SAFEALIGN_COPY_UINT32(&num_results, repbuf, NULL); - - /* no results if not found */ - if ((num_results == 0) || (replen <= NETGR_METADATA_COUNT)) { - free(repbuf); - return NSS_STATUS_RETURN; - } - - result->data = (char *) repbuf; - result->data_size = replen; - /* skip metadata fields */ - result->idx.position = NETGR_METADATA_COUNT; - - /* call again ourselves, this will return the first result */ - return internal_getnetgrent_r(result, buffer, buflen, errnop); + return NSS_STATUS_RETURN; } enum nss_status _nss_sss_getnetgrent_r(struct __netgrent *result, @@ -298,32 +269,18 @@ enum nss_status _nss_sss_getnetgrent_r(struct __netgrent *result, { enum nss_status nret; - sss_nss_lock(); + /* no lock needed because results are already stored in result */ nret = internal_getnetgrent_r(result, buffer, buflen, errnop); - sss_nss_unlock(); return nret; } enum nss_status _nss_sss_endnetgrent(struct __netgrent *result) { - enum nss_status nret; - int errnop; - int saved_errno = errno; - - sss_nss_lock(); - + /* no lock needed because resources in the responder are already + * released */ /* make sure we do not have leftovers, and release memory */ CLEAR_NETGRENT_DATA(result); - nret = sss_nss_make_request(SSS_NSS_ENDNETGRENT, - NULL, NULL, NULL, &errnop); - if (nret != NSS_STATUS_SUCCESS) { - errno = errnop; - } else { - errno = saved_errno; - } - - sss_nss_unlock(); - return nret; + return NSS_STATUS_SUCCESS; } From be388cc832d1a5d19dc3e19ad336c633c735aef3 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Tue, 16 Mar 2021 18:27:57 +0100 Subject: [PATCH 2/2] intg test: test is innetgr() is thread-safe This integration test adds 2 large netgroups in LDAP and runs a program with 2 threads looking up those netgroups in parallel. Resolves: https://github.com/SSSD/sssd/issues/5540 --- src/tests/intg/Makefile.am | 14 +++- src/tests/intg/sss_netgroup_thread_test.c | 81 +++++++++++++++++++++++ src/tests/intg/test_netgroup.py | 26 ++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 src/tests/intg/sss_netgroup_thread_test.c diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am index f717214ad3..133f07423a 100644 --- a/src/tests/intg/Makefile.am +++ b/src/tests/intg/Makefile.am @@ -62,6 +62,18 @@ getsockopt_wrapper_la_LDFLAGS = \ -avoid-version \ -module +bin_PROGRAMS = sss_netgroup_thread_test + +sss_netgroup_thread_test_SOURCES = \ + sss_netgroup_thread_test.c \ + $(NULL) +sss_netgroup_thread_test_CFLAGS = \ + $(AM_CFLAGS) \ + $(NULL) +sss_netgroup_thread_test_LDADD = \ + -lpthread \ + $(NULL) + dist_dbussysconf_DATA = cwrap-dbus-system.conf install-data-hook: @@ -162,7 +174,7 @@ clean-local: PAM_CERT_DB_PATH="$(abs_builddir)/../test_CA/SSSD_test_CA.pem" SOFTHSM2_CONF="$(abs_builddir)/../test_CA/softhsm2_one.conf" -intgcheck-installed: config.py passwd group pam_sss_service pam_sss_alt_service pam_sss_sc_required pam_sss_try_sc pam_sss_allow_missing_name pam_sss_domains +intgcheck-installed: config.py passwd group pam_sss_service pam_sss_alt_service pam_sss_sc_required pam_sss_try_sc pam_sss_allow_missing_name pam_sss_domains sss_netgroup_thread_test pipepath="$(DESTDIR)$(pipepath)"; \ if test $${#pipepath} -gt 80; then \ echo "error: Pipe directory path too long," \ diff --git a/src/tests/intg/sss_netgroup_thread_test.c b/src/tests/intg/sss_netgroup_thread_test.c new file mode 100644 index 0000000000..d3bee9b3e6 --- /dev/null +++ b/src/tests/intg/sss_netgroup_thread_test.c @@ -0,0 +1,81 @@ +/* + Helper program to test if innetgr() is thread-safe + + Authors: + Sumit Bose <sb...@redhat.com> + + Copyright (c) 2021 Red Hat, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <stdio.h> +#include <stdbool.h> +#include <pthread.h> +#include <pwd.h> +#include <grp.h> +#include <netdb.h> +#include <unistd.h> +#include <sys/types.h> + + + +struct data { + const char *group; + const char *host; + const char *user; + const char *domain; + bool *failed; +}; + +static void *full_netgroup(void *arg) +{ + int ret; + size_t c = 0; + struct data *data = arg; + + do { + ret = innetgr(data->group, data->host, data->user, data->domain); + if (ret != 1) { + *(data->failed) = true; + } + c++; + } while (!*(data->failed) && c<100000); + + pthread_exit(NULL); +} + +int main() +{ + pthread_t thread[2]; + bool failed[2] = {false, false}; + + struct data data[3] = {{"ng1", "host1", "user924", "domain1", &failed[0]}, + {"ng2", "host2", "user925", "domain2", &failed[1]}, + {NULL, NULL, NULL, NULL, NULL}}; + + + pthread_create(&thread[0], NULL, full_netgroup, &data[0]); + pthread_create(&thread[1], NULL, full_netgroup, &data[1]); + + pthread_join(thread[1], NULL); + pthread_join(thread[0], NULL); + + if (failed[0] || failed[1]) { + printf ("Test failed.\n"); + return 1; + } + + return 0; +} diff --git a/src/tests/intg/test_netgroup.py b/src/tests/intg/test_netgroup.py index 6a33423cda..250b07e88a 100644 --- a/src/tests/intg/test_netgroup.py +++ b/src/tests/intg/test_netgroup.py @@ -510,3 +510,29 @@ def test_offline_netgroups(add_tripled_netgroup): res, _, netgrps = get_sssd_netgroups("tripled_netgroup") assert res == NssReturnCode.SUCCESS assert netgrps == [("host", "user", "domain")] + + +@pytest.fixture +def add_thread_test_netgroup(request, ldap_conn): + ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) + + triple_list = [] + for i in range(1, 999): + triple_list.append("(host1,user" + str(i) + ",domain1)") + ent_list.add_netgroup("ng1", triple_list) + + triple_list = [] + for i in range(1, 999): + triple_list.append("(host2,user" + str(i) + ",domain2)") + ent_list.add_netgroup("ng2", triple_list) + + create_ldap_fixture(request, ldap_conn, ent_list) + conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS) + create_conf_fixture(request, conf) + create_sssd_fixture(request) + return None + + +def test_innetgr_with_threads(add_thread_test_netgroup): + + subprocess.check_call(["sss_netgroup_thread_test"])
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
