Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
On Tue, Jul 22, 2014 at 06:43:56PM +0200, Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
sorry, I completely forgot, too. Patch looks good and didn't show issues in my tests. ACK.
bye, Sumit
On Wed, Jul 23, 2014 at 07:14:42PM +0200, Sumit Bose wrote:
On Tue, Jul 22, 2014 at 06:43:56PM +0200, Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
sorry, I completely forgot, too. Patch looks good and didn't show issues in my tests. ACK.
bye, Sumit
Thanks for the swift review, pushed to master: 8a8618717c99b7331125fa736b45d9155da797d3 e3c994b7b779e6d6d6d125ee28d30bd139590f49
On (22/07/14 18:43), Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
From 3c4ce4a2b9f58e0b50f9ad48d07110b30bc74cd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 11 Mar 2014 17:39:31 +0100 Subject: [PATCH 2/2] Only check GID if ID-mapping
Makefile.am | 11 ++++++++--- src/providers/ldap/sdap_async_nested_groups.c | 9 ++++++++- src/tests/cmocka/test_nested_groups.c | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am index e3592868ce29b569a71f6ad74bc24cc617301b34..783a8922ede16ea3cc3eb6df568773126c5661fb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1740,15 +1740,20 @@ fqnames_tests_LDADD = \ nestedgroups_tests_SOURCES = \ $(TEST_MOCK_OBJ) \ $(TEST_MOCK_PROVIDER_OBJ) \
- src/providers/ldap/sdap_idmap.c \ src/tests/cmocka/test_nested_groups.c \
- src/providers/ldap/sdap_async_nested_groups.c
- src/providers/ldap/sdap_async_nested_groups.c \
- $(NULL)
nestedgroups_tests_CFLAGS = \
- $(AM_CFLAGS)
- $(AM_CFLAGS) \
- $(NULL)
nestedgroups_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
- libsss_idmap.la \
- libsss_test_common.la \
- $(NULL)
test_sss_idmap_SOURCES = \ src/tests/cmocka/test_sss_idmap.c diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c index 305afbc9d4f153fe2daa6289cc318b1183be72ca..5398b14bc0e163c122def1c3d729b28d72873b82 100644 --- a/src/providers/ldap/sdap_async_nested_groups.c +++ b/src/providers/ldap/sdap_async_nested_groups.c @@ -34,6 +34,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" #include "providers/ldap/sdap_async_private.h" +#include "providers/ldap/sdap_idmap.h"
#define sdap_nested_group_sysdb_search_users(domain, filter) \ sdap_nested_group_sysdb_search((domain), (filter), true) @@ -242,6 +243,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, errno_t ret; int32_t ad_group_type; bool posix_group = true;
bool use_id_mapping;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) { ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -265,7 +267,12 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, } }
- if (posix_group) {
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
group_ctx->opts->idmap_ctx,group_ctx->domain->name,group_ctx->domain->domain_id);- if (posix_group && !use_id_mapping) { ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, &gid); }
270 use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( 271 group_ctx->opts->idmap_ctx, 272 group_ctx->domain->name, 273 group_ctx->domain->domain_id); 274 275 if (posix_group && !use_id_mapping) { // 'posix_group' can be true and 'use_id_mapping' is not equal to 0 // sysdb_attrs_get_uint32_t will not be called. 276 ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, 277 &gid); 278 } 279 if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) { ^^^^^^^^^^^^^ 'ret' needn't be ENOENT because function 'sysdb_attrs_get_int32_t' could be called on line 249
'gid' will not be initialised.
LS
On Mon, Aug 11, 2014 at 10:05:56AM +0200, Lukas Slebodnik wrote:
On (22/07/14 18:43), Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
From 3c4ce4a2b9f58e0b50f9ad48d07110b30bc74cd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 11 Mar 2014 17:39:31 +0100 Subject: [PATCH 2/2] Only check GID if ID-mapping
Makefile.am | 11 ++++++++--- src/providers/ldap/sdap_async_nested_groups.c | 9 ++++++++- src/tests/cmocka/test_nested_groups.c | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am index e3592868ce29b569a71f6ad74bc24cc617301b34..783a8922ede16ea3cc3eb6df568773126c5661fb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1740,15 +1740,20 @@ fqnames_tests_LDADD = \ nestedgroups_tests_SOURCES = \ $(TEST_MOCK_OBJ) \ $(TEST_MOCK_PROVIDER_OBJ) \
- src/providers/ldap/sdap_idmap.c \ src/tests/cmocka/test_nested_groups.c \
- src/providers/ldap/sdap_async_nested_groups.c
- src/providers/ldap/sdap_async_nested_groups.c \
- $(NULL)
nestedgroups_tests_CFLAGS = \
- $(AM_CFLAGS)
- $(AM_CFLAGS) \
- $(NULL)
nestedgroups_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
- libsss_idmap.la \
- libsss_test_common.la \
- $(NULL)
test_sss_idmap_SOURCES = \ src/tests/cmocka/test_sss_idmap.c diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c index 305afbc9d4f153fe2daa6289cc318b1183be72ca..5398b14bc0e163c122def1c3d729b28d72873b82 100644 --- a/src/providers/ldap/sdap_async_nested_groups.c +++ b/src/providers/ldap/sdap_async_nested_groups.c @@ -34,6 +34,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" #include "providers/ldap/sdap_async_private.h" +#include "providers/ldap/sdap_idmap.h"
#define sdap_nested_group_sysdb_search_users(domain, filter) \ sdap_nested_group_sysdb_search((domain), (filter), true) @@ -242,6 +243,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, errno_t ret; int32_t ad_group_type; bool posix_group = true;
bool use_id_mapping;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) { ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -265,7 +267,12 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, } }
- if (posix_group) {
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
group_ctx->opts->idmap_ctx,group_ctx->domain->name,group_ctx->domain->domain_id);- if (posix_group && !use_id_mapping) { ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, &gid); }
270 use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( 271 group_ctx->opts->idmap_ctx, 272 group_ctx->domain->name, 273 group_ctx->domain->domain_id); 274 275 if (posix_group && !use_id_mapping) { // 'posix_group' can be true and 'use_id_mapping' is not equal to 0 // sysdb_attrs_get_uint32_t will not be called. 276 ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, 277 &gid); 278 } 279 if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) { ^^^^^^^^^^^^^ 'ret' needn't be ENOENT because function 'sysdb_attrs_get_int32_t' could be called on line 249
'gid' will not be initialised.LS
Can you send a patch?
On (11/08/14 10:24), Jakub Hrozek wrote:
On Mon, Aug 11, 2014 at 10:05:56AM +0200, Lukas Slebodnik wrote:
On (22/07/14 18:43), Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
From 3c4ce4a2b9f58e0b50f9ad48d07110b30bc74cd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 11 Mar 2014 17:39:31 +0100 Subject: [PATCH 2/2] Only check GID if ID-mapping
Makefile.am | 11 ++++++++--- src/providers/ldap/sdap_async_nested_groups.c | 9 ++++++++- src/tests/cmocka/test_nested_groups.c | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am index e3592868ce29b569a71f6ad74bc24cc617301b34..783a8922ede16ea3cc3eb6df568773126c5661fb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1740,15 +1740,20 @@ fqnames_tests_LDADD = \ nestedgroups_tests_SOURCES = \ $(TEST_MOCK_OBJ) \ $(TEST_MOCK_PROVIDER_OBJ) \
- src/providers/ldap/sdap_idmap.c \ src/tests/cmocka/test_nested_groups.c \
- src/providers/ldap/sdap_async_nested_groups.c
- src/providers/ldap/sdap_async_nested_groups.c \
- $(NULL)
nestedgroups_tests_CFLAGS = \
- $(AM_CFLAGS)
- $(AM_CFLAGS) \
- $(NULL)
nestedgroups_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
- libsss_idmap.la \
- libsss_test_common.la \
- $(NULL)
test_sss_idmap_SOURCES = \ src/tests/cmocka/test_sss_idmap.c diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c index 305afbc9d4f153fe2daa6289cc318b1183be72ca..5398b14bc0e163c122def1c3d729b28d72873b82 100644 --- a/src/providers/ldap/sdap_async_nested_groups.c +++ b/src/providers/ldap/sdap_async_nested_groups.c @@ -34,6 +34,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" #include "providers/ldap/sdap_async_private.h" +#include "providers/ldap/sdap_idmap.h"
#define sdap_nested_group_sysdb_search_users(domain, filter) \ sdap_nested_group_sysdb_search((domain), (filter), true) @@ -242,6 +243,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, errno_t ret; int32_t ad_group_type; bool posix_group = true;
bool use_id_mapping;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) { ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -265,7 +267,12 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, } }
- if (posix_group) {
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
group_ctx->opts->idmap_ctx,group_ctx->domain->name,group_ctx->domain->domain_id);- if (posix_group && !use_id_mapping) { ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, &gid); }
270 use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( 271 group_ctx->opts->idmap_ctx, 272 group_ctx->domain->name, 273 group_ctx->domain->domain_id); 274 275 if (posix_group && !use_id_mapping) { // 'posix_group' can be true and 'use_id_mapping' is not equal to 0 // sysdb_attrs_get_uint32_t will not be called. 276 ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, 277 &gid); 278 } 279 if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) { ^^^^^^^^^^^^^ 'ret' needn't be ENOENT because function 'sysdb_attrs_get_int32_t' could be called on line 249
'gid' will not be initialised.LS
Can you send a patch?
I didn't send, because I was not sure was is the right solution.
LS
On (11/08/14 10:29), Lukas Slebodnik wrote:
On (11/08/14 10:24), Jakub Hrozek wrote:
On Mon, Aug 11, 2014 at 10:05:56AM +0200, Lukas Slebodnik wrote:
On (22/07/14 18:43), Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote:
Hi,
the attached two patches are not strictly related to tokenGroups processing, but it's very easy to reproduce the problem that way. The issue is only confusing DEBUG messages, but it has already cost me several hours in processing logs from an SSSD user, so I think a fix is due, at least for master.
See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
From 3c4ce4a2b9f58e0b50f9ad48d07110b30bc74cd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 11 Mar 2014 17:39:31 +0100 Subject: [PATCH 2/2] Only check GID if ID-mapping
Makefile.am | 11 ++++++++--- src/providers/ldap/sdap_async_nested_groups.c | 9 ++++++++- src/tests/cmocka/test_nested_groups.c | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am index e3592868ce29b569a71f6ad74bc24cc617301b34..783a8922ede16ea3cc3eb6df568773126c5661fb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1740,15 +1740,20 @@ fqnames_tests_LDADD = \ nestedgroups_tests_SOURCES = \ $(TEST_MOCK_OBJ) \ $(TEST_MOCK_PROVIDER_OBJ) \
- src/providers/ldap/sdap_idmap.c \ src/tests/cmocka/test_nested_groups.c \
- src/providers/ldap/sdap_async_nested_groups.c
- src/providers/ldap/sdap_async_nested_groups.c \
- $(NULL)
nestedgroups_tests_CFLAGS = \
- $(AM_CFLAGS)
- $(AM_CFLAGS) \
- $(NULL)
nestedgroups_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
- libsss_idmap.la \
- libsss_test_common.la \
- $(NULL)
test_sss_idmap_SOURCES = \ src/tests/cmocka/test_sss_idmap.c diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c index 305afbc9d4f153fe2daa6289cc318b1183be72ca..5398b14bc0e163c122def1c3d729b28d72873b82 100644 --- a/src/providers/ldap/sdap_async_nested_groups.c +++ b/src/providers/ldap/sdap_async_nested_groups.c @@ -34,6 +34,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" #include "providers/ldap/sdap_async_private.h" +#include "providers/ldap/sdap_idmap.h"
#define sdap_nested_group_sysdb_search_users(domain, filter) \ sdap_nested_group_sysdb_search((domain), (filter), true) @@ -242,6 +243,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, errno_t ret; int32_t ad_group_type; bool posix_group = true;
bool use_id_mapping;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) { ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -265,7 +267,12 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, } }
- if (posix_group) {
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
group_ctx->opts->idmap_ctx,group_ctx->domain->name,group_ctx->domain->domain_id);- if (posix_group && !use_id_mapping) { ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, &gid); }
270 use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( 271 group_ctx->opts->idmap_ctx, 272 group_ctx->domain->name, 273 group_ctx->domain->domain_id); 274 275 if (posix_group && !use_id_mapping) { // 'posix_group' can be true and 'use_id_mapping' is not equal to 0 // sysdb_attrs_get_uint32_t will not be called. 276 ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, 277 &gid); 278 } 279 if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) { ^^^^^^^^^^^^^ 'ret' needn't be ENOENT because function 'sysdb_attrs_get_int32_t' could be called on line 249
'gid' will not be initialised.LS
Can you send a patch?
I didn't send, because I was not sure was is the right solution.
LS
I saw it also in vagrind output.
==1895== Conditional jump or move depends on uninitialised value(s) ==1895== at 0x13F1A1D7: sdap_nested_group_hash_group (sdap_async_nested_groups.c:279) ==1895== by 0x13F1DAA1: sdap_nested_group_send (sdap_async_nested_groups.c:718) ==1895== by 0x13F1998D: sdap_get_groups_process (sdap_async_groups.c:1847) ==1895== by 0x13F0F9CE: sdap_get_generic_ext_done (sdap_async.c:1467) ==1895== by 0x13F0EE9F: sdap_process_result (sdap_async.c:357) ==1895== by 0x54ABFBE: tevent_common_loop_timer_delay (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54ACFC9: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54AB6B6: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54A7F2C: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54A80CA: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54AB656: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x5283872: server_loop (server.c:587)
Do we want to initialise variable 'gid' to 0 or (gid_t)-1?
LS
On Mon, Aug 11, 2014 at 03:16:12PM +0200, Lukas Slebodnik wrote:
On (11/08/14 10:29), Lukas Slebodnik wrote:
On (11/08/14 10:24), Jakub Hrozek wrote:
On Mon, Aug 11, 2014 at 10:05:56AM +0200, Lukas Slebodnik wrote:
On (22/07/14 18:43), Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote:
On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote: > Hi, > > the attached two patches are not strictly related to tokenGroups > processing, but it's very easy to reproduce the problem that way. The > issue is only confusing DEBUG messages, but it has already cost me > several hours in processing logs from an SSSD user, so I think a fix is > due, at least for master. > > See the patches and the commit messages for more details.
as a first note, the second patch makes nestedgroups-tests fail because sdap_idmap_domain_has_algorithmic_mapping() must be made available to the test and sdap_idmap_ctx must be initialized.
But since this 'only' influences the tests I will run some tests with the current version of the patches.
bye, Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
From 3c4ce4a2b9f58e0b50f9ad48d07110b30bc74cd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 11 Mar 2014 17:39:31 +0100 Subject: [PATCH 2/2] Only check GID if ID-mapping
Makefile.am | 11 ++++++++--- src/providers/ldap/sdap_async_nested_groups.c | 9 ++++++++- src/tests/cmocka/test_nested_groups.c | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am index e3592868ce29b569a71f6ad74bc24cc617301b34..783a8922ede16ea3cc3eb6df568773126c5661fb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1740,15 +1740,20 @@ fqnames_tests_LDADD = \ nestedgroups_tests_SOURCES = \ $(TEST_MOCK_OBJ) \ $(TEST_MOCK_PROVIDER_OBJ) \
- src/providers/ldap/sdap_idmap.c \ src/tests/cmocka/test_nested_groups.c \
- src/providers/ldap/sdap_async_nested_groups.c
- src/providers/ldap/sdap_async_nested_groups.c \
- $(NULL)
nestedgroups_tests_CFLAGS = \
- $(AM_CFLAGS)
- $(AM_CFLAGS) \
- $(NULL)
nestedgroups_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
- libsss_idmap.la \
- libsss_test_common.la \
- $(NULL)
test_sss_idmap_SOURCES = \ src/tests/cmocka/test_sss_idmap.c diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c index 305afbc9d4f153fe2daa6289cc318b1183be72ca..5398b14bc0e163c122def1c3d729b28d72873b82 100644 --- a/src/providers/ldap/sdap_async_nested_groups.c +++ b/src/providers/ldap/sdap_async_nested_groups.c @@ -34,6 +34,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" #include "providers/ldap/sdap_async_private.h" +#include "providers/ldap/sdap_idmap.h"
#define sdap_nested_group_sysdb_search_users(domain, filter) \ sdap_nested_group_sysdb_search((domain), (filter), true) @@ -242,6 +243,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, errno_t ret; int32_t ad_group_type; bool posix_group = true;
bool use_id_mapping;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) { ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -265,7 +267,12 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, } }
- if (posix_group) {
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
group_ctx->opts->idmap_ctx,group_ctx->domain->name,group_ctx->domain->domain_id);- if (posix_group && !use_id_mapping) { ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, &gid); }
270 use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( 271 group_ctx->opts->idmap_ctx, 272 group_ctx->domain->name, 273 group_ctx->domain->domain_id); 274 275 if (posix_group && !use_id_mapping) { // 'posix_group' can be true and 'use_id_mapping' is not equal to 0 // sysdb_attrs_get_uint32_t will not be called. 276 ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, 277 &gid); 278 } 279 if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) { ^^^^^^^^^^^^^ 'ret' needn't be ENOENT because function 'sysdb_attrs_get_int32_t' could be called on line 249
'gid' will not be initialised.LS
Can you send a patch?
I didn't send, because I was not sure was is the right solution.
LS
I saw it also in vagrind output.
==1895== Conditional jump or move depends on uninitialised value(s) ==1895== at 0x13F1A1D7: sdap_nested_group_hash_group (sdap_async_nested_groups.c:279) ==1895== by 0x13F1DAA1: sdap_nested_group_send (sdap_async_nested_groups.c:718) ==1895== by 0x13F1998D: sdap_get_groups_process (sdap_async_groups.c:1847) ==1895== by 0x13F0F9CE: sdap_get_generic_ext_done (sdap_async.c:1467) ==1895== by 0x13F0EE9F: sdap_process_result (sdap_async.c:357) ==1895== by 0x54ABFBE: tevent_common_loop_timer_delay (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54ACFC9: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54AB6B6: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54A7F2C: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54A80CA: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54AB656: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x5283872: server_loop (server.c:587)
Do we want to initialise variable 'gid' to 0 or (gid_t)-1?
I guess 0 is safer because we explicitly skip it later, right?
On (11/08/14 15:22), Jakub Hrozek wrote:
On Mon, Aug 11, 2014 at 03:16:12PM +0200, Lukas Slebodnik wrote:
On (11/08/14 10:29), Lukas Slebodnik wrote:
On (11/08/14 10:24), Jakub Hrozek wrote:
On Mon, Aug 11, 2014 at 10:05:56AM +0200, Lukas Slebodnik wrote:
On (22/07/14 18:43), Jakub Hrozek wrote:
On Thu, Mar 13, 2014 at 12:00:47PM +0100, Sumit Bose wrote: > On Wed, Mar 12, 2014 at 11:20:42PM +0100, Jakub Hrozek wrote: > > Hi, > > > > the attached two patches are not strictly related to tokenGroups > > processing, but it's very easy to reproduce the problem that way. The > > issue is only confusing DEBUG messages, but it has already cost me > > several hours in processing logs from an SSSD user, so I think a fix is > > due, at least for master. > > > > See the patches and the commit messages for more details. > > as a first note, the second patch makes nestedgroups-tests fail because > sdap_idmap_domain_has_algorithmic_mapping() must be made available to > the test and sdap_idmap_ctx must be initialized. > > But since this 'only' influences the tests I will run some tests with > the current version of the patches. > > bye, > Sumit
I completely forgot about these patches until I was doing cleanup of my git branches. Attached is a new revision..
From 3c4ce4a2b9f58e0b50f9ad48d07110b30bc74cd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 11 Mar 2014 17:39:31 +0100 Subject: [PATCH 2/2] Only check GID if ID-mapping
Makefile.am | 11 ++++++++--- src/providers/ldap/sdap_async_nested_groups.c | 9 ++++++++- src/tests/cmocka/test_nested_groups.c | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am index e3592868ce29b569a71f6ad74bc24cc617301b34..783a8922ede16ea3cc3eb6df568773126c5661fb 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1740,15 +1740,20 @@ fqnames_tests_LDADD = \ nestedgroups_tests_SOURCES = \ $(TEST_MOCK_OBJ) \ $(TEST_MOCK_PROVIDER_OBJ) \
- src/providers/ldap/sdap_idmap.c \ src/tests/cmocka/test_nested_groups.c \
- src/providers/ldap/sdap_async_nested_groups.c
- src/providers/ldap/sdap_async_nested_groups.c \
- $(NULL)
nestedgroups_tests_CFLAGS = \
- $(AM_CFLAGS)
- $(AM_CFLAGS) \
- $(NULL)
nestedgroups_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
- libsss_idmap.la \
- libsss_test_common.la \
- $(NULL)
test_sss_idmap_SOURCES = \ src/tests/cmocka/test_sss_idmap.c diff --git a/src/providers/ldap/sdap_async_nested_groups.c b/src/providers/ldap/sdap_async_nested_groups.c index 305afbc9d4f153fe2daa6289cc318b1183be72ca..5398b14bc0e163c122def1c3d729b28d72873b82 100644 --- a/src/providers/ldap/sdap_async_nested_groups.c +++ b/src/providers/ldap/sdap_async_nested_groups.c @@ -34,6 +34,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" #include "providers/ldap/sdap_async_private.h" +#include "providers/ldap/sdap_idmap.h"
#define sdap_nested_group_sysdb_search_users(domain, filter) \ sdap_nested_group_sysdb_search((domain), (filter), true) @@ -242,6 +243,7 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, errno_t ret; int32_t ad_group_type; bool posix_group = true;
bool use_id_mapping;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) { ret = sysdb_attrs_get_int32_t(group, SYSDB_GROUP_TYPE, &ad_group_type);
@@ -265,7 +267,12 @@ sdap_nested_group_hash_group(struct sdap_nested_group_ctx *group_ctx, } }
- if (posix_group) {
- use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping(
group_ctx->opts->idmap_ctx,group_ctx->domain->name,group_ctx->domain->domain_id);- if (posix_group && !use_id_mapping) { ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, &gid); }
270 use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( 271 group_ctx->opts->idmap_ctx, 272 group_ctx->domain->name, 273 group_ctx->domain->domain_id); 274 275 if (posix_group && !use_id_mapping) { // 'posix_group' can be true and 'use_id_mapping' is not equal to 0 // sysdb_attrs_get_uint32_t will not be called. 276 ret = sysdb_attrs_get_uint32_t(group, map[SDAP_AT_GROUP_GID].sys_name, 277 &gid); 278 } 279 if (!posix_group || ret == ENOENT || (ret == EOK && gid == 0)) { ^^^^^^^^^^^^^ 'ret' needn't be ENOENT because function 'sysdb_attrs_get_int32_t' could be called on line 249
'gid' will not be initialised.LS
Can you send a patch?
I didn't send, because I was not sure was is the right solution.
LS
I saw it also in vagrind output.
==1895== Conditional jump or move depends on uninitialised value(s) ==1895== at 0x13F1A1D7: sdap_nested_group_hash_group (sdap_async_nested_groups.c:279) ==1895== by 0x13F1DAA1: sdap_nested_group_send (sdap_async_nested_groups.c:718) ==1895== by 0x13F1998D: sdap_get_groups_process (sdap_async_groups.c:1847) ==1895== by 0x13F0F9CE: sdap_get_generic_ext_done (sdap_async.c:1467) ==1895== by 0x13F0EE9F: sdap_process_result (sdap_async.c:357) ==1895== by 0x54ABFBE: tevent_common_loop_timer_delay (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54ACFC9: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54AB6B6: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54A7F2C: _tevent_loop_once (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54A80CA: tevent_common_loop_wait (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x54AB656: ??? (in /usr/lib64/libtevent.so.0.9.20) ==1895== by 0x5283872: server_loop (server.c:587)
Do we want to initialise variable 'gid' to 0 or (gid_t)-1?
I guess 0 is safer because we explicitly skip it later, right?
Could you send a patch? I can review it :-)
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Sumit Bose