[sssd/f16] Fixes a serious memory hierarchy bug causing unpredictable behavior in the LDAP provider.
Stephen Gallagher
sgallagh at fedoraproject.org
Thu Feb 2 19:24:58 UTC 2012
commit abd541eb0ab81f367ecf87918b80141f0aaf9f63
Author: Stephen Gallagher <sgallagh at redhat.com>
Date: Thu Feb 2 14:23:16 2012 -0500
Fixes a serious memory hierarchy bug causing unpredictable behavior in the LDAP provider.
0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch | 265 ++++++++++++++++++++++
sssd.spec | 7 +-
2 files changed, 271 insertions(+), 1 deletions(-)
---
diff --git a/0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch b/0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch
new file mode 100644
index 0000000..cd58c17
--- /dev/null
+++ b/0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch
@@ -0,0 +1,265 @@
+From 707b20e80a5c5b86944dc55bbc652b392a4c6454 Mon Sep 17 00:00:00 2001
+From: Stephen Gallagher <sgallagh at 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
+
diff --git a/sssd.spec b/sssd.spec
index f6db929..e6ca874 100644
--- a/sssd.spec
+++ b/sssd.spec
@@ -19,7 +19,7 @@
Name: sssd
Version: 1.7.0
-Release: 3%{?dist}
+Release: 4%{?dist}
Group: Applications/System
Summary: System Security Services Daemon
License: GPLv3+
@@ -30,6 +30,7 @@ BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
### Patches ###
Patch0001: 0001-LDAP-Do-not-fail-if-RootDSE-check-cannot-determine-s.patch
+Patch0002: 0002-DP-Fix-bugs-in-sss_dp_get_account_int.patch
### Dependencies ###
@@ -379,6 +380,10 @@ fi
%postun -n libipa_hbac -p /sbin/ldconfig
%changelog
+* Wed Feb 01 2012 Stephen Gallagher <sgallagh at redhat.com> - 1.7.0-4
+- Fixes a serious memory hierarchy bug causing unpredictable behavior in the
+ LDAP provider.
+
* Wed Feb 01 2012 Stephen Gallagher <sgallagh at redhat.com> - 1.7.0-3
- Resolves: rhbz#773706 - SSSD fails during autodetection of search bases for
new LDAP features
More information about the scm-commits
mailing list