On Thu, 2012-12-20 at 17:03 +0100, Pavel Březina wrote:
> On 12/20/2012 04:41 PM, Simo Sorce wrote:
> > On Thu, 2012-12-20 at 16:14 +0100, Pavel Březina wrote:
> >> On 12/20/2012 02:20 PM, Simo Sorce wrote:
> >>> On Thu, 2012-12-20 at 12:29 +0100, Pavel Březina wrote:
> >>>> On 12/20/2012 06:10 AM, Simo Sorce wrote:
> >>>>> -static errno_t sss_mc_get_record(struct sss_mc_ctx *mcc,
> >>>>> +static errno_t sss_mc_get_record(struct sss_mc_ctx **_mcc,
> >>>>> size_t rec_len,
> >>>>> struct sized_string *key,
> >>>>> struct sss_mc_rec **_rec)
> >>>>> {
> >>>>> + struct sss_mc_ctx *mcc = *_mcc;
> >>>>> struct sss_mc_rec *old_rec = NULL;
> >>>>> struct sss_mc_rec *rec;
> >>>>> int old_slots;
> >>>>> @@ -424,6 +425,16 @@ static errno_t sss_mc_get_record(struct sss_mc_ctx
> >>>>> *mcc,
> >>>>> /* we are going to use more space, find enough free slots */
> >>>>> ret = sss_mc_find_free_slots(mcc, num_slots, &base_slot);
> >>>>> if (ret != EOK) {
> >>>>> + if (ret == EFAULT) {
> >>>>> + TALLOC_CTX *parent;
> >>>>> + DEBUG(SSSDBG_CRIT_FAILURE,
> >>>>> + ("Fatal internal mmap cache error, invalidating
> >>>>> cache!\n"));
> >>>>> + parent = talloc_parent(mcc);
> >>>>> + if (parent == NULL) {
> >>>>> + talloc_zfree(*_mcc);
> >>>>> + }
> >>>>> + (void)sss_mmap_cache_reinit(parent, -1, -1, _mcc);
> >>>>> + }
> >>>>> return ret;
> >>>>> }
> >>>>
> >>>> Hi,
> >>>>
> >>>> Is if (parent == NULL) only precaution or can it actually happen?
> >>>
> >>> Only a precaution, all the callers use an actual memory context.
> >>>
> >>>> If you talloc_zfree(*_mcc) then you will hit EINVAL in reinit() and the
> >>>> cache won't be reinitialized. Is this intentional? If so, can you
> >>>> comment it in the code?
> >>>
> >>> Well it was not intentional I meant to return after the free.
> >>> However I am wondering if we should really care ...
> >>>
> >>> If the caller had NULL as the parent something may have gone wrong, but
> >>> at worst we will leak a few bytes... maybe I shouldn't consider a NULL
> >>> parent as an error after all. Opinions ?
> >>>
> >>> Simo.
> >>>
> >>
> >> Does the memory cache require a parent context for some reason (do you
> >> manipulate with the parent within the cache)?
> >>
> >> If not I think you should not take care whether is has a parent or not.
> >> For what we know, it may be intentional by the caller. Otherwise you
> >> should check if mem_ctx != NULL in init().
> >>
> >> Looking into the init() function I found out that you are using wrong
> >> context:
> >>
> >> mc_ctx->name = talloc_strdup(mem_ctx, name);
> >> if (!mc_ctx->name) {
> >> ret = ENOMEM;
> >> goto done;
> >> }
> >>
> >> It should say mc_ctx not mem_ctx.
> >
> > Nice catch.
> >
> > I also realized that the reinit() function uses talloc_free() instead of
> > talloc_zfree() on the mc_ctx which would leave a wild pointer to freed
> > memory in the caller should the init() fail.
> >
> > So in the attached patch I fixed all this and removed any check about
> > the talloc_parent(), we just trust it returns the correct value and the
> > caller did a sensible thing with the first init()
> >
> > Simo.
>
> Nack.
>
> src/responder/nss/nsssrv_mmap_cache.c: In function ‘sss_mmap_cache_reinit’:
> src/responder/nss/nsssrv_mmap_cache.c:1051:11: error: expected
> expression before ‘do’
>
> talloc_zfree() doesn't return value (it's inside do...while)
Ouch.
Sorry.
Amended patch attached.
--
Simo Sorce * Red Hat, Inc * New York
>From fef0ead5e8303959987aa413646e4f5d90b51eb2 Mon Sep 17 00:00:00 2001
From: Simo Sorce <[email protected]>
Date: Wed, 19 Dec 2012 21:17:40 -0500
Subject: [PATCH] mmap cache: invalidate cache on fatal error
If a fatal EFAULT error is returned by the internal function that frees used
memory invalidate the whole cache and reinit it. This way we avoid further
corruption and insure clients see consistent data.
Also insure we use the right context in init() and we use talloc_zfree() in
reinit so that if the init() later fails we do not leave around a pointer
to free memory in the callers.
---
src/responder/nss/nsssrv_cmd.c | 4 ++--
src/responder/nss/nsssrv_mmap_cache.c | 32 ++++++++++++++++++++++++++------
src/responder/nss/nsssrv_mmap_cache.h | 4 ++--
3 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 27cbedcbe252ca01570face16d4915d507ef1b99..2ad9194c5d8bdf29def477beb821bcdfe84f8037 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -432,7 +432,7 @@ static int fill_pwent(struct sss_packet *packet,
num++;
if (pw_mmap_cache && nctx->pwd_mc_ctx) {
- ret = sss_mmap_cache_pw_store(nctx->pwd_mc_ctx,
+ ret = sss_mmap_cache_pw_store(&nctx->pwd_mc_ctx,
&fullname, &pwfield,
uid, gid,
&gecos, &homedir, &shell);
@@ -2185,7 +2185,7 @@ static int fill_grent(struct sss_packet *packet,
/* body was reallocated, so fullname might be pointing to
* where body used to be, not where it is */
to_sized_string(&fullname, (const char *)&body[rzero+STRS_ROFFSET]);
- ret = sss_mmap_cache_gr_store(nctx->grp_mc_ctx,
+ ret = sss_mmap_cache_gr_store(&nctx->grp_mc_ctx,
&fullname, &pwfield, gid, memnum,
(char *)&body[rzero] + STRS_ROFFSET +
fullname.len + pwfield.len,
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
index d3852c67d1cd637cbcff81bf55c59fee1d39e2c8..7149ca80533f72f2592deeed600974b98f14f33b 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -392,11 +392,12 @@ static struct sss_mc_rec *sss_mc_find_record(struct sss_mc_ctx *mcc,
return rec;
}
-static errno_t sss_mc_get_record(struct sss_mc_ctx *mcc,
+static errno_t sss_mc_get_record(struct sss_mc_ctx **_mcc,
size_t rec_len,
struct sized_string *key,
struct sss_mc_rec **_rec)
{
+ struct sss_mc_ctx *mcc = *_mcc;
struct sss_mc_rec *old_rec = NULL;
struct sss_mc_rec *rec;
int old_slots;
@@ -424,6 +425,11 @@ static errno_t sss_mc_get_record(struct sss_mc_ctx *mcc,
/* we are going to use more space, find enough free slots */
ret = sss_mc_find_free_slots(mcc, num_slots, &base_slot);
if (ret != EOK) {
+ if (ret == EFAULT) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Fatal internal mmap cache error, invalidating cache!\n"));
+ (void)sss_mmap_cache_reinit(talloc_parent(mcc), -1, -1, _mcc);
+ }
return ret;
}
@@ -494,7 +500,7 @@ static errno_t sss_mmap_cache_invalidate(struct sss_mc_ctx *mcc,
* passwd map
***************************************************************************/
-errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx *mcc,
+errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx **_mcc,
struct sized_string *name,
struct sized_string *pw,
uid_t uid, gid_t gid,
@@ -502,6 +508,7 @@ errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx *mcc,
struct sized_string *homedir,
struct sized_string *shell)
{
+ struct sss_mc_ctx *mcc = *_mcc;
struct sss_mc_rec *rec;
struct sss_mc_pwd_data *data;
struct sized_string uidkey;
@@ -530,7 +537,7 @@ errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx *mcc,
return ENOMEM;
}
- ret = sss_mc_get_record(mcc, rec_len, name, &rec);
+ ret = sss_mc_get_record(_mcc, rec_len, name, &rec);
if (ret != EOK) {
return ret;
}
@@ -630,12 +637,13 @@ done:
* group map
***************************************************************************/
-int sss_mmap_cache_gr_store(struct sss_mc_ctx *mcc,
+int sss_mmap_cache_gr_store(struct sss_mc_ctx **_mcc,
struct sized_string *name,
struct sized_string *pw,
gid_t gid, size_t memnum,
char *membuf, size_t memsize)
{
+ struct sss_mc_ctx *mcc = *_mcc;
struct sss_mc_rec *rec;
struct sss_mc_grp_data *data;
struct sized_string gidkey;
@@ -664,7 +672,7 @@ int sss_mmap_cache_gr_store(struct sss_mc_ctx *mcc,
return ENOMEM;
}
- ret = sss_mc_get_record(mcc, rec_len, name, &rec);
+ ret = sss_mc_get_record(_mcc, rec_len, name, &rec);
if (ret != EOK) {
return ret;
}
@@ -916,7 +924,7 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
}
mc_ctx->fd = -1;
- mc_ctx->name = talloc_strdup(mem_ctx, name);
+ mc_ctx->name = talloc_strdup(mc_ctx, name);
if (!mc_ctx->name) {
ret = ENOMEM;
goto done;
@@ -1043,6 +1051,15 @@ errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, size_t n_elem,
}
type = (*mc_ctx)->type;
+
+ if (n_elem == (size_t)-1) {
+ n_elem = (*mc_ctx)->ft_size * 8;
+ }
+
+ if (timeout == (time_t)-1) {
+ timeout = (*mc_ctx)->valid_time_slot;
+ }
+
ret = talloc_free(*mc_ctx);
if (ret != 0) {
/* This can happen only if destructor is associated with this
@@ -1051,6 +1068,9 @@ errno_t sss_mmap_cache_reinit(TALLOC_CTX *mem_ctx, size_t n_elem,
" context failed.\n"));
}
+ /* make sure we do not leave a potentially freed pointer around */
+ *mc_ctx = NULL;
+
ret = sss_mmap_cache_init(mem_ctx, name, type, n_elem, timeout, mc_ctx);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to re-initialize mmap cache.\n"));
diff --git a/src/responder/nss/nsssrv_mmap_cache.h b/src/responder/nss/nsssrv_mmap_cache.h
index 0da637970de91b0dec7496523f1511cee4fdbaab..25cec40cc26d6732c1465c014ab5aab4a59ec906 100644
--- a/src/responder/nss/nsssrv_mmap_cache.h
+++ b/src/responder/nss/nsssrv_mmap_cache.h
@@ -36,7 +36,7 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx, const char *name,
enum sss_mc_type type, size_t n_elem,
time_t valid_time, struct sss_mc_ctx **mcc);
-errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx *mcc,
+errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx **_mcc,
struct sized_string *name,
struct sized_string *pw,
uid_t uid, gid_t gid,
@@ -44,7 +44,7 @@ errno_t sss_mmap_cache_pw_store(struct sss_mc_ctx *mcc,
struct sized_string *homedir,
struct sized_string *shell);
-errno_t sss_mmap_cache_gr_store(struct sss_mc_ctx *mcc,
+errno_t sss_mmap_cache_gr_store(struct sss_mc_ctx **_mcc,
struct sized_string *name,
struct sized_string *pw,
gid_t gid, size_t memnum,
--
1.8.0.1
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
