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(a)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