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(a)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