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

Lukas Slebodnik lslebodn at redhat.com
Wed Jul 29 15:01:08 UTC 2015


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<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.
>
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



More information about the sssd-devel mailing list