[SSSD] [PATCH] mmap_cache: "Override" functions for initgr mmap cache

Michal Židek mzidek at redhat.com
Mon Jul 27 15:19:29 UTC 2015


On 07/24/2015 04:57 PM, Lukas Slebodnik wrote:
> On (24/07/15 15:36), Michal Židek wrote:
>> On 07/24/2015 03:21 PM, Lukas Slebodnik wrote:
>>> On (17/07/15 10:16), Lukas Slebodnik wrote:
>>>> On (16/07/15 19:19), Michal Židek wrote:
>>>>> On 07/16/2015 05:07 PM, Lukas Slebodnik wrote:
>>>>>> On (16/07/15 13:46), Michal Židek wrote:
>>>>>>> On 07/15/2015 06:42 PM, Lukas Slebodnik wrote:
>>>>>>>> ehlo,
>>>>>>>>
>>>>>>>> reproducer:
>>>>>>>>    add user and few groups to ldap
>>>>>>>>    call id user
>>>>>>>>    remove one group
>>>>>>>>    authenticate
>>>>>>>>    call id user // removed groups should not appear in output
>>>>>>>>
>>>>>>>> LS
>>>>>>>>
>>>>>>>>
>>>>>>>> 0001-mmap_cache-Override-functions-for-initgr-mmap-cache.patch
>>>>>>>>
>>>>>>>>
>>>>>>>>  From 146337d5a3a15236b0a8c26e10d233e8cbc3a1d8 Mon Sep 17 00:00:00 2001
>>>>>>>> From: Lukas Slebodnik<lslebodn at redhat.com>
>>>>>>>> Date: Wed, 15 Jul 2015 17:47:29 +0200
>>>>>>>> Subject: [PATCH] mmap_cache: "Override" functions for initgr mmap cache
>>>>>>>>
>>>>>>>> Functions sss_mc_get_strs_offset and sss_mc_get_strs_len provides
>>>>>>>> data about strings for individual memory caches (passwd, ...)
>>>>>>>> Their are used in generic responder mmap cache code to find a record
>>>>>>>> in mmap cache (sss_mc_find_record). Data provided from functions sss_mc_get_*
>>>>>>>> are used for checking the validity of record. So in case of corrupted record
>>>>>>>> the whole mmap cache can be invalidated.
>>>>>>>>
>>>>>>>> Functions sss_mc_get_strs_offset and sss_mc_get_strs_len did not provide
>>>>>>>> data for initgroups mmap cache and therefore particular record could not be
>>>>>>>> invalidated.
>>>>>>>>
>>>>>>>> Resolves:
>>>>>>>> https://fedorahosted.org/sssd/ticket/2716
>>>>>>>> ---
>>>>>>>>   src/responder/nss/nsssrv_mmap_cache.c | 15 ++++++++++++---
>>>>>>>>   src/sss_client/nss_mc_initgr.c        | 16 +++++++++++-----
>>>>>>>>   src/util/mmap_cache.h                 |  6 +++++-
>>>>>>>>   3 files changed, 28 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
>>>>>>>> index ebda8ac6fab3dd87f5a1d8e43717bf7a5b5a9878..6251ff93ce7f8bf473fc770f7aa3d2e0642395a3 100644
>>>>>>>> --- a/src/responder/nss/nsssrv_mmap_cache.c
>>>>>>>> +++ b/src/responder/nss/nsssrv_mmap_cache.c
>>>>>>>> @@ -475,6 +475,9 @@ static errno_t sss_mc_get_strs_offset(struct sss_mc_ctx *mcc,
>>>>>>>>       case SSS_MC_GROUP:
>>>>>>>>           *_offset = offsetof(struct sss_mc_grp_data, strs);
>>>>>>>>           return EOK;
>>>>>>>> +    case SSS_MC_INITGROUPS:
>>>>>>>> +        *_offset = offsetof(struct sss_mc_initgr_data, gids);
>>>>>>>> +        return EOK;
>>>>>>>>       default:
>>>>>>>>           DEBUG(SSSDBG_FATAL_FAILURE, "Unknown memory cache type.\n");
>>>>>>>>           return EINVAL;
>>>>>>>> @@ -492,6 +495,9 @@ static errno_t sss_mc_get_strs_len(struct sss_mc_ctx *mcc,
>>>>>>>>       case SSS_MC_GROUP:
>>>>>>>>           *_len = ((struct sss_mc_grp_data *)&rec->data)->strs_len;
>>>>>>>>           return EOK;
>>>>>>>> +    case SSS_MC_INITGROUPS:
>>>>>>>> +        *_len = ((struct sss_mc_initgr_data *)&rec->data)->data_len;
>>>>>>>> +        return EOK;
>>>>>>>>       default:
>>>>>>>>           DEBUG(SSSDBG_FATAL_FAILURE, "Unknown memory cache type.\n");
>>>>>>>>           return EINVAL;
>>>>>>>> @@ -974,8 +980,8 @@ errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx **_mcc,
>>>>>>>>           return EINVAL;
>>>>>>>>       }
>>>>>>>>
>>>>>>>> -    /* memnum + reserved + array of members + name*/
>>>>>>>> -    data_len = (2 + memnum) * sizeof(uint32_t) + name->len;
>>>>>>>> +    /* array of members + name */
>>>>>>>> +    data_len = memnum * sizeof(uint32_t) + name->len;
>>>>>>>
>>>>>>> It is actually not array of members but array of group ids
>>>>>>> the user is member of. It would be good to rename the
>>>>>>> memnum to num_groups or something similiar.
>>>>>>>
>>>>>>>>       rec_len = sizeof(struct sss_mc_rec) + sizeof(struct sss_mc_initgr_data)
>>>>>>>>                 + data_len;
>>>>>>>>       if (rec_len > mcc->dt_size) {
>>>>>>>> @@ -998,10 +1004,13 @@ errno_t sss_mmap_cache_initgr_store(struct sss_mc_ctx **_mcc,
>>>>>>>>                               name->str, name->len, name->str, name->len);
>>>>>>>>
>>>>>>>>       /* initgroups struct */
>>>>>>>> +    data->strs_len = name->len;
>>>>>>>> +    data->data_len = data_len;
>>>>>>>> +    data->reserved = MC_INVALID_VAL32;
>>>>>>>>       data->members = memnum;
>>>>>>>         ^^^^^^^^^^^^^   ^^^^^^
>>>>>>> Same as above.
>>>>>>>
>>>>>>>>       memcpy(data->gids, membuf, memnum * sizeof(uint32_t));
>>>>>>>                            ^^^^^^  ^^^^^^
>>>>>>> gids_buf
>>>>>>>
>>>>>>>>       memcpy(&data->gids[memnum], name->str, name->len);
>>>>>>>> -    data->name = MC_PTR_DIFF(&data->gids[memnum], data);
>>>>>>>                                             ^^^^^^
>>>>>>>> +    data->strs = data->name = MC_PTR_DIFF(&data->gids[memnum], data);
>>>>>>>>
>>>>>>>>       MC_LOWER_BARRIER(rec);
>>>>>>>>
>>>>>>>> diff --git a/src/sss_client/nss_mc_initgr.c b/src/sss_client/nss_mc_initgr.c
>>>>>>>> index bfb09d6550c310fbab254dc9b3ab7b306b7d3f06..0ccde2130526786e1949287cde1e4c1bc4596670 100644
>>>>>>>> --- a/src/sss_client/nss_mc_initgr.c
>>>>>>>> +++ b/src/sss_client/nss_mc_initgr.c
>>>>>>>> @@ -93,6 +93,7 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,
>>>>>>>>       uint32_t hash;
>>>>>>>>       uint32_t slot;
>>>>>>>>       int ret;
>>>>>>>> +    const size_t data_offset = offsetof(struct sss_mc_initgr_data, gids);
>>>>>>>>       uint8_t *max_addr;
>>>>>>>>
>>>>>>>>       ret = sss_nss_mc_get_ctx("initgroups", &initgr_mc_ctx);
>>>>>>>> @@ -128,16 +129,21 @@ errno_t sss_nss_mc_initgroups_dyn(const char *name, size_t name_len,
>>>>>>>>           }
>>>>>>>>
>>>>>>>>           data = (struct sss_mc_initgr_data *)rec->data;
>>>>>>>> +        rec_name = (char *)data + data->name;
>>>>>>>>           /* Integrity check
>>>>>>>> -         * - array with gids must be within data_table
>>>>>>>> -         * - string must be within data_table */
>>>>>>>> -        if ((uint8_t *)data->gids > max_addr
>>>>>>>> -                || (uint8_t *)data + data->name + name_len > max_addr) {
>>>>>>>> +         * - name_len cannot be longer than all strings or data
>>>>>>>> +         * - data->name cannot point outside strings
>>>>>>>> +         * - all data must be within data_table
>>>>>>>> +         * - name must be within data_table */
>>>>>>>> +        if (name_len > data->data_len
>>>>>>>> +            || name_len > data->strs_len
>>>>>>>> +            || (data->name + name_len) > (data_offset + data->data_len)
>>>>>>>> +            || (uint8_t *)data->gids + data->data_len > max_addr
>>>>>>>> +            || (uint8_t *)rec_name + name_len > max_addr) {
>>>>>>>>               ret = ENOENT;
>>>>>>>>               goto done;
>>>>>>>>           }
>>>>>>>>
>>>>>>>> -        rec_name = (char *)data + data->name;
>>>>>>>>           if (strcmp(name, rec_name) == 0) {
>>>>>>>>               break;
>>>>>>>>           }
>>>>>>>> diff --git a/src/util/mmap_cache.h b/src/util/mmap_cache.h
>>>>>>>> index 438e28a3d217041278fc1bb60aa553d098516035..6cd1e46218fc2a814b34200f0837ee7fff382378 100644
>>>>>>>> --- a/src/util/mmap_cache.h
>>>>>>>> +++ b/src/util/mmap_cache.h
>>>>>>>> @@ -79,7 +79,7 @@ typedef uint32_t rel_ptr_t;
>>>>>>>>
>>>>>>>>
>>>>>>>>   #define SSS_MC_MAJOR_VNO    1
>>>>>>>> -#define SSS_MC_MINOR_VNO    0
>>>>>>>> +#define SSS_MC_MINOR_VNO    1
>>>>>>>>
>>>>>>>>   #define SSS_MC_HEADER_UNINIT    0   /* after ftruncate or before reset */
>>>>>>>>   #define SSS_MC_HEADER_ALIVE     1   /* current and in use */
>>>>>>>> @@ -139,6 +139,10 @@ struct sss_mc_grp_data {
>>>>>>>>
>>>>>>>>   struct sss_mc_initgr_data {
>>>>>>>>       rel_ptr_t name;         /* ptr to name string, rel. to struct base addr */
>>>>>>>> +    rel_ptr_t strs;         /* ptr to concatenation of all strings */
>>>>>>>        ^^^^^^^^^^^^^^
>>>>>>> This pointer is not needed. The name pointer is enough.
>>>>>>>
>>>>>>>> +    uint32_t strs_len;      /* length of strs */
>>>>>>>                 ^^^^^^^^
>>>>>>> This should be called name_len.
>>>>>>>
>>>>>>>> +    uint32_t data_len;      /* all initgroups data len */
>>>>>>>> +    uint32_t reserved;
>>>>>>>>       uint32_t members;       /* number of members in groups */
>>>>>>>                  ^^^^^^^           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>>> Again not members, number of gids.
>>>>>>>
>>>>>>>>       uint32_t gids[0];       /* array of all groups
>>>>>>>>                                * string with name is stored after gids */
>>>>>>>> -- 2.4.3
>>>>>>>
>>>>>>>
>>>>>>> If you do not want to squash the renaming into the same patch, feel free
>>>>>>> to attach it as separate patch.
>>>>>> Yes, they need to be in separate patch because they are not related
>>>>>> to the regression.
>>>>>>
>>>>>>> I am fine either way. Sorry I should
>>>>>>> have catch this in the first iteration of the initgr memcache patches.
>>>>>>>
>>>>>> I glad the biggest issue in patches was just variable names.
>>>>>>
>>>>>> LS
>>>>>>
>>>>>>
>>>>>
>>>>> I still think we should not use the 'strs' in the sss_mc_initgr_data
>>>>> structure. If we want to have multiple names in it in the future
>>>>> it should be called 'names'. The reason is that the we have
>>>>> getter functions sss_mc_get_strs_offset and sss_mc_get_strs_len
>>>>> but they have nothing to do with the current 'strs' member in
>>>>> the sss_mc_initgr_data which is confusing (they work with the
>>>>> gids here). So either rename the functions to something more generic
>>>>> or do not use the 'strs' name for the member.
>>>>>
>>>>> Other than that the patches look good.
>>>>>
>>>> http://martinfowler.com/bliki/TwoHardThings.html
>>>>
>>>> So please sent patch which rename functions or variables
>>>> and I will sqaush it.
>>>>
>>> Bump,
>>>
>>> I still wait for a patch with names you like.
>>>
>>> LS
>>
>>
>> I thought we are waiting for your patch that does
>> not crash.
>>
> crash is caused by combination of these patches and
> another mmap cache bug (#2712). So patches need to be pushed
> as a patchset.
>
>> I told you I do not like the 'strs' for the reason
>> mentioned above. It is a newly added member in your
>> patch (the already existing strs members are fine),
>> so what should I rename? It is not in master
>> yet, only in your patch. The first patch that renames
>> already existing variables is OK.
>>
>> Good that you bumped the thread, hope the waiting
>> deadlock is now solved :)
>>
> There was not a deadlock. I was working on other issues.
> I do not want to waste my time with making up a name.
> @see http://martinfowler.com/bliki/TwoHardThings.html
>
> So please provide patch with renamed functions.

No functions need to be renamed.
Rename the *newly added* 'strs' member in the sss_mc_initgr_data
to 'names' so that it does not conflict with the sss_mc_get_strs_offset
and sss_mc_get_strs_len functions. That is all I wanted.

Michal

-- 
Senior Principal Intern



More information about the sssd-devel mailing list