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.
How to test?
You can use simple program form ticket description #2380.
Program call getuid, therefore user should be from sssd and not /etc/passwd.
It needn't crash at first time, but you can use simple for loop
for i in {1..100}; do ./a1; done
You can also use 3th patch to see code flow. e.g.
bash-4.2$ ./a1
more [2] threads try to init mem ctc
init.done
seed or table size do not match
sss_nss_check_header end:Invalid argument
more [3] threads try to init mem ctc
calling munmap
calling close
Segmentation fault (core dumped)
bash-4.2$ ./a1
init.done
more [2] threads try to init mem ctc
more [3] threads try to init mem ctc
seed or table size do not match
sss_nss_check_header end:Invalid argument
seed or table size do not match
seed or table size do not match
calling munmap
calling close
sss_nss_check_header beg:Invalid argument
calling close
init.done
Floating point exception (core dumped)
If you apply 3rd patch you can simulate old version with simple diff
diff --git a/src/sss_client/nss_mc_common.c b/src/sss_client/nss_mc_common.c
index 768763f..cea3a78 100644
--- a/src/sss_client/nss_mc_common.c
+++ b/src/sss_client/nss_mc_common.c
@@ -131,7 +131,7 @@ errno_t sss_nss_mc_get_ctx(const char *name, struct
sss_cli_mc_ctx *ctx)
*/
fprintf(stderr, "more [%d] threads try to init mem ctc\n",
ctx->init_count);
- return EAGAIN;
+ //return EAGAIN;
}
ret = asprintf(&file, "%s/%s", SSS_NSS_MCACHE_DIR, name);
LS
>From 79e23d5ad87ec454fe2f3f2b7e79694897db10bd Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Mon, 14 Jul 2014 16:02:22 +0200
Subject: [PATCH 1/3] sss_client: thread safe initialisation of
sss_nss_mc_get_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)
This patch does not allow to parallel initialisation.
Resolves:
https://fedorahosted.org/sssd/ticket/2380
---
src/sss_client/nss_mc.h | 1 +
src/sss_client/nss_mc_common.c | 10 ++++++++++
src/sss_client/nss_mc_group.c | 3 ++-
src/sss_client/nss_mc_passwd.c | 3 ++-
4 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/src/sss_client/nss_mc.h b/src/sss_client/nss_mc.h
index
685cc41c0530750d890050f0917dc88be14d96ea..a77b86261bfbafc83b0e65315ec478f7f40b57e8
100644
--- a/src/sss_client/nss_mc.h
+++ b/src/sss_client/nss_mc.h
@@ -37,6 +37,7 @@ typedef int errno_t;
struct sss_cli_mc_ctx {
bool initialized;
int fd;
+ volatile int init_count; /* numbers of threads trying to initialize ctx */
uint32_t seed; /* seed from the tables header */
diff --git a/src/sss_client/nss_mc_common.c b/src/sss_client/nss_mc_common.c
index
db9be94b442b1b5a97ff9074fb1d98a1feea5e1a..d918b697f9cf159e6dbea46ea0f670993dc14bbf
100644
--- a/src/sss_client/nss_mc_common.c
+++ b/src/sss_client/nss_mc_common.c
@@ -118,6 +118,15 @@ errno_t sss_nss_mc_get_ctx(const char *name, struct
sss_cli_mc_ctx *ctx)
goto done;
}
+ if (__sync_add_and_fetch(&ctx->init_count, 1) != 1) {
+ /*
+ * More threads try to init sss_cli_mc_ctx. There can be raise
+ * condition and we do not want to call clean up in case of error
+ * in done section. Therefore return EAGAIN is used.
+ */
+ return EAGAIN;
+ }
+
ret = asprintf(&file, "%s/%s", SSS_NSS_MCACHE_DIR, name);
if (ret == -1) {
ret = ENOMEM;
@@ -154,6 +163,7 @@ errno_t sss_nss_mc_get_ctx(const char *name, struct
sss_cli_mc_ctx *ctx)
}
ctx->initialized = true;
+ ctx->init_count = 0;
ret = 0;
diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index
5af55468f1a6fb2847844f2131cad6e2f83588b4..7a3788e1d853c197661b220b67434f2c2275c9a8
100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -29,7 +29,8 @@
#include "nss_mc.h"
#include "util/util_safealign.h"
-struct sss_cli_mc_ctx gr_mc_ctx = { false, -1, 0, NULL, 0, NULL, 0, NULL, 0 };
+struct sss_cli_mc_ctx gr_mc_ctx = { false, -1, 0,
+ 0, NULL, 0, NULL, 0, NULL, 0 };
static errno_t sss_nss_mc_parse_result(struct sss_mc_rec *rec,
struct group *result,
diff --git a/src/sss_client/nss_mc_passwd.c b/src/sss_client/nss_mc_passwd.c
index
95b8a0407bc8650d6354b13823fbb486b8c64839..f2970eefa3f52601eca3836280aa24173def296c
100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -28,7 +28,8 @@
#include <time.h>
#include "nss_mc.h"
-struct sss_cli_mc_ctx pw_mc_ctx = { false, -1, 0, NULL, 0, NULL, 0, NULL, 0 };
+struct sss_cli_mc_ctx pw_mc_ctx = { false, -1, 0,
+ 0, NULL, 0, NULL, 0, NULL, 0 };
static errno_t sss_nss_mc_parse_result(struct sss_mc_rec *rec,
struct passwd *result,
--
1.9.3
>From 871446aa8787892e91a4e6240884a7361087c83a Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@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
7a3788e1d853c197661b220b67434f2c2275c9a8..d3f3f247348c51a59174c5f30bc8704962b3de57
100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -131,6 +131,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;
@@ -206,6 +210,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
f2970eefa3f52601eca3836280aa24173def296c..5845d044b34348fa4428435ec5b962a520148135
100644
--- a/src/sss_client/nss_mc_passwd.c
+++ b/src/sss_client/nss_mc_passwd.c
@@ -124,6 +124,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;
@@ -200,6 +204,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
>From 2b9625f0b38bedc41164d058051746fcf146bf60 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@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 | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/src/sss_client/nss_mc_common.c b/src/sss_client/nss_mc_common.c
index
d918b697f9cf159e6dbea46ea0f670993dc14bbf..768763fcc1fd45c648a5d20a414a42e55624a925
100644
--- a/src/sss_client/nss_mc_common.c
+++ b/src/sss_client/nss_mc_common.c
@@ -72,12 +72,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;
}
@@ -94,6 +96,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;
}
}
@@ -115,6 +118,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 beg:%s\n", strerror(ret));
goto done;
}
@@ -124,28 +129,34 @@ errno_t sss_nss_mc_get_ctx(const char *name, struct
sss_cli_mc_ctx *ctx)
* condition and we do not want to call clean up in case of error
* in done section. Therefore return EAGAIN is used.
*/
+ fprintf(stderr, "more [%d] threads try to init mem ctc\n",
+ ctx->init_count);
return EAGAIN;
}
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;
@@ -154,16 +165,19 @@ errno_t sss_nss_mc_get_ctx(const char *name, struct
sss_cli_mc_ctx *ctx)
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;
ctx->init_count = 0;
+ fprintf(stderr, "init.done\n");
ret = 0;
@@ -171,9 +185,11 @@ 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));
}
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
