From 270a0a1b6182ef1fbff2a93af6731788cf954874 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Thu, 29 Apr 2010 12:30:05 -0400 Subject: Better handle sdap_handle memory from callers. Always just mark the sdap_handle as not connected and let later _send() functions to take care of freeing the handle before reconnecting. Introduce restart functions to avoid calling _send() functions in _done() functions error paths as this would have the same effect as directly freeing the sdap_handle and cause access to freed memory in sdap_handle_release() By freeing sdap_handle only in the connection _recv() function we guarantee it can never be done within sdap_handle_release() but only in a following event. --- src/providers/data_provider_be.c | 2 +- src/providers/dp_backend.h | 4 ++ src/providers/ipa/ipa_access.c | 8 --- src/providers/ldap/ldap_common.c | 2 +- src/providers/ldap/ldap_id.c | 56 ++++++++++----- src/providers/ldap/ldap_id_enum.c | 111 ++++++++++++++++++++++++----- src/providers/ldap/sdap_async_connection.c | 3 + 7 files changed, 144 insertions(+), 42 deletions(-) diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 69aba6ceb..e5a5ff7b0 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -405,7 +405,7 @@ static int be_get_account_info(DBusMessage *message, struct sbus_connection *con } /* process request */ - be_req = talloc(becli, struct be_req); + be_req = talloc_zero(becli, struct be_req); if (!be_req) { err_maj = DP_ERR_FATAL; err_min = ENOMEM; diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h index f1069d0db..d657af18b 100644 --- a/src/providers/dp_backend.h +++ b/src/providers/dp_backend.h @@ -103,6 +103,8 @@ struct bet_ops { be_req_fn_t finalize; }; +#define MAX_BE_REQ_RESTARTS 2 + struct be_req { struct be_client *becli; struct be_ctx *be_ctx; @@ -110,6 +112,8 @@ struct be_req { be_async_callback_t fn; void *pvt; + + int restarts; }; struct be_acct_req { diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c index a7058bad1..d88e5c931 100644 --- a/src/providers/ipa/ipa_access.c +++ b/src/providers/ipa/ipa_access.c @@ -325,10 +325,6 @@ static struct tevent_req *hbac_get_host_info_send(TALLOC_CTX *memctx, } if (sdap_ctx->gsh == NULL || ! sdap_ctx->gsh->connected) { - if (sdap_ctx->gsh != NULL) { - talloc_zfree(sdap_ctx->gsh); - } - subreq = sdap_cli_connect_send(state, ev, sdap_ctx->opts, sdap_ctx->be, sdap_ctx->service, NULL); if (!subreq) { @@ -780,10 +776,6 @@ static struct tevent_req *hbac_get_rules_send(TALLOC_CTX *memctx, } if (sdap_ctx->gsh == NULL || ! sdap_ctx->gsh->connected) { - if (sdap_ctx->gsh != NULL) { - talloc_zfree(sdap_ctx->gsh); - } - subreq = sdap_cli_connect_send(state, ev, sdap_ctx->opts, sdap_ctx->be, sdap_ctx->service, NULL); if (!subreq) { diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c index b5765c276..90ec7e2e7 100644 --- a/src/providers/ldap/ldap_common.c +++ b/src/providers/ldap/ldap_common.c @@ -359,7 +359,7 @@ void sdap_mark_offline(struct sdap_id_ctx *ctx) if (ctx->gsh) { /* make sure we mark the connection as gone when we go offline so that * we do not try to reuse a bad connection by mistale later */ - talloc_zfree(ctx->gsh); + ctx->gsh->connected = false; } be_mark_offline(ctx->be); diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c index 15e2f30ad..a2217da66 100644 --- a/src/providers/ldap/ldap_id.c +++ b/src/providers/ldap/ldap_id.c @@ -99,8 +99,6 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx, if (!sdap_connected(ctx)) { - if (ctx->gsh) talloc_zfree(ctx->gsh); - /* FIXME: add option to decide if tls should be used * or SASL/GSSAPI, etc ... */ subreq = sdap_cli_connect_send(state, ev, ctx->opts, @@ -299,8 +297,6 @@ struct tevent_req *groups_get_send(TALLOC_CTX *memctx, if (!sdap_connected(ctx)) { - if (ctx->gsh) talloc_zfree(ctx->gsh); - /* FIXME: add option to decide if tls should be used * or SASL/GSSAPI, etc ... */ subreq = sdap_cli_connect_send(state, ev, ctx->opts, @@ -465,8 +461,6 @@ static struct tevent_req *groups_by_user_send(TALLOC_CTX *memctx, if (!sdap_connected(ctx)) { - if (ctx->gsh) talloc_zfree(ctx->gsh); - /* FIXME: add option to decide if tls should be used * or SASL/GSSAPI, etc ... */ subreq = sdap_cli_connect_send(state, ev, ctx->opts, @@ -654,13 +648,43 @@ void sdap_account_info_handler(struct be_req *breq) if (ret != EOK) return sdap_handler_done(breq, DP_ERR_FATAL, ret, err); } +static void sdap_account_info_immediate(struct tevent_context *ctx, + struct tevent_immediate *im, + void *private_data) +{ + struct be_req *breq = talloc_get_type(private_data, struct be_req); + + sdap_account_info_handler(breq); +} + +static int sdap_account_info_restart(struct be_req *breq) +{ + struct tevent_immediate *im; + + breq->restarts++; + if (breq->restarts > MAX_BE_REQ_RESTARTS) { + return ELOOP; + } + + im = tevent_create_immediate(breq); + if (!im) { + return ENOMEM; + } + + /* schedule a completely new event to avoid deep recursions */ + tevent_schedule_immediate(im, breq->be_ctx->ev, + sdap_account_info_immediate, breq); + + return EOK; +} + static void sdap_account_info_users_done(struct tevent_req *req) { struct be_req *breq = tevent_req_callback_data(req, struct be_req); struct sdap_id_ctx *ctx; int dp_err = DP_ERR_OK; const char *error = NULL; - int ret; + int ret, err; ret = users_get_recv(req); talloc_zfree(req); @@ -674,9 +698,9 @@ static void sdap_account_info_users_done(struct tevent_req *req) ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); if (sdap_check_gssapi_reconnect(ctx)) { - talloc_zfree(ctx->gsh); - sdap_account_info_handler(breq); - return; + ctx->gsh->connected = false; + err = sdap_account_info_restart(breq); + if (err == EOK) return; } sdap_mark_offline(ctx); } @@ -691,7 +715,7 @@ static void sdap_account_info_groups_done(struct tevent_req *req) struct sdap_id_ctx *ctx; int dp_err = DP_ERR_OK; const char *error = NULL; - int ret; + int ret, err; ret = groups_get_recv(req); talloc_zfree(req); @@ -705,9 +729,9 @@ static void sdap_account_info_groups_done(struct tevent_req *req) ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); if (sdap_check_gssapi_reconnect(ctx)) { - talloc_zfree(ctx->gsh); - sdap_account_info_handler(breq); - return; + ctx->gsh->connected = false; + err = sdap_account_info_restart(breq); + if (err == EOK) return; } sdap_mark_offline(ctx); } @@ -736,8 +760,8 @@ static void sdap_account_info_initgr_done(struct tevent_req *req) ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct sdap_id_ctx); if (sdap_check_gssapi_reconnect(ctx)) { - talloc_zfree(ctx->gsh); - sdap_account_info_handler(breq); + ctx->gsh->connected = false; + sdap_account_info_restart(breq); return; } sdap_mark_offline(ctx); diff --git a/src/providers/ldap/ldap_id_enum.c b/src/providers/ldap/ldap_id_enum.c index bc06e8bdc..d86b38d0f 100644 --- a/src/providers/ldap/ldap_id_enum.c +++ b/src/providers/ldap/ldap_id_enum.c @@ -143,12 +143,14 @@ int ldap_id_enumerate_set_timer(struct sdap_id_ctx *ctx, struct timeval tv) return EOK; } +#define MAX_ENUM_RESTARTS 3 struct global_enum_state { struct tevent_context *ev; struct sdap_id_ctx *ctx; bool purge; + int restarts; }; static struct tevent_req *enum_users_send(TALLOC_CTX *memctx, @@ -162,6 +164,8 @@ static struct tevent_req *enum_groups_send(TALLOC_CTX *memctx, bool purge); static void ldap_id_enum_groups_done(struct tevent_req *subreq); static void ldap_id_enum_cleanup_done(struct tevent_req *subreq); +static int ldap_id_enum_users_restart(struct tevent_req *req); +static int ldap_id_enum_groups_restart(struct tevent_req *req); static struct tevent_req *ldap_id_enumerate_send(struct tevent_context *ev, struct sdap_id_ctx *ctx) @@ -175,6 +179,7 @@ static struct tevent_req *ldap_id_enumerate_send(struct tevent_context *ev, state->ev = ev; state->ctx = ctx; + state->restarts = 0; ctx->last_enum = tevent_timeval_current(); @@ -203,6 +208,7 @@ static void ldap_id_enum_users_done(struct tevent_req *subreq) struct global_enum_state); enum tevent_req_state tstate; uint64_t err = 0; + int ret; if (tevent_req_is_error(subreq, &tstate, &err)) { if (tstate != TEVENT_REQ_USER_ERROR) { @@ -228,12 +234,9 @@ fail: (int)err, strerror(err))); if (sdap_check_gssapi_reconnect(state->ctx)) { - talloc_zfree(state->ctx->gsh); - subreq = enum_users_send(state, state->ev, state->ctx, state->purge); - if (subreq != NULL) { - tevent_req_set_callback(subreq, ldap_id_enum_users_done, req); - return; - } + state->ctx->gsh->connected = false; + ret = ldap_id_enum_users_restart(req); + if (ret == EOK) return; } sdap_mark_offline(state->ctx); } @@ -250,6 +253,7 @@ static void ldap_id_enum_groups_done(struct tevent_req *subreq) struct global_enum_state); enum tevent_req_state tstate; uint64_t err = 0; + int ret; if (tevent_req_is_error(subreq, &tstate, &err)) { if (tstate != TEVENT_REQ_USER_ERROR) { @@ -278,12 +282,9 @@ static void ldap_id_enum_groups_done(struct tevent_req *subreq) fail: /* check if credentials are expired otherwise go offline on failures */ if (sdap_check_gssapi_reconnect(state->ctx)) { - talloc_zfree(state->ctx->gsh); - subreq = enum_groups_send(state, state->ev, state->ctx, state->purge); - if (subreq != NULL) { - tevent_req_set_callback(subreq, ldap_id_enum_groups_done, req); - return; - } + state->ctx->gsh->connected = false; + ret = ldap_id_enum_groups_restart(req); + if (ret == EOK) return; } sdap_mark_offline(state->ctx); DEBUG(1, ("Failed to enumerate groups (%d [%s]), retrying later!\n", @@ -299,6 +300,88 @@ static void ldap_id_enum_cleanup_done(struct tevent_req *subreq) tevent_req_done(req); } +static void ldap_id_enum_users_immediate(struct tevent_context *ctx, + struct tevent_immediate *im, + void *private_data) +{ + struct tevent_req *req = talloc_get_type(private_data, + struct tevent_req); + struct global_enum_state *state = tevent_req_data(req, + struct global_enum_state); + struct tevent_req *subreq; + + subreq = enum_users_send(state, state->ev, state->ctx, state->purge); + if (subreq == NULL) { + tevent_req_error(req, ENOMEM); + return; + } + tevent_req_set_callback(subreq, ldap_id_enum_users_done, req); +} + +static int ldap_id_enum_users_restart(struct tevent_req *req) +{ + struct global_enum_state *state = tevent_req_data(req, + struct global_enum_state); + struct tevent_immediate *im; + + state->restarts++; + if (state->restarts < MAX_ENUM_RESTARTS) { + return ELOOP; + } + + im = tevent_create_immediate(req); + if (!im) { + return ENOMEM; + } + + /* schedule a completely new event to avoid deep recursions */ + tevent_schedule_immediate(im, state->ev, + ldap_id_enum_users_immediate, req); + + return EOK; +} + +static void ldap_id_enum_groups_immediate(struct tevent_context *ctx, + struct tevent_immediate *im, + void *private_data) +{ + struct tevent_req *req = talloc_get_type(private_data, + struct tevent_req); + struct global_enum_state *state = tevent_req_data(req, + struct global_enum_state); + struct tevent_req *subreq; + + subreq = enum_groups_send(state, state->ev, state->ctx, state->purge); + if (subreq == NULL) { + tevent_req_error(req, ENOMEM); + return; + } + tevent_req_set_callback(subreq, ldap_id_enum_groups_done, req); +} + +static int ldap_id_enum_groups_restart(struct tevent_req *req) +{ + struct global_enum_state *state = tevent_req_data(req, + struct global_enum_state); + struct tevent_immediate *im; + + state->restarts++; + if (state->restarts < MAX_ENUM_RESTARTS) { + return ELOOP; + } + + im = tevent_create_immediate(req); + if (!im) { + return ENOMEM; + } + + /* schedule a completely new event to avoid deep recursions */ + tevent_schedule_immediate(im, state->ev, + ldap_id_enum_groups_immediate, req); + + return EOK; +} + /* ==User-Enumeration===================================================== */ @@ -357,8 +440,6 @@ static struct tevent_req *enum_users_send(TALLOC_CTX *memctx, if (!sdap_connected(ctx)) { - if (ctx->gsh) talloc_zfree(ctx->gsh); - /* FIXME: add option to decide if tls should be used * or SASL/GSSAPI, etc ... */ subreq = sdap_cli_connect_send(state, ev, ctx->opts, @@ -512,8 +593,6 @@ static struct tevent_req *enum_groups_send(TALLOC_CTX *memctx, if (!sdap_connected(ctx)) { - if (ctx->gsh) talloc_zfree(ctx->gsh); - /* FIXME: add option to decide if tls should be used * or SASL/GSSAPI, etc ... */ subreq = sdap_cli_connect_send(state, ev, ctx->opts, diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c index 9871dc210..ec36a6018 100644 --- a/src/providers/ldap/sdap_async_connection.c +++ b/src/providers/ldap/sdap_async_connection.c @@ -1119,6 +1119,9 @@ int sdap_cli_connect_recv(struct tevent_req *req, } if (gsh) { + if (*gsh) { + talloc_zfree(*gsh); + } *gsh = talloc_steal(memctx, state->sh); if (!*gsh) { return ENOMEM; -- cgit