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

Lukas Slebodnik lslebodn at redhat.com
Fri Jul 24 13:21:42 UTC 2015


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



More information about the sssd-devel mailing list