From 707b20e80a5c5b86944dc55bbc652b392a4c6454 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgallagh@redhat.com>
Date: Sat, 21 Jan 2012 12:11:23 -0500
Subject: [PATCH 5/5] DP: Fix bugs in sss_dp_get_account_int
The conversion to the tevent_req style introduced numerous bugs
related to memory management of the various client requests. In
some circumstances, this could cause memory corruption and
segmentation faults in the NSS responder. This patch makes the
following changes:
1) Rename the internal lookup from subreq to sidereq, to indicate
that it is not a sub-request of the current lookup (and therefore
is not cancelled if the current request is).
2) Change the handling of the callback loops since they call
tevent_req_[done|error], which results in them being freed (and
therefore removed from the cb_list. This was the source of the
memory corruption that would occasionally result in dereferencing
an unreadable request.
3) Remove the unnecessary sss_dp_get_account_int_recv() function
and change sss_dp_get_account_done() so that it only frees the
sidereq. All of the waiting processes have already been signaled
with the final results from sss_dp_get_account_int_done()
---
src/responder/common/responder_dp.c | 110 +++++++++++-----------------
src/responder/nss/nsssrv_cmd.c | 1 +
src/responder/pam/pamsrv_cmd.c | 1 +
src/responder/sudo/sudosrv_get_sudorules.c | 1 +
4 files changed, 47 insertions(+), 66 deletions(-)
diff --git a/src/responder/common/responder_dp.c b/src/responder/common/responder_dp.c
index f51e2496a165cc2b776af776f2e9d1ea75b8e62c..9219037cc6055899e675eef846af54238d5c61e1 100644
--- a/src/responder/common/responder_dp.c
+++ b/src/responder/common/responder_dp.c
@@ -92,11 +92,28 @@ static int sss_dp_req_destructor(void *ptr)
/* If there are callbacks that haven't been invoked, return
* an error now.
*/
- DLIST_FOR_EACH(cb, sdp_req->cb_list) {
+ while((cb = sdp_req->cb_list) != NULL) {
state = tevent_req_data(cb->req, struct dp_get_account_state);
state->err_maj = DP_ERR_FATAL;
state->err_min = EIO;
+
+ /* tevent_req_done/error will free cb */
tevent_req_error(cb->req, EIO);
+
+ /* Freeing the cb removes it from the cb_list.
+ * Therefore, the cb_list should now be pointing
+ * at a new callback. If it's not, it means the
+ * callback handler didn't free cb and may leak
+ * memory. Be paranoid and protect against this
+ * situation.
+ */
+ if (cb == sdp_req->cb_list) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ ("BUG: a callback did not free its request. "
+ "May leak memory\n"));
+ /* Skip to the next since a memory leak is non-fatal */
+ sdp_req->cb_list = sdp_req->cb_list->next;
+ }
}
/* Destroy the hash entry */
@@ -225,14 +242,6 @@ sss_dp_get_account_int_send(struct resp_ctx *rctx,
static void
sss_dp_get_account_done(struct tevent_req *subreq);
-static errno_t
-sss_dp_get_account_int_recv(TALLOC_CTX *mem_ctx,
- struct tevent_req *req,
- dbus_uint16_t *err_maj,
- dbus_uint32_t *err_min,
- char **err_msg);
-
-
/* Send a request to the data provider
* Once this function is called, the communication
* with the data provider will always run to
@@ -252,7 +261,7 @@ sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
errno_t ret;
int hret;
struct tevent_req *req;
- struct tevent_req *subreq;
+ struct tevent_req *sidereq;
struct dp_get_account_state *state;
struct sss_dp_req *sdp_req;
struct sss_dp_callback *cb;
@@ -343,19 +352,19 @@ sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
*/
value.type = HASH_VALUE_PTR;
- subreq = sss_dp_get_account_int_send(rctx, state->key, dom,
+ sidereq = sss_dp_get_account_int_send(rctx, state->key, dom,
be_type, filter);
- if (!subreq) {
+ if (!sidereq) {
ret = ENOMEM;
goto error;
}
- tevent_req_set_callback(subreq, sss_dp_get_account_done, NULL);
+ tevent_req_set_callback(sidereq, sss_dp_get_account_done, NULL);
/* We should now be able to find the sdp_req in the hash table */
hret = hash_lookup(rctx->dp_request_table, state->key, &value);
if (hret != HASH_SUCCESS) {
/* Something must have gone wrong with creating the request */
- talloc_zfree(subreq);
+ talloc_zfree(sidereq);
ret = EIO;
goto error;
}
@@ -402,23 +411,10 @@ error:
}
static void
-sss_dp_get_account_done(struct tevent_req *subreq)
+sss_dp_get_account_done(struct tevent_req *sidereq)
{
- errno_t ret;
- struct tevent_req *req = tevent_req_callback_data(subreq,
- struct tevent_req);
- struct dp_get_account_state *state =
- tevent_req_data(req, struct dp_get_account_state);
-
- ret = sss_dp_get_account_int_recv(state, req,
- &state->err_maj,
- &state->err_min,
- &state->err_msg);
- if (ret != EOK) {
- tevent_req_done(req);
- } else {
- tevent_req_error(req, ret);
- }
+ /* Nothing to do here. The callbacks have already been invoked */
+ talloc_zfree(sidereq);
}
errno_t
@@ -599,7 +595,7 @@ static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr)
int ret;
struct tevent_req *req;
struct sss_dp_req *sdp_req;
- struct sss_dp_callback *cb, *prevcb = NULL;
+ struct sss_dp_callback *cb;
struct dp_get_account_int_state *state;
struct dp_get_account_state *cb_state;
@@ -630,58 +626,40 @@ static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr)
}
/* Check whether we need to issue any callbacks */
- DLIST_FOR_EACH(cb, sdp_req->cb_list) {
+ while ((cb = sdp_req->cb_list) != NULL) {
cb_state = tevent_req_data(cb->req, struct dp_get_account_state);
cb_state->err_maj = sdp_req->err_maj;
cb_state->err_min = sdp_req->err_min;
cb_state->err_msg = talloc_strdup(cb_state, sdp_req->err_msg);
/* Don't bother checking for NULL. If it fails due to ENOMEM,
- * we can't really handle it annyway.
+ * we can't really handle it anyway.
*/
+ /* tevent_req_done/error will free cb */
if (ret == EOK) {
tevent_req_done(cb->req);
} else {
tevent_req_error(cb->req, ret);
}
- /* Freeing the request removes it from the list */
- if (prevcb) talloc_free(prevcb);
- prevcb = cb;
+ /* Freeing the cb removes it from the cb_list.
+ * Therefore, the cb_list should now be pointing
+ * at a new callback. If it's not, it means the
+ * callback handler didn't free cb and may leak
+ * memory. Be paranoid and protect against this
+ * situation.
+ */
+ if (cb == sdp_req->cb_list) {
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ ("BUG: a callback did not free its request. "
+ "May leak memory\n"));
+ /* Skip to the next since a memory leak is non-fatal */
+ sdp_req->cb_list = sdp_req->cb_list->next;
+ }
}
- talloc_free(prevcb);
/* We're done with this request. Free the sdp_req
* This will clean up the hash table entry as well
*/
talloc_zfree(sdp_req);
}
-
-static errno_t
-sss_dp_get_account_int_recv(TALLOC_CTX *mem_ctx,
- struct tevent_req *req,
- dbus_uint16_t *err_maj,
- dbus_uint32_t *err_min,
- char **err_msg)
-{
- struct dp_get_account_int_state *state =
- tevent_req_data(req, struct dp_get_account_int_state);
-
- enum tevent_req_state TRROEstate;
- uint64_t TRROEerr;
-
- *err_maj = state->sdp_req->err_maj;
- *err_min = state->sdp_req->err_min;
- *err_msg = talloc_steal(mem_ctx, state->sdp_req->err_msg);
-
- if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) {
- if (TRROEstate == TEVENT_REQ_USER_ERROR) {
- *err_maj = DP_ERR_FATAL;
- *err_min = TRROEerr;
- } else {
- return EIO;
- }
- }
-
- return EOK;
-}
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 3bc30ab8641b1787ded15165890e61836e46e802..fc2dca8d7a9e9dc1e5d68c98f95a5d3d67231f4a 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -700,6 +700,7 @@ static void nsssrv_dp_send_acct_req_done(struct tevent_req *req)
ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req,
&err_maj, &err_min,
&err_msg);
+ talloc_zfree(req);
if (ret != EOK) {
NSS_CMD_FATAL_ERROR(cb_ctx->cctx);
}
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 3b2d509e237b12516e1234a34a8542ae09752c43..2e544cd5aa5a566e5557f2d9280b57b24f39befd 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -994,6 +994,7 @@ static void pam_dp_send_acct_req_done(struct tevent_req *req)
ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req,
&err_maj, &err_min,
&err_msg);
+ talloc_zfree(req);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
("Fatal error, killing connection!\n"));
diff --git a/src/responder/sudo/sudosrv_get_sudorules.c b/src/responder/sudo/sudosrv_get_sudorules.c
index 5d54f95ab78bc43338dd55205e85dbba7bd5f437..1723fd42c8222e72e46498a3b83e427099243369 100644
--- a/src/responder/sudo/sudosrv_get_sudorules.c
+++ b/src/responder/sudo/sudosrv_get_sudorules.c
@@ -181,6 +181,7 @@ static void sudosrv_dp_send_acct_req_done(struct tevent_req *req)
ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req,
&err_maj, &err_min,
&err_msg);
+ talloc_zfree(req);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,
("Fatal error, killing connection!\n"));
--
1.7.4.1