[SSSD] sss_client: thread safe initialisation of sss_nss_mc_get_ctx

Lukas Slebodnik lslebodn at redhat.com
Wed Jul 16 13:56:52 UTC 2014


On (16/07/14 14:02), Sumit Bose wrote:
>On Wed, Jul 16, 2014 at 11:32:53AM +0200, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> attached patches fix problems with mmap cache in client code.
>> The 1st patch is at least 5th version, because I found few problems in my
>> previous versions myself. I hope you will not find anything else :-)
>> 
>> Patches change client code. It will be good to have at least 2 ACKs.
>
>Currently we use sss_nss_lock() and sss_nss_unlock() to protect
>sss_nss_make_request() in multi-threaded clients (there is sss_pam_lock()
>and sss_pam_lock() for pam_sss as well). I wonder if a new pair like
>sss_mc_init_lock() sss_mc_init_unlock() might help to protect
>sss_nss_mc_get_ctx() as well?
>
New version with sss_nss_{lock,unlock} is attached.
I didn't want call lock each invocation of sss_nss_mc_get_ctx,
because it should be fast memory cache and locking is useless with already
initialised structure. You can also compare attached patch with first version
Any comments are welcomed.

Function sss_nss_mc_get_ctx was split into two parts for simplification
of locking and unlocking. The locking is used only in new static function
sss_nss_mc_init_ctx. This function will not be called very often therefore the
same mutex is used as in other nss functions.

Testing was already described in 1st mail.
You just need different diff to see logging without locking.
--- a/src/sss_client/nss_mc_common.c
+++ b/src/sss_client/nss_mc_common.c
@@ -113,7 +113,7 @@ static errno_t sss_nss_mc_init_ctx(const char *name,
     int ret;
 
     fprintf(stderr, "before nss lock\n");
-    sss_nss_lock();
+    //sss_nss_lock();
     fprintf(stderr, "after nss lock\n");
     /* check if ctx is initialised by previous thread. */
     if (ctx->initialized) {
@@ -183,7 +183,7 @@ done:
         memset(ctx, 0, sizeof(struct sss_cli_mc_ctx));
     }
     free(file);
-    sss_nss_unlock();
+    //sss_nss_unlock();
     fprintf(stderr, "after nss unlock\n");
 
     return ret;

LS
-------------- next part --------------
>From 032ce79028cb044485d8836d8290adb7ac33ec08 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 16 Jul 2014 14:32:04 +0200
Subject: [PATCH 1/3] sss_client: thread safe initialisation of sss_cli_mc_ctx

In multi threaded application, it may happen that more threads will call
function getpwuid(or similar) and sss client will not have initialized
structure for fast memory cache. This structure is initialized just once.
There isn't any problem with multi threaded application after successful
initialisation.

The race condition will happen if more threads try to initialise structure
sss_cli_mc_ctx in function sss_nss_mc_get_ctx (ctx->initialized is false)
It takes some time to initialise mmap cache: open file, get file size, mmap
file, initialize structure sss_cli_mc_ctx. One of problems is that file with
memory cache can be opened more times (file descriptor leak), but the race
condition is with initialising structure sss_cli_mc_ctx. One tread will start
to initialise this structure; another thread will think that structure is
already initialised and will check consistency of this structure. It will fail
because 1st thread did not finish initialisation. Therefore 2nd thread will
return EINVAL and will do clean up in done section: munmap, close file and
reset structure data. The 1st thread will finish an try to use memory cache,
but structure was zero initialised by 2nd thread and it will cause dereference
of NULL pointer in 1st thread (SIGSEGV) or dividing by zero in murmurhash
function(SIGFPE)

Function sss_nss_mc_get_ctx was split into two parts for simplification
of locking and unlocking. The locking is used only in new static function
sss_nss_mc_init_ctx. This function will not be called very often therefore the
same mutex is used as in other nss functions.

Resolves:
https://fedorahosted.org/sssd/ticket/2380
---
 src/sss_client/nss_mc_common.c | 44 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/src/sss_client/nss_mc_common.c b/src/sss_client/nss_mc_common.c
index db9be94b442b1b5a97ff9074fb1d98a1feea5e1a..cd1ac42da66a2546cb8dec1e8e79088883a870a1 100644
--- a/src/sss_client/nss_mc_common.c
+++ b/src/sss_client/nss_mc_common.c
@@ -31,6 +31,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include "nss_mc.h"
+#include "sss_cli.h"
 #include "util/io.h"
 
 /* FIXME: hook up to library destructor to avoid leaks */
@@ -101,18 +102,15 @@ errno_t sss_nss_check_header(struct sss_cli_mc_ctx *ctx)
     return 0;
 }
 
