The attached two patches fix https://fedorahosted.org/sssd/ticket/1189 that was mostly manifested by ""BUG: a callback did not free its request." during enumeration (and reading of the autofs map).
[PATCH 1/2] Remove setent structure when callback is called The setent_remove_ref() destructor was only changing the ->head variable local to the entry being destroyed. This doesn't work properly when the entry is the head, the head needs to be set to NULL. The patch also changes two unrelated places where we were using the low-level setent_notify_done() instead of the nss-specific nss_setent_notify_done() and removes one unused function. [PATCH 2/2] Allocate setent structure on state, not on the client context Inside the _send() functions that use the setent contexts, we should allocate the setent structure on state, not on the client context. Mostly because the callbacks attached to tevent_req should also free the setent structures after they receive data. The services code already does this correctly. There's also one unrelated fix for a wrong string format variable.
From 3a22917b0b8484b578b704110c88d5d737141135 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Thu, 9 Feb 2012 20:00:22 +0100 Subject: [PATCH 1/2] Remove setent structure when callback is called --- src/responder/autofs/autofssrv_cmd.c | 2 +- src/responder/common/responder.h | 4 ++-- src/responder/common/responder_cmd.c | 18 +++++++++--------- src/responder/nss/nsssrv_cmd.c | 13 ++++--------- src/responder/nss/nsssrv_private.h | 1 - 5 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c index d2b37f71949291538b4a449755dab3645cf1d71b..73dbc5eaa046451b3af814e413b40055ec08e6e0 100644 --- a/src/responder/autofs/autofssrv_cmd.c +++ b/src/responder/autofs/autofssrv_cmd.c @@ -87,7 +87,7 @@ autofs_setent_add_ref(TALLOC_CTX *memctx, static void autofs_setent_notify(struct autofs_map_ctx *map_ctx, errno_t ret) { - setent_notify(map_ctx->reqs, ret); + setent_notify(&map_ctx->reqs, ret); } static errno_t diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h index 7ec2ee3dc089687302e4fa56072d37469bef6413..48678ff92ae6be42dacfef8f9ec8647b4f06eec0 100644 --- a/src/responder/common/responder.h +++ b/src/responder/common/responder.h @@ -168,8 +168,8 @@ errno_t setent_add_ref(TALLOC_CTX *memctx, void *pvt, struct setent_req_list **list, struct tevent_req *req); -void setent_notify(struct setent_req_list *list, errno_t err); -void setent_notify_done(struct setent_req_list *list); +void setent_notify(struct setent_req_list **list, errno_t err); +void setent_notify_done(struct setent_req_list **list); errno_t sss_cmd_check_cache(struct ldb_message *msg, diff --git a/src/responder/common/responder_cmd.c b/src/responder/common/responder_cmd.c index f36ea14aff972237b954a3e8f0b26d97c8d17877..16f38eafd6a3362b8338ced459caa88bf2a4edb7 100644 --- a/src/responder/common/responder_cmd.c +++ b/src/responder/common/responder_cmd.c @@ -160,8 +160,8 @@ int sss_cmd_execute(struct cli_ctx *cctx, struct sss_cmd_table *sss_cmds) struct setent_req_list { struct setent_req_list *prev; struct setent_req_list *next; - /* Need to point to the head of the list from a talloc destructor */ - struct setent_req_list *head; + /* Need to modify the list from a talloc destructor */ + struct setent_req_list **head; void *pvt; @@ -178,7 +178,7 @@ int setent_remove_ref(TALLOC_CTX *ctx) { struct setent_req_list *entry = talloc_get_type(ctx, struct setent_req_list); - DLIST_REMOVE(entry->head, entry); + DLIST_REMOVE(*(entry->head), entry); return 0; } @@ -197,18 +197,18 @@ errno_t setent_add_ref(TALLOC_CTX *memctx, entry->req = req; entry->pvt = pvt; DLIST_ADD_END(*list, entry, struct setent_req_list *); - entry->head = *list; + entry->head = list; talloc_set_destructor((TALLOC_CTX *)entry, setent_remove_ref); return EOK; } -void setent_notify(struct setent_req_list *list, errno_t err) +void setent_notify(struct setent_req_list **list, errno_t err) { struct setent_req_list *reql; /* Notify the waiting clients */ - while ((reql = list)) { + while ((reql = *list) != NULL) { /* Each tevent_req_done() call will free * the request, removing it from the list. */ @@ -218,7 +218,7 @@ void setent_notify(struct setent_req_list *list, errno_t err) tevent_req_error(reql->req, err); } - if (reql == list) { + if (reql == *list) { /* The consumer failed to free the * request. Log a bug and continue. */ @@ -226,12 +226,12 @@ void setent_notify(struct setent_req_list *list, errno_t err) ("BUG: a callback did not free its request. " "May leak memory\n")); /* Skip to the next since a memory leak is non-fatal */ - list = list->next; + *list = (*list)->next; } } } -void setent_notify_done(struct setent_req_list *list) +void setent_notify_done(struct setent_req_list **list) { return setent_notify(list, EOK); } diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 1c9160b7ab824654133a3d34be29bcde8a4a7a17..68f6e0bb4a527936f098f1deee0bdd20a4c4c280 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -77,11 +77,6 @@ int nss_cmd_done(struct nss_cmd_ctx *cmdctx, int ret) /*************************** * Enumeration procedures * ***************************/ -struct tevent_req *nss_setent_get_req(struct getent_ctx *getent_ctx) -{ - return setent_get_req(getent_ctx->reqs); -} - errno_t nss_setent_add_ref(TALLOC_CTX *memctx, struct getent_ctx *getent_ctx, struct tevent_req *req) @@ -91,12 +86,12 @@ errno_t nss_setent_add_ref(TALLOC_CTX *memctx, void nss_setent_notify_error(struct getent_ctx *getent_ctx, errno_t ret) { - return setent_notify(getent_ctx->reqs, ret); + return setent_notify(&getent_ctx->reqs, ret); } void nss_setent_notify_done(struct getent_ctx *getent_ctx) { - return setent_notify_done(getent_ctx->reqs); + return setent_notify_done(&getent_ctx->reqs); } struct setent_ctx { @@ -1403,7 +1398,7 @@ static errno_t nss_cmd_setpwent_step(struct setent_step_ctx *step_ctx) } /* Notify the waiting clients */ - setent_notify_done(nctx->pctx->reqs); + nss_setent_notify_done(nctx->pctx); if (step_ctx->returned_to_mainloop) { return EAGAIN; @@ -2701,7 +2696,7 @@ static errno_t nss_cmd_setgrent_step(struct setent_step_ctx *step_ctx) } /* Notify the waiting clients */ - setent_notify_done(nctx->gctx->reqs); + nss_setent_notify_done(nctx->gctx); if (step_ctx->returned_to_mainloop) { return EAGAIN; diff --git a/src/responder/nss/nsssrv_private.h b/src/responder/nss/nsssrv_private.h index c0bf2ee2c72060fcff46f835237b3c4b0ac5322c..c6526595f03309f4dfc866609d85b07d58676d78 100644 --- a/src/responder/nss/nsssrv_private.h +++ b/src/responder/nss/nsssrv_private.h @@ -104,7 +104,6 @@ int nss_cmd_done(struct nss_cmd_ctx *cmdctx, int ret); errno_t nss_setent_add_ref(TALLOC_CTX *memctx, struct getent_ctx *getent_ctx, struct tevent_req *req); -struct tevent_req *nss_setent_get_req(struct getent_ctx *getent_ctx); void nss_setent_notify_error(struct getent_ctx *getent_ctx, errno_t ret); void nss_setent_notify_done(struct getent_ctx *getent_ctx); -- 1.7.7.6
From f3d0cf921050b3cfbecad3fc8a9aacbfaf6ebc51 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Mon, 13 Feb 2012 14:54:55 +0100 Subject: [PATCH 2/2] Allocate setent structure on state, not on the client context https://fedorahosted.org/sssd/ticket/1189 --- src/responder/autofs/autofssrv_cmd.c | 4 ++-- src/responder/nss/nsssrv_cmd.c | 8 ++++---- src/responder/nss/nsssrv_netgroup.c | 4 ++-- src/responder/nss/nsssrv_services.c | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c index 73dbc5eaa046451b3af814e413b40055ec08e6e0..2be87290270b7033bd8742cc4c85a0d8074196a6 100644 --- a/src/responder/autofs/autofssrv_cmd.c +++ b/src/responder/autofs/autofssrv_cmd.c @@ -436,7 +436,7 @@ setautomntent_send(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_TRACE_LIBS, ("Map %s is being looked up, registering for notification\n", state->mapname)); - ret = autofs_setent_add_ref(cmdctx->cctx, state->map, req); + ret = autofs_setent_add_ref(state, state->map, req); if (ret != EOK) { goto fail; } @@ -460,7 +460,7 @@ setautomntent_send(TALLOC_CTX *mem_ctx, } state->map->map_table = actx->maps; - ret = autofs_setent_add_ref(cmdctx->cctx, state->map, req); + ret = autofs_setent_add_ref(state, state->map, req); if (ret != EOK) { talloc_free(state->map); goto fail; diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 68f6e0bb4a527936f098f1deee0bdd20a4c4c280..7ec0b7c03c171c671a27a33747e5a508779d1eb5 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -1210,7 +1210,7 @@ struct tevent_req *nss_cmd_setpwent_send(TALLOC_CTX *mem_ctx, * Register for notification when it's * ready. */ - ret = nss_setent_add_ref(state->client, state->nctx->pctx, req); + ret = nss_setent_add_ref(state, state->nctx->pctx, req); if (ret != EOK) { talloc_free(req); return NULL; @@ -1232,7 +1232,7 @@ struct tevent_req *nss_cmd_setpwent_send(TALLOC_CTX *mem_ctx, state->getent_ctx = nctx->pctx; /* Add a callback reference for ourselves */ - ret = nss_setent_add_ref(state->client, state->nctx->pctx, req); + ret = nss_setent_add_ref(state, state->nctx->pctx, req); if (ret) goto error; /* ok, start the searches */ @@ -2508,7 +2508,7 @@ struct tevent_req *nss_cmd_setgrent_send(TALLOC_CTX *mem_ctx, * Register for notification when it's * ready. */ - ret = nss_setent_add_ref(state->client, state->nctx->gctx, req); + ret = nss_setent_add_ref(state, state->nctx->gctx, req); if (ret != EOK) { talloc_free(req); return NULL; @@ -2530,7 +2530,7 @@ struct tevent_req *nss_cmd_setgrent_send(TALLOC_CTX *mem_ctx, state->getent_ctx = nctx->gctx; /* Add a callback reference for ourselves */ - ret = nss_setent_add_ref(state->client, state->nctx->gctx, req); + ret = nss_setent_add_ref(state, state->nctx->gctx, req); if (ret) goto error; /* ok, start the searches */ diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 9943bca60552015ba5d693d65b69b1c124b4477a..5c79342c9754cfaa35d53ae4485e0cf0225df490 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -254,7 +254,7 @@ static struct tevent_req *setnetgrent_send(TALLOC_CTX *mem_ctx, /* Result object is still being constructed * Register for notification when it's ready */ - ret = nss_setent_add_ref(cmdctx->cctx, state->netgr, req); + ret = nss_setent_add_ref(state, state->netgr, req); if (ret != EOK) { goto error; } @@ -281,7 +281,7 @@ static struct tevent_req *setnetgrent_send(TALLOC_CTX *mem_ctx, state->netgr->lookup_table = nctx->netgroups; /* Add a reference for ourselves */ - ret = nss_setent_add_ref(cmdctx->cctx, state->netgr, req); + ret = nss_setent_add_ref(state, state->netgr, req); if (ret != EOK) { talloc_free(state->netgr); goto error; diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 3ce3d090271a5a182b37892d7ae1d570c1db1df7..38754ff077b6b5f148a1f200a6c889e1c3a628f7 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c @@ -1464,7 +1464,7 @@ setservent_step_done(struct tevent_req *req) talloc_zfree(req); if (ret == ENOENT) { DEBUG(SSSDBG_TRACE_FUNC, - ("Domain [%d] returned no results\n", dctx->domain->name)); + ("Domain [%s] returned no results\n", dctx->domain->name)); } else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ("Error [%s] while retrieving info from domain [%s]. " -- 1.7.7.6
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
