[SSSD] [PATCH] Add support for terminating idle connections in sssd_nss

Simo Sorce simo at redhat.com
Wed May 30 13:08:21 UTC 2012


On Wed, 2012-05-30 at 08:23 -0400, Stephen Gallagher wrote:
> This patch was submitted by Shantanu Goel in a Trac ticket. Sending to
> the list for review. I haven't yet had a chance to dig into it myself
> yet, but the concept as described in
> https://fedorahosted.org/sssd/ticket/1354 is sound, so I'd like to see
> this get cleaned up and included.

Comments inline,
but I can already say I want more comments in the code, there are none.

> 
> 
> 
> 
> 
> 
> differences
> between files
> attachment
> (sssd-1.5.1-expire-idle-connection.diff)
> 
> --- .gen-patch.27968/src/confdb/confdb.h        2012-04-20
> 14:01:34.477465315 -0400
> +++ sssd-1.5.1/src/confdb/confdb.h      2012-04-20 14:01:45.069779773
> -0400
> @@ -78,6 +78,7 @@
>  #define CONFDB_NSS_VETOED_SHELL  "vetoed_shells"
>  #define CONFDB_NSS_ALLOWED_SHELL "allowed_shells"
>  #define CONFDB_NSS_SHELL_FALLBACK "shell_fallback"
> +#define CONFDB_NSS_IDLE_TIMEOUT "idle_timeout"
>  
>  /* PAM */
>  #define CONFDB_PAM_CONF_ENTRY "config/pam"
> --- .gen-patch.27968/src/responder/common/responder_common.c    2012-04-20 14:01:34.478465251 -0400
> +++ sssd-1.5.1/src/responder/common/responder_common.c  2012-04-20
> 14:01:45.070779708 -0400
> @@ -88,9 +88,82 @@
>      return EOK;
>  }
>  
> +static void idle_handler(struct tevent_context *,
> +                        struct tevent_timer *,
> +                        struct timeval,
> +                        void *);
> +
> +static void start_timer(struct resp_ctx *rctx, struct timeval *tvp)
> +{
> +    struct timeval tv;
> +
> +    if (rctx->idle_timeout <= 0)
> +       return;
> +    if (rctx->timer)
> +       return;
> +    if (!rctx->cctx)
> +       return;

Style guidelines violations ^^
We use:
if (x) {
    blah
}

> +    if (!tvp) {
> +       tvp = &tv;
> +       gettimeofday(tvp, NULL);
> +    }

Please use tevent_timeval_current() here ^

> +    tvp->tv_sec += rctx->idle_timeout;
> +    rctx->timer = tevent_add_timer(rctx->cctx->ev, rctx,
> +                                  *tvp, idle_handler, rctx);
> +    if (!rctx->timer)
> +       DEBUG(0, ("could not start idle timer\n"));
> +}
> +
> +static void idle_handler(struct tevent_context *ev,
> +                        struct tevent_timer *te,
> +                        struct timeval current_time,
> +                        void *data)
> +{
> +    struct resp_ctx *rctx = data;
> +    struct cli_ctx *cctx = rctx->cctx;
> +    int n = 0;
> +
> +    while (cctx) {
> +       struct cli_ctx *cctx_next = cctx->next;
> +
> +       if (cctx->active || cctx->creq)
> +           cctx->active = 0;
> +       else if (talloc_free(cctx) == 0)
> +           n++;

sever style violation ^^ as well, plus I want a comment on top that
explain why we are doing an either or.

Also what's the point of this check on talloc_free() ?
It is not like we have any options left if it fails.

Also please use a for loop and a continue after the if (), will be much
more readable.

> +       cctx = cctx_next;
> +    }
> +    if (n)
> +       DEBUG(2, ("terminated %d idle connecions\n", n));

braces ^^

> +    /*
> +     * The timer event is freed by the library.
> +     */
> +    rctx->timer = NULL;
> +    start_timer(rctx, &current_time);
> +}
> +
> +static void client_add(struct cli_ctx *cctx)
> +{
> +    struct resp_ctx *rctx = cctx->rctx;
> +
> +    cctx->next = rctx->cctx;
> +    cctx->prev = &rctx->cctx;
> +    rctx->cctx = cctx;
> +    if (cctx->next)
> +       cctx->next->prev = &cctx->next;

use DLIST_* macros here ^^ and elswhere, I do not want to see custom
linked list manipulation unless unavoidable (and in that case I want to
see a comment that explains why it is unavoidable.

> +    start_timer(rctx, NULL);
> +}
> +
>  static int client_destructor(struct cli_ctx *ctx)
>  {
>      if (ctx->cfd > 0) close(ctx->cfd);

style violation, pluse return of close is not checked, check and fire a
debug message in case of error.

> +    if (ctx->prev) {
> +       *ctx->prev = ctx->next;
> +       if (ctx->next)
> +           ctx->next->prev = ctx->prev;
> +       ctx->next = NULL;
> +       ctx->prev = NULL;
> +    }

use DLIST_*

>      return 0;
>  }
>  
> @@ -132,6 +205,8 @@
>  {
>      int ret;
>  
> +    cctx->active++;
> +
>      ret = sss_packet_send(cctx->creq->out, cctx->cfd);
>      if (ret == EAGAIN) {
>          /* not all data was sent, loop again */
> @@ -155,6 +230,8 @@
>  {
>      int ret;
>  
> +    cctx->active++;
> +

Why these active++ up here, it feels wrong.
I haven't checked this throghly but I would think either an accessor
function should be used or even just fold this into
client_add()/start_timer()

>      if (!cctx->creq) {
>          cctx->creq = talloc_zero(cctx, struct cli_request);
>          if (!cctx->creq) {
> @@ -298,6 +375,8 @@
>      cctx->ev = ev;
>      cctx->rctx = rctx;
>  
> +    client_add(cctx);
> +
>      talloc_set_destructor(cctx, client_destructor);
>  
>      DEBUG(4, ("Client connected to privileged pipe!\n"));
> @@ -356,6 +435,8 @@
>      cctx->ev = ev;
>      cctx->rctx = rctx;
>  
> +    client_add(cctx);
> +
>      talloc_set_destructor(cctx, client_destructor);
>  
>      DEBUG(4, ("Client connected!\n"));
> @@ -585,24 +666,27 @@
>      return EIO;
>  }
>  
> -int sss_process_init(TALLOC_CTX *mem_ctx,
> -                     struct tevent_context *ev,
> -                     struct confdb_ctx *cdb,
> -                     struct sss_cmd_table sss_cmds[],
> -                     const char *sss_pipe_name,
> -                     const char *sss_priv_pipe_name,
> -                     const char *confdb_service_path,
> -                     const char *svc_name,
> -                     uint16_t svc_version,
> -                     struct sbus_interface *monitor_intf,
> -                     const char *cli_name,
> -                     struct sbus_interface *dp_intf,
> -                     struct resp_ctx **responder_ctx)
> +int sss_process_init_timeout(TALLOC_CTX *mem_ctx,
> +                            struct tevent_context *ev,
> +                            struct confdb_ctx *cdb,
> +                            struct sss_cmd_table sss_cmds[],
> +                            const char *sss_pipe_name,
> +                            const char *sss_priv_pipe_name,
> +                            const char *confdb_service_path,
> +                            const char *svc_name,
> +                            uint16_t svc_version,
> +                            struct sbus_interface *monitor_intf,
> +                            const char *cli_name,
> +                            struct sbus_interface *dp_intf,
> +                            struct resp_ctx **responder_ctx,
> +                            time_t idle_timeout)
>  {
>      struct resp_ctx *rctx;
>      struct sss_domain_info *dom;
>      int ret;
>  
> +    DEBUG(2, ("connection idle timeout %d\n", idle_timeout));
> +
>      rctx = talloc_zero(mem_ctx, struct resp_ctx);
>      if (!rctx) {
>          DEBUG(0, ("fatal error initializing resp_ctx\n"));
> @@ -614,6 +698,7 @@
>      rctx->sock_name = sss_pipe_name;
>      rctx->priv_sock_name = sss_priv_pipe_name;
>      rctx->confdb_service_path = confdb_service_path;
> +    rctx->idle_timeout = idle_timeout;
>  
>      ret = confdb_get_domains(rctx->cdb, &rctx->domains);
>      if (ret != EOK) {
> @@ -666,6 +751,27 @@
>      return EOK;
>  }
>  
> +int sss_process_init(TALLOC_CTX *mem_ctx,
> +                    struct tevent_context *ev,
> +                    struct confdb_ctx *cdb,
> +                    struct sss_cmd_table sss_cmds[],
> +                    const char *sss_pipe_name,
> +                    const char *sss_priv_pipe_name,
> +                    const char *confdb_service_path,
> +                    const char *svc_name,
> +                    uint16_t svc_version,
> +                    struct sbus_interface *monitor_intf,
> +                    const char *cli_name,
> +                    struct sbus_interface *dp_intf,
> +                    struct resp_ctx **responder_ctx)
> +{
> +    return sss_process_init_timeout(mem_ctx, ev, cdb, sss_cmds,
> +                                   sss_pipe_name, sss_priv_pipe_name,
> +                                   confdb_service_path, svc_name,
> +                                   svc_version, monitor_intf,
> cli_name,
> +                                   dp_intf, responder_ctx, 0);
> +}

Why not simply add the timeval and keep the original name ?
In what case do we want to idle out only for nss stuff ?
I think we always want to idle out for any client connection, whtehr it
is for nss, pam, ssh, sudo, etc...

>  int sss_dp_get_domain_conn(struct resp_ctx *rctx, const char *domain,
>                             struct be_conn **_conn)
>  {
> --- .gen-patch.27968/src/responder/common/responder.h   2012-04-20
> 14:01:34.478465251 -0400
> +++ sssd-1.5.1/src/responder/common/responder.h 2012-04-20
> 14:01:45.070779708 -0400
> @@ -90,6 +90,11 @@
>      struct sss_names_ctx *names;
>  
>      void *pvt_ctx;
> +
> +    struct cli_ctx *cctx;
> +
> +    time_t idle_timeout;
> +    struct tevent_timer *timer;
>  };
>  
>  /* Needed for the NSS responder */
> @@ -120,6 +125,11 @@
>      int netgrent_cur;
>  
>      time_t pam_timeout;
> +
> +    int active;
> +
> +    struct cli_ctx *next;
> +    struct cli_ctx **prev;
>  };
>  
>  struct sss_cmd_table {
> @@ -142,6 +152,21 @@
>                       struct sbus_interface *dp_intf,
>                       struct resp_ctx **responder_ctx);
>  
> +int sss_process_init_timeout(TALLOC_CTX *mem_ctx,
> +                            struct tevent_context *ev,
> +                            struct confdb_ctx *cdb,
> +                            struct sss_cmd_table sss_cmds[],
> +                            const char *sss_pipe_name,
> +                            const char *sss_priv_pipe_name,
> +                            const char *confdb_service_path,
> +                            const char *svc_name,
> +                            uint16_t svc_version,
> +                            struct sbus_interface *monitor_intf,
> +                            const char *cli_name,
> +                            struct sbus_interface *dp_intf,
> +                            struct resp_ctx **responder_ctx,
> +                            time_t idle_timeout);
> +
>  int sss_parse_name(TALLOC_CTX *memctx,
>                     struct sss_names_ctx *snctx,
>                     const char *orig, char **domain, char **name);
> --- .gen-patch.27968/src/responder/nss/nsssrv.c 2012-04-20
> 14:01:34.478465251 -0400
> +++ sssd-1.5.1/src/responder/nss/nsssrv.c       2012-04-20
> 14:01:45.070779708 -0400
> @@ -258,6 +258,7 @@
>      struct nss_ctx *nctx;
>      int ret, max_retries;
>      int hret;
> +    int idle_timeout;
>  
>      nctx = talloc_zero(mem_ctx, struct nss_ctx);
>      if (!nctx) {
> @@ -273,15 +274,22 @@
>  
>      nss_cmds = get_nss_cmds();
>  
> -    ret = sss_process_init(nctx, ev, cdb,
> -                           nss_cmds,
> -                           SSS_NSS_SOCKET_NAME, NULL,
> -                           CONFDB_NSS_CONF_ENTRY,
> -                           NSS_SBUS_SERVICE_NAME,
> -                           NSS_SBUS_SERVICE_VERSION,
> -                           &monitor_nss_interface,
> -                           "NSS", &nss_dp_interface,
> -                           &nctx->rctx);
> +    ret = confdb_get_int(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
> +                         CONFDB_NSS_IDLE_TIMEOUT, 0, &idle_timeout);
> +    if (ret != EOK) {
> +        DEBUG(0, ("fatal error getting nss config\n"));

use new debug codes ^^ (also above and below elsewhere).

> +        return ret;
> +    }
> +
> +    ret = sss_process_init_timeout(nctx, ev, cdb,
> +                                  nss_cmds,
> +                                  SSS_NSS_SOCKET_NAME, NULL,
> +                                  CONFDB_NSS_CONF_ENTRY,
> +                                  NSS_SBUS_SERVICE_NAME,
> +                                  NSS_SBUS_SERVICE_VERSION,
> +                                  &monitor_nss_interface,
> +                                  "NSS", &nss_dp_interface,
> +                                  &nctx->rctx, idle_timeout);
>      if (ret != EOK) {
>          return ret;
>      }
> --- .gen-patch.27968/src/sss_client/common.c    2012-04-20
> 14:01:34.479465187 -0400
> +++ sssd-1.5.1/src/sss_client/common.c  2012-04-20 14:01:51.388370756
> -0400
> @@ -38,6 +38,7 @@
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <string.h>
> +#include <signal.h>
>  #include <fcntl.h>
>  #include <poll.h>
>  
> @@ -154,8 +155,8 @@
>              }
>  
>              /* Write failed */
> -            sss_cli_close_socket();
>              *errnop = errno;
> +            sss_cli_close_socket();
>              return NSS_STATUS_UNAVAIL;
>          }
>  
> @@ -264,8 +265,8 @@
>               * since the transaction has failed half way
>               * through. */
>  
> -            sss_cli_close_socket();
>              *errnop = errno;
> +            sss_cli_close_socket();
>              ret = NSS_STATUS_UNAVAIL;
>              goto failed;
>          }

The above 2 sound like some sort of bugfix, so they shouldn't be merged
in the same patch, they need a separate patch with a good explanation in
the commit message.

> @@ -363,12 +364,11 @@
>   * 0-3: 32bit unsigned version number
>   */
>  
> -static int sss_nss_check_version(const char *socket_name)
> +static int sss_nss_check_version(const char *socket_name, int
> *errnop)
>  {
>      uint8_t *repbuf;
>      size_t replen;
>      enum nss_status nret;
> -    int errnop;
>      int res = NSS_STATUS_UNAVAIL;
>      uint32_t expected_version;
>      struct sss_cli_req_data req;
> @@ -379,6 +379,7 @@
>                 strcmp(socket_name, SSS_PAM_PRIV_SOCKET_NAME) == 0) {
>          expected_version = SSS_PAM_PROTOCOL_VERSION;
>      } else {
> +       *errnop = EFAULT;
>          return NSS_STATUS_UNAVAIL;
>      }
>  
> @@ -386,20 +387,20 @@
>      req.data = &expected_version;
>  
>      nret = sss_nss_make_request_nochecks(SSS_GET_VERSION, &req,
> -                                         &repbuf, &replen, &errnop);
> +                                         &repbuf, &replen, errnop);
>      if (nret != NSS_STATUS_SUCCESS) {
>          return nret;
>      }
>  
> -    if (!repbuf) {
> -        return res;
> -    }
> -
> -    if (((uint32_t *)repbuf)[0] == expected_version) {
> +    if (repbuf && ((uint32_t *)repbuf)[0] == expected_version) {
> +       *errnop = 0;
>          res = NSS_STATUS_SUCCESS;
> -    }
> +    } else
> +       *errnop = EFAULT;

style

> +    if (repbuf)
> +       free(repbuf);

useless change, do not do it!
free() does not need guards against NULL
 
> -    free(repbuf);
>      return res;
>  }
>  
> @@ -670,12 +671,11 @@
>  
>      sss_cli_sd = mysd;
>  
> +-    if (sss_nss_check_version(socket_name) == NSS_STATUS_SUCCESS) {
> +    if (sss_nss_check_version(socket_name, errnop) ==
> NSS_STATUS_SUCCESS) {

Line wrapping, plus if you are changin this line then split it in
ret = foo()
if (ret == bar) {

>          return SSS_STATUS_SUCCESS;
>      }
>  
>      sss_cli_close_socket();
> -    *errnop = EFAULT;
>      return SSS_STATUS_UNAVAIL;
>  }
>  
> @@ -686,8 +686,10 @@
>                        uint8_t **repbuf, size_t *replen,
>                        int *errnop)
>  {
> -    enum sss_status ret;
>      char *envval;
> +    int retry, restore;
> +    struct sigaction sa, osa;
> +    enum nss_status ret = NSS_STATUS_UNAVAIL;
>  
>      /* avoid looping in the nss daemon */
>      envval = getenv("_SSS_LOOPS");
> @@ -695,12 +697,32 @@
>          return NSS_STATUS_NOTFOUND;
>      }
>  
> -    ret = sss_cli_check_socket(errnop, SSS_NSS_SOCKET_NAME);
> -    if (ret != SSS_STATUS_SUCCESS) {
> -        return NSS_STATUS_UNAVAIL;
> +    /*
> +     * Block off SIGPIPE and retry the command
> +     * in case the other end closes the pipe on us.
> +     */
> +    sigemptyset(&sa.sa_mask);
> +    sa.sa_flags = 0;
> +    sa.sa_handler = SIG_IGN;
> +    if (sigaction(SIGPIPE, &sa, &osa) == 0) {
> +       restore = 1;
> +       retry = 2;
> +    } else {
> +       restore = 0;
> +       retry = 1;
>      }

NACK NACK NACK

We do not mess with application signals in a nsswitch library
absolutely unacceptable.

> +    do {
> +       if (sss_cli_check_socket(errnop,
> +                                SSS_NSS_SOCKET_NAME) ==
> SSS_STATUS_SUCCESS)
> +           ret = sss_nss_make_request_nochecks(cmd, rd, repbuf,
> +                                               replen, errnop);
> +    } while (ret == NSS_STATUS_UNAVAIL
> +            && (*errnop == EPIPE || *errnop == 0)
> +            && --retry > 0);
> +    if (restore)
> +       sigaction(SIGPIPE, &osa, NULL);
>  
> -    return sss_nss_make_request_nochecks(cmd, rd, repbuf, replen,
> errnop);
> +    return ret;
>  }
>  
>  errno_t check_server_cred(int sockfd)
> 
> 
It's a NACK.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list