diff options
| -rw-r--r-- | src/responder/common/responder_dp.c | 110 | ||||
| -rw-r--r-- | src/responder/nss/nsssrv_cmd.c | 1 | ||||
| -rw-r--r-- | src/responder/pam/pamsrv_cmd.c | 1 | ||||
| -rw-r--r-- | 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 6bc086c2a..b9d05cdb3 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 @@ -253,7 +262,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; @@ -360,19 +369,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; } @@ -419,23 +428,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 @@ -616,7 +612,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; @@ -647,58 +643,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 dec7f3050..e3420fa2e 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c @@ -726,6 +726,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 8cb642217..0f9fd5ed8 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 b1c3fa5e8..45eecf56f 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")); |