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
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 Slebodniklslebodn@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. I am fine either way. Sorry I should have catch this in the first iteration of the initgr memcache patches.
Michal
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 Slebodniklslebodn@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;@@ -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;@@ -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
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 Slebodniklslebodn@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.
Michal
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 Slebodniklslebodn@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;@@ -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;@@ -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
On (16/07/15 17:07), 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 Slebodniklslebodn@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;@@ -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;@@ -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
From e6f0d72167fdd6921e173025a0e374996fc156d5 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Thu, 16 Jul 2015 16:54:00 +0200 Subject: [PATCH 1/2] mmap_cache: Rename variables
self-NACK
There is a crash in nss responder
Thread 1 (Thread 0x7f43151a8700 (LWP 30847)): #0 0x00007f431058a174 in __strcmp_sse2 () from /lib64/libc.so.6 No symbol table info available. #1 0x000000000041768b in sss_mc_find_record (mcc=0x113ee90, key=0x7ffce089a5b0) at +src/responder/nss/nsssrv_mmap_cache.c:562 rec = 0x7f4306f2b038 hash = 13194 slot = <value optimized out> name_ptr = <value optimized out> t_key = <value optimized out> strs_offset = 24 strs_len = <value optimized out> max_addr = 0x7f43076cc238 <Address 0x7f43076cc238 out of bounds> ret = 0 __FUNCTION__ = "sss_mc_find_record"
LS
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 Slebodniklslebodn@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;@@ -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;@@ -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
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 Slebodniklslebodn@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.
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 :)
Michal
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 Slebodniklslebodn@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.
LS
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 Slebodniklslebodn@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.
Michal
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 Slebodniklslebodn@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
On (17/07/15 14:07), Lukas Slebodnik wrote:
On (16/07/15 17:07), 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 Slebodniklslebodn@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;@@ -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;@@ -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
From e6f0d72167fdd6921e173025a0e374996fc156d5 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Thu, 16 Jul 2015 16:54:00 +0200 Subject: [PATCH 1/2] mmap_cache: Rename variables
self-NACK
There is a crash in nss responder
Thread 1 (Thread 0x7f43151a8700 (LWP 30847)): #0 0x00007f431058a174 in __strcmp_sse2 () from /lib64/libc.so.6 No symbol table info available. #1 0x000000000041768b in sss_mc_find_record (mcc=0x113ee90, key=0x7ffce089a5b0) at +src/responder/nss/nsssrv_mmap_cache.c:562 rec = 0x7f4306f2b038 hash = 13194 slot = <value optimized out> name_ptr = <value optimized out> t_key = <value optimized out> strs_offset = 24 strs_len = <value optimized out> max_addr = 0x7f43076cc238 <Address 0x7f43076cc238 out of bounds> ret = 0 __FUNCTION__ = "sss_mc_find_record"
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
LS
On (29/07/15 17:05), Lukas Slebodnik wrote:
On (17/07/15 14:07), Lukas Slebodnik wrote:
From e6f0d72167fdd6921e173025a0e374996fc156d5 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Thu, 16 Jul 2015 16:54:00 +0200 Subject: [PATCH 1/2] mmap_cache: Rename variables
self-NACK
There is a crash in nss responder
Thread 1 (Thread 0x7f43151a8700 (LWP 30847)): #0 0x00007f431058a174 in __strcmp_sse2 () from /lib64/libc.so.6 No symbol table info available. #1 0x000000000041768b in sss_mc_find_record (mcc=0x113ee90, key=0x7ffce089a5b0) at +src/responder/nss/nsssrv_mmap_cache.c:562 rec = 0x7f4306f2b038 hash = 13194 slot = <value optimized out> name_ptr = <value optimized out> t_key = <value optimized out> strs_offset = 24 strs_len = <value optimized out> max_addr = 0x7f43076cc238 <Address 0x7f43076cc238 out of bounds> ret = 0 __FUNCTION__ = "sss_mc_find_record"
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote:
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
On 07/30/2015 05:06 PM, Michal Židek wrote:
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote:
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
Ah... nevermind, I had exported SSS_NSS_USE_MEMCACHE=no in the terminal where I was running the tests.
Michal
On 07/30/2015 05:23 PM, Michal Židek wrote:
On 07/30/2015 05:06 PM, Michal Židek wrote:
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote:
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
Ah... nevermind, I had exported SSS_NSS_USE_MEMCACHE=no in the terminal where I was running the tests.
On the other hand this proves that your tests really catch situation when memcache does not work :)
Thank you for expanding the tests coverage.
Tentative ACK to the patches (will fully ack when CI results finish).
I filled a ticket to track one issue, but it does not seem to be related to the memcache. https://fedorahosted.org/sssd/ticket/2741
Michal
On 07/31/2015 04:54 PM, Michal Židek wrote:
On 07/30/2015 05:23 PM, Michal Židek wrote:
On 07/30/2015 05:06 PM, Michal Židek wrote:
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote:
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
Ah... nevermind, I had exported SSS_NSS_USE_MEMCACHE=no in the terminal where I was running the tests.
On the other hand this proves that your tests really catch situation when memcache does not work :)
Thank you for expanding the tests coverage.
Tentative ACK to the patches (will fully ack when CI results finish).
I filled a ticket to track one issue, but it does not seem to be related to the memcache. https://fedorahosted.org/sssd/ticket/2741
Well, it is related to the memcache. But not that memcache itself would not work. We somewhere miss call to clear the memcache in the code branch that 'su' triggers.
Michal
On (31/07/15 16:54), Michal Židek wrote:
On 07/30/2015 05:23 PM, Michal Židek wrote:
On 07/30/2015 05:06 PM, Michal Židek wrote:
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote:
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
Ah... nevermind, I had exported SSS_NSS_USE_MEMCACHE=no in the terminal where I was running the tests.
On the other hand this proves that your tests really catch situation when memcache does not work :)
Thank you for expanding the tests coverage.
Tentative ACK to the patches (will fully ack when CI results finish).
I filled a ticket to track one issue, but it does not seem to be related to the memcache. https://fedorahosted.org/sssd/ticket/2741
Ticket #2741 seems to be duplicate of #2716 It means that regression is not fixed an patches canot be pushed
LS
On (31/07/15 17:07), Michal Židek wrote:
On 07/31/2015 04:54 PM, Michal Židek wrote:
On 07/30/2015 05:23 PM, Michal Židek wrote:
On 07/30/2015 05:06 PM, Michal Židek wrote:
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote:
Attached is a new patch set. I also contains fix for #2712 and not just for #2716. Otherwise there would be a crash. So it would be better push them together.
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
Ah... nevermind, I had exported SSS_NSS_USE_MEMCACHE=no in the terminal where I was running the tests.
On the other hand this proves that your tests really catch situation when memcache does not work :)
Thank you for expanding the tests coverage.
Tentative ACK to the patches (will fully ack when CI results finish).
I filled a ticket to track one issue, but it does not seem to be related to the memcache. https://fedorahosted.org/sssd/ticket/2741
Well, it is related to the memcache. But not that memcache itself would not work. We somewhere miss call to clear the memcache in the code branch that 'su' triggers.
The bug #2761 was fixed just partialy by the second patch "mmap_cache: "Override" functions for initgr mmap cache"
Initgroups mmap cache was update but it was a coincidence caused by bug #2743 with validation of ingegrity check. Which was fixed by another commit.
This is a reason why I opened ticket #2743. It is an important bug which should be tracked and probably fixed in stable branches. There are two possible solutions. Remove integrity check for sssd >= 1.12 or fix integrity check for sssd >= 1.11
Updated patches are attached. BTW I would catch this bug as well with integration test but we do not have a pam wrapper yet.
Thank you for review.
LS
On 08/03/2015 02:08 PM, Lukas Slebodnik wrote:
On (31/07/15 17:07), Michal Židek wrote:
On 07/31/2015 04:54 PM, Michal Židek wrote:
On 07/30/2015 05:23 PM, Michal Židek wrote:
On 07/30/2015 05:06 PM, Michal Židek wrote:
On 07/30/2015 04:15 PM, Lukas Slebodnik wrote:
On (29/07/15 17:05), Lukas Slebodnik wrote: > Attached is a new patch set. I also contains fix for #2712 and not > just for > #2716. Otherwise there would be a crash. So it would be better > push them together. >
Integration test were failing sporadically due to bug in clien code. 3rd patch fix this problem.
Other patches are the same.
LS
Integration tests still do not work for me even with this version.
$ contrib/ci/run -n -r autoreconf: success 00:00:14 ci-autoreconf.log DEBUG BUILD: ci-build-debug configure: success 00:00:10 ci-build-debug/ci-configure.log make-tests: success 00:02:18 ci-build-debug/ci-make-tests.log make-check-valgrind: success 00:01:41 ci-build-debug/ci-make-check-valgrind.log make-intgcheck: failure 00:01:55 ci-build-debug/ci-make-intgcheck.log FAILURE
See sanitized log. These are the newly added _with_mc tests.
Michal
Ah... nevermind, I had exported SSS_NSS_USE_MEMCACHE=no in the terminal where I was running the tests.
On the other hand this proves that your tests really catch situation when memcache does not work :)
Thank you for expanding the tests coverage.
Tentative ACK to the patches (will fully ack when CI results finish).
I filled a ticket to track one issue, but it does not seem to be related to the memcache. https://fedorahosted.org/sssd/ticket/2741
Well, it is related to the memcache. But not that memcache itself would not work. We somewhere miss call to clear the memcache in the code branch that 'su' triggers.
The bug #2761 was fixed just partialy by the second patch "mmap_cache: "Override" functions for initgr mmap cache"
Initgroups mmap cache was update but it was a coincidence caused by bug #2743 with validation of ingegrity check. Which was fixed by another commit.
This is a reason why I opened ticket #2743. It is an important bug which should be tracked and probably fixed in stable branches. There are two possible solutions. Remove integrity check for sssd >= 1.12 or fix integrity check for sssd >= 1.11
Updated patches are attached. BTW I would catch this bug as well with integration test but we do not have a pam wrapper yet.
Thank you for review.
LS
With the added patch, now both 'su' and 'ssh' authentications properly update the memcache.
CI link: http://sssd-ci.duckdns.org/logs/commit/6f/d9b55cc0feccdcbc5bfae25f4e016c012d...
F20: FAIL: responder_cache_req-tests FRawhide: failed mock build
Local 'contrib/ci/run -n -r' passed on F21.
ACK to all patches.
Michal
On Tue, Aug 04, 2015 at 08:58:14PM +0200, Michal Židek wrote:
ACK to all patches.
Michal
* master: * cb8c24707275c5bda7310d67e7f46c75d3ac36ea * dda0258705de7255e6ec54b7f9adbde83a220996 * a2c10cf31d14bac598f5cd008973375c3f9575a6 * 38b07019861240cf5107f5d51fc0027519e21619 * ba847347cade817ee927397d82c952b51b0dcb2b * ea7839cec593b4a7c678fab52ab864518db6699b * 225dc6914cdc8920b02a129b98ece1ed97b99c03 * 39b31427e2d11ca318df11fd48db33a7cc610aa7
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek