On (27/07/15 17:19), Michal Židek wrote: >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<lsleb...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
