On Thu, Feb 25, 2016 at 01:37:27PM +0100, Sumit Bose wrote:
On Thu, Feb 25, 2016 at 12:50:55PM +0100, Jakub Hrozek wrote:
> On Tue, Feb 23, 2016 at 12:53:25PM +0100, Sumit Bose wrote:
> > Hi,
> >
> > this patch fixes and issue during initgroups in AD forests. Please see
> > the commit message for details.
> >
> > To reproduce this you can create a new user outside of CN=Users on the
> > forest root. The new user can be created in an existing container or in
> > a new OU container. Most important is that it is not a child of
> > CN=Users. In a child domain (it must be a child, domains with a
> > different base won't trigger the issue) create a user with the same
> > name. With this setup 'id user(a)forest.root' will not return the
complete
> > list of group the user is a member of and the patch should fix this.
> >
> > bye,
> > Sumit
> >
>
> Hi,
>
> the patch works fine, tested with a user named the same in the same OU
> in different domains. Coverity found some warnings:
>
> Error: COMPILER_WARNING:
> sssd-1.13.90/src/providers/ldap/sdap_async_initgroups.c:2835:12: warning: unused
variable 'dn_len' [-Wunused-variable]
> # size_t dn_len;
> # ^
> # 2833| const char *cname;
> # 2834| bool in_transaction = false;
> # 2835|-> size_t dn_len;
> # 2836| size_t c = 0;
> # 2837|
>
> Error: COMPILER_WARNING:
> sssd-1.13.90/src/providers/ldap/sdap_async_initgroups.c: scope_hint: In function
'sdap_get_initgr_user'
> sssd-1.13.90/src/providers/ldap/sdap_async_initgroups.c:2836:12: warning: unused
variable 'c' [-Wunused-variable]
> # size_t c = 0;
> # ^
> # 2834| bool in_transaction = false;
> # 2835| size_t dn_len;
> # 2836|-> size_t c = 0;
> # 2837|
> # 2838| DEBUG(SSSDBG_TRACE_ALL, "Receiving info for the user\n");
>
> Error: COMPILER_WARNING:
> sssd-1.13.90/src/db/sysdb_subdomains.c:23: included_from: Included from here.
> sssd-1.13.90/src/db/sysdb_subdomains.c: scope_hint: In function
'try_to_find_expected_dn'
> sssd-1.13.90/src/util/util.h:144:9: warning: 'result_dn_str' may be used
uninitialized in this function [-Wmaybe-uninitialized]
> # sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> # ^
> sssd-1.13.90/src/db/sysdb_subdomains.c:1075:17: note: 'result_dn_str' was
declared here
> # const char *result_dn_str;
> # ^
> # 142| int __debug_macro_level = level; \
> # 143| if (DEBUG_IS_SET(__debug_macro_level)) { \
> # 144|-> sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> # 145| __debug_macro_level, \
> # 146| format, ##__VA_ARGS__); \
>
> Additionally, I wonder if we should namespace the function?
Thank you for the review, new version attached.
bye,
Sumit
ACK.
There were some downstream tests failing, but the same tests kept
failing even with a vanilla RHEL-7.2 package, so I think it's a fluke in
the test and not a regression.