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

Lukas Slebodnik lslebodn at redhat.com
Fri Jul 17 08:16:48 UTC 2015


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.

LS



More information about the sssd-devel mailing list