-errno_t sss_nss_mc_get_ctx(const char *name, struct sss_cli_mc_ctx *ctx)
+static errno_t sss_nss_mc_init_ctx(const char *name,
+                                   struct sss_cli_mc_ctx *ctx)
 {
     struct stat fdstat;
     char *file = NULL;
-    char *envval;
     int ret;
 
-    envval = getenv("SSS_NSS_USE_MEMCACHE");
-    if (envval && strcasecmp(envval, "NO") == 0) {
-        return EPERM;
-    }
-
+    sss_nss_lock();
+    /* check if ctx is initialised by previous thread. */
     if (ctx->initialized) {
         ret = sss_nss_check_header(ctx);
         goto done;
@@ -168,6 +166,38 @@ done:
         memset(ctx, 0, sizeof(struct sss_cli_mc_ctx));
     }
     free(file);
+    sss_nss_unlock();
+
+    return ret;
+}
+
+errno_t sss_nss_mc_get_ctx(const char *name, struct sss_cli_mc_ctx *ctx)
+{
+    char *envval;
+    int ret;
+
+    envval = getenv("SSS_NSS_USE_MEMCACHE");
+    if (envval && strcasecmp(envval, "NO") == 0) {
+        return EPERM;
+    }
+
+    if (ctx->initialized) {
+        ret = sss_nss_check_header(ctx);
+        goto done;
+    }
+
+    ret = sss_nss_mc_init_ctx(name, ctx);
+
+done:
+    if (ret) {
+        if ((ctx->mmap_base != NULL) && (ctx->mmap_size != 0)) {
+            munmap(ctx->mmap_base, ctx->mmap_size);
+        }
+        if (ctx->fd != -1) {
+            close(ctx->fd);
+        }
+        memset(ctx, 0, sizeof(struct sss_cli_mc_ctx));
+    }
     return ret;
 }
 
-- 
1.9.3

-------------- next part --------------
>From ab9023eee5c9d545dd6a9e4af5154b79a73326ef Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 9 Jul 2014 19:03:30 +0200
Subject: [PATCH 2/3] sss_client: Fix memory leak in nss_mc_{group,passwd}

Memory leak can happen with long living clients where there are records with
colliding hashes; usually LDAP servers with many  users or groups.

Function sss_nss_mc_get_record allocates memory that is stored into "rec",
with next iteration variable rec is overriden with new record and old
one is lost and cannot be freed.

