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)