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(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.
No,
because in future there could be added another strings
and I do not want to rename it later from names -> strs
I'm sorry but "naming" is really difficult task.
LS