Example code flow:
src/sss_client/nss_mc_group.c:133: alloc_arg: "sss_nss_mc_get_record" allocates memory that is stored into "rec".
src/sss_client/nss_mc_common.c:216:13: alloc_fn: Storage is returned from allocation function "malloc".
src/sss_client/nss_mc_common.c:216:13: var_assign: Assigning: "copy_rec" = "malloc(rec_len)".
src/sss_client/nss_mc_common.c:225:9: noescape: Resource "copy_rec" is not freed or pointed-to in function "memcpy". [Note: The source code implementation of the function has been overridden by a builtin model.]
src/sss_client/nss_mc_common.c:239:5: var_assign: Assigning: "*_rec" = "copy_rec".
src/sss_client/nss_mc_group.c:163: noescape: Resource "rec" is not freed or pointed-to in "sss_nss_mc_next_slot_with_hash".
src/sss_client/nss_mc_common.c:294:60: noescape: "sss_nss_mc_next_slot_with_hash(struct sss_mc_rec *, uint32_t)" does not free or save its pointer parameter "rec".
src/sss_client/nss_mc_group.c:133: overwrite_var: Overwriting "rec" in call to "sss_nss_mc_get_record" leaks the storage that "rec" points to.
src/sss_client/nss_mc_common.c:239:5: write_notnull_to_parm: Assigning: "*_rec" = "copy_rec".
---
 src/sss_client/nss_mc_group.c  | 8 ++++++++
 src/sss_client/nss_mc_passwd.c | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index 5af55468f1a6fb2847844f2131cad6e2f83588b4..268b40ef02f2a621c4f61755ce4dfe2c3786bfa6 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -130,6 +130,10 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t name_len,
      * it's value is not MC_INVALID_VAL, then the cache is
      * probbably corrupted. */
     while (MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) {
+        /* free record from previous iteration */
+        free(rec);
+        rec = NULL;
+
         ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec);
         if (ret) {
             goto done;
@@ -205,6 +209,10 @@ errno_t sss_nss_mc_getgrgid(gid_t gid,
      * it's value is not MC_INVALID_VAL, then the cache is
      * probbably corrupted. */
     while (MC_SLOT_WITHIN_BOUNDS(slot, gr_mc_ctx.dt_size)) {
+        /* free record from previous iteration */
+        free(rec);
+        rec = NULL;
+
         ret = sss_nss_mc_get_record(&gr_mc_ctx, slot, &rec);
         if (ret) {
             goto done;
diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c
index 95b8a0407bc8650d6354b13823fbb486b8c64839..fa19afc3c0e468430183ed3f13b80e086251ee01 100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -123,6 +123,10 @@ errno_t sss_nss_mc_getpwnam(const char *name, size_t name_len,
      * it's value is not MC_INVALID_VAL, then the cache is
      * probbably corrupted. */
     while (MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) {
+        /* free record from previous iteration */
+        free(rec);
+        rec = NULL;
+
         ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec);
         if (ret) {
             goto done;
@@ -199,6 +203,10 @@ errno_t sss_nss_mc_getpwuid(uid_t uid,
      * it's value is not MC_INVALID_VAL, then the cache is
      * probbably corrupted. */
     while (MC_SLOT_WITHIN_BOUNDS(slot, pw_mc_ctx.dt_size)) {
+        /* free record from previous iteration */
+        free(rec);
+        rec = NULL;
+
         ret = sss_nss_mc_get_record(&pw_mc_ctx, slot, &rec);
         if (ret) {
             goto done;
-- 
1.9.3

-------------- next part --------------
>From 2bc6154ba18266fe834cb7dac28d68b2babf918f Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Mon, 14 Jul 2014 15:52:15 +0200
Subject: [PATCH 3/3] DO NOT PUSH ME - just for easier review

---
 src/sss_client/nss_mc_common.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/sss_client/nss_mc_common.c b/src/sss_client/nss_mc_common.c
index cd1ac42da66a2546cb8dec1e8e79088883a870a1..9cdc0343d3168dd341c74746f55b657c345de3ae 100644
--- a/src/sss_client/nss_mc_common.c
+++ b/src/sss_client/nss_mc_common.c
@@ -73,12 +73,14 @@ errno_t sss_nss_check_header(struct sss_cli_mc_ctx *ctx)
     }
     if (count == 0) {
         /* couldn't successfully read header we have to give up */
+        fprintf(stderr, "couldn't successfully read header we must give up\n");
         return EIO;
     }
 
     if (h.major_vno != SSS_MC_MAJOR_VNO ||
         h.minor_vno != SSS_MC_MINOR_VNO ||
         h.status == SSS_MC_HEADER_RECYCLED) {
+        fprintf(stderr, "header versions do not match or header is recycled\n");
         return EINVAL;
     }
 
@@ -95,6 +97,7 @@ errno_t sss_nss_check_header(struct sss_cli_mc_ctx *ctx)
             ctx->hash_table != MC_PTR_ADD(ctx->mmap_base, h.hash_table) ||
             ctx->dt_size != h.dt_size ||
             ctx->ht_size != h.ht_size) {
+            fprintf(stderr, "seed or table size do not match\n");
             return EINVAL;
         }
     }
@@ -109,32 +112,41 @@ static errno_t sss_nss_mc_init_ctx(const char *name,
     char *file = NULL;
     int ret;
 
+    fprintf(stderr, "before nss lock\n");
     sss_nss_lock();
+    fprintf(stderr, "after nss lock\n");
     /* check if ctx is initialised by previous thread. */
     if (ctx->initialized) {
+        fprintf(stderr, "already initialised\n");
         ret = sss_nss_check_header(ctx);
+        if (ret)
+            fprintf(stderr, "sss_nss_check_header beg:%s\n", strerror(ret));
         goto done;
     }
 
     ret = asprintf(&file, "%s/%s", SSS_NSS_MCACHE_DIR, name);
     if (ret == -1) {
         ret = ENOMEM;
+        perror("asprintf\n");
         goto done;
     }
 
     ctx->fd = sss_open_cloexec(file, O_RDONLY, &ret);
     if (ctx->fd == -1) {
+        perror("sss_open_cloexec");
         goto done;
     }
 
     ret = fstat(ctx->fd, &fdstat);
     if (ret == -1) {
         ret = EIO;
+        perror("stat");
         goto done;
     }
 
     if (fdstat.st_size < MC_HEADER_SIZE) {
         ret = ENOMEM;
+        fprintf(stderr, "fdstat.st_size < MC_HEADER_SIZE\n");
         goto done;
     }
     ctx->mmap_size = fdstat.st_size;
@@ -143,15 +155,18 @@ static errno_t sss_nss_mc_init_ctx(const char *name,
                           PROT_READ, MAP_SHARED, ctx->fd, 0);
     if (ctx->mmap_base == MAP_FAILED) {
         ret = ENOMEM;
+        perror("mmap");
         goto done;
     }
 
     ret = sss_nss_check_header(ctx);
     if (ret != 0) {
+        fprintf(stderr, "sss_nss_check_header end:%s\n", strerror(ret));
         goto done;
     }
 
     ctx->initialized = true;
+    fprintf(stderr, "init.done\n");
 
     ret = 0;
 
@@ -159,14 +174,17 @@ done:
     if (ret) {
         if ((ctx->mmap_base != NULL) && (ctx->mmap_size != 0)) {
             munmap(ctx->mmap_base, ctx->mmap_size);
+            fprintf(stderr, "calling munmap\n");
         }
         if (ctx->fd != -1) {
             close(ctx->fd);
+            fprintf(stderr, "calling close\n");
         }
         memset(ctx, 0, sizeof(struct sss_cli_mc_ctx));
     }
     free(file);
     sss_nss_unlock();
+    fprintf(stderr, "after nss unlock\n");
 
     return ret;
 }
@@ -183,6 +201,8 @@ errno_t sss_nss_mc_get_ctx(const char *name, struct sss_cli_mc_ctx *ctx)
 
     if (ctx->initialized) {
         ret = sss_nss_check_header(ctx);
+        if (ret)
+            fprintf(stderr, "sss_nss_check_header:%s\n", strerror(ret));
         goto done;
     }
 
@@ -192,9 +212,11 @@ done:
     if (ret) {
         if ((ctx->mmap_base != NULL) && (ctx->mmap_size != 0)) {
             munmap(ctx->mmap_base, ctx->mmap_size);
+            fprintf(stderr, "calling munmap 2\n");
         }
         if (ctx->fd != -1) {
             close(ctx->fd);
+            fprintf(stderr, "calling munmap 2\n");
         }
         memset(ctx, 0, sizeof(struct sss_cli_mc_ctx));
     }
-- 
1.9.3



More information about the sssd-devel mailing list