https://fedorahosted.org/sssd/ticket/1869
Currently the private data passed to the PAM request is a structure allocated on the client context. But in the odd case where the back end would be stopped or stuck until the idle timeout hits, the DP callback. This patch introduces a new structure allocated on responder context, whose only purpose is to live as long as the request is active. The attached patch is mostly a workaround, I think the correct fix would be to start using a tevent_req-styled request structure similar to the one we use in the NSS responder, but we've seen users hitting this problem even on RHEL5 with sssd-1-5 recently, so I would like to fix the problem in some way. If this patch is accepted, I'll file a ticket to convert the PAM responder to a tevent_req interface for better manageability.
>From 4b5292f3ac1f65cdb3d96c56ab441fc10b1b0128 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Fri, 5 Apr 2013 17:02:20 +0200 Subject: [PATCH] Allocate PAM DP request data on responder context https://fedorahosted.org/sssd/ticket/1869 Currently the private data passed to the PAM request is a structure allocated on the client context. But in the odd case where the back end would be stopped or stuck until the idle timeout hits, the DP callback would access data that were freed when the client timed out. This patch introduces a new structure allocated on responder context, whose only purpose is to live as long as the request is active. --- src/responder/pam/pamsrv.h | 6 ++++++ src/responder/pam/pamsrv_cmd.c | 12 ++++++++++++ src/responder/pam/pamsrv_dp.c | 41 ++++++++++++++++++++++++++++++++++++----- 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h index 3617231d025d1e7dd7ff116f0f74adfb8024cbb7..129419b56c801307280dcd89b633cb018cbe31f6 100644 --- a/src/responder/pam/pamsrv.h +++ b/src/responder/pam/pamsrv.h @@ -39,6 +39,10 @@ struct pam_ctx { hash_table_t *id_table; }; +struct pam_auth_dp_req { + struct pam_auth_req *preq; +}; + struct pam_auth_req { struct cli_ctx *cctx; struct sss_domain_info *domain; @@ -50,6 +54,8 @@ struct pam_auth_req { struct ldb_result *res; bool check_provider; void *data; + + struct pam_auth_dp_req *dpreq_spy; }; struct sss_cmd_table *get_pam_cmds(void); diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index e6300a759832e61202db1e2c2379ee98be7a51e3..ba4e969ffae74c65a73b2ecca849c83ed75fe172 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -739,6 +739,17 @@ done: return ret; } +static int pam_auth_req_destructor(struct pam_auth_req *preq) +{ + if (preq && preq->dpreq_spy) { + /* If there is still a request pending, tell the spy + * the client is going away + */ + preq->dpreq_spy->preq = NULL; + } + return 0; +} + static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd) { struct sss_domain_info *dom; @@ -754,6 +765,7 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd) if (!preq) { return ENOMEM; } + talloc_set_destructor(preq, pam_auth_req_destructor); preq->cctx = cctx; preq->pd = create_pam_data(preq); diff --git a/src/responder/pam/pamsrv_dp.c b/src/responder/pam/pamsrv_dp.c index afaca59370e2c12f74345ecb743568b33af005a2..46c54dadda8ab2083984cc228a9a36283fff3a94 100644 --- a/src/responder/pam/pamsrv_dp.c +++ b/src/responder/pam/pamsrv_dp.c @@ -37,13 +37,26 @@ static void pam_dp_process_reply(DBusPendingCall *pending, void *ptr) DBusMessage* msg; int ret; int type; - struct pam_auth_req *preq; + struct pam_auth_req *preq = NULL; + struct pam_auth_dp_req *pdp_req; - preq = talloc_get_type(ptr, struct pam_auth_req); + pdp_req = talloc_get_type(ptr, struct pam_auth_dp_req); + preq = pdp_req->preq; + talloc_free(pdp_req); dbus_error_init(&dbus_error); - msg = dbus_pending_call_steal_reply(pending); + + /* Check if the client still exists. If not, simply free all the resources + * and quit */ + if (preq == NULL) { + DEBUG(SSSDBG_MINOR_FAILURE, ("Client already disconnected\n")); + dbus_pending_call_unref(pending); + dbus_message_unref(msg); + return; + } + + /* Sanity-check of message validity */ if (msg == NULL) { DEBUG(0, ("Severe error. A reply callback was called but no reply was" "received and no timeout occurred\n")); @@ -51,7 +64,6 @@ static void pam_dp_process_reply(DBusPendingCall *pending, void *ptr) goto done; } - type = dbus_message_get_type(msg); switch (type) { case DBUS_MESSAGE_TYPE_METHOD_RETURN: @@ -79,6 +91,16 @@ done: preq->callback(preq); } +static int pdp_req_destructor(struct pam_auth_dp_req *pdp_req) +{ + if (pdp_req && pdp_req->preq) { + /* If there is still a client waiting, reset the + * spy */ + pdp_req->preq->dpreq_spy = NULL; + } + return 0; +} + int pam_dp_send_req(struct pam_auth_req *preq, int timeout) { struct pam_data *pd = preq->pd; @@ -86,6 +108,7 @@ int pam_dp_send_req(struct pam_auth_req *preq, int timeout) DBusMessage *msg; dbus_bool_t ret; int res; + struct pam_auth_dp_req *pdp_req; /* double check dp_ctx has actually been initialized. * in some pathological cases it may happen that nss starts up before @@ -118,9 +141,17 @@ int pam_dp_send_req(struct pam_auth_req *preq, int timeout) return EIO; } + pdp_req = talloc(preq->cctx->rctx, struct pam_auth_dp_req); + if (pdp_req == NULL) { + return ENOMEM; + } + pdp_req->preq = preq; + preq->dpreq_spy = pdp_req; + talloc_set_destructor(pdp_req, pdp_req_destructor); + res = sbus_conn_send(be_conn->conn, msg, timeout, pam_dp_process_reply, - preq, NULL); + pdp_req, NULL); dbus_message_unref(msg); return res; } -- 1.8.1.4
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
