[SSSD] [PATCH 3/3] mmap cache: invalidate cache on fatal error

Simo Sorce simo at redhat.com
Thu Dec 20 15:41:16 UTC 2012


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.

-- 
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-1-mmap-cache-invalidate-cache-on-fatal-error.patch
Type: text/x-patch
Size: 7466 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121220/cb87db1a/attachment.bin>


More information about the sssd-devel mailing list