[SSSD] [PATCHES] ad: use tokengroups even when id mapping is disabled

Pavel Březina pbrezina at redhat.com
Thu Dec 12 12:20:59 UTC 2013


On 12/12/2013 01:08 PM, Jakub Hrozek wrote:
> On Thu, Dec 12, 2013 at 12:11:00PM +0100, Sumit Bose wrote:
>> On Thu, Dec 12, 2013 at 11:46:01AM +0100, Pavel Březina wrote:
>>> On 12/10/2013 04:59 PM, Sumit Bose wrote:
>>>> On Wed, Nov 20, 2013 at 02:41:49PM +0100, Pavel Březina wrote:
>>>>> On 11/19/2013 11:52 AM, Jakub Hrozek wrote:
>>>>>> On Fri, Nov 15, 2013 at 12:22:53PM +0100, Pavel Březina wrote:
>>>>>>> https://fedorahosted.org/sssd/ticket/1568
>>>>>>
>>>>>>>  From b66343b207679cbbbdb5d4a54a7f465fbf2ec97f Mon Sep 17 00:00:00 2001
>>>>>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>>>>>> Date: Tue, 12 Nov 2013 13:52:35 +0100
>>>>>>> Subject: [PATCH 1/3] sysdb: add sysdb_group_dn_str()
>>>>>>
>>>>>> The patch doesn't apply, but more importantly there already is
>>>>>> sysdb_group_dn_str(), can you use that instead?
>>>>>
>>>>> Thanks.
>>>>>
>>>>>>
>>>>>> The other two patches don't apply either. I have just a couple of
>>>>>> comments and questions as I scrolled through them:
>>>>>>
>>>>>> Patch #2 looks good to me so far, but I only read the diff.
>>>>>>
>>>>>> In patch #3, I don't see the point of using tmp_ctx and stealing to "state"
>>>>>> later if the request is relatively short. Can you use "state" directly?
>>>>>> That way you guarantee that the memory will go away with the request.
>>>>>
>>>>> I don't know, I think freeing all resources when they become
>>>>> irrelevant is more clear.
>>>>>
>>>>>> btw did you test if the code works even if only some of the groups have
>>>>>> posix attributes set?
>>>>>
>>>>> Yes, those non-posix groups are downloaded by SID but they have
>>>>> empty members.
>>>>>
>>>>> Rebase patches are attached.
>>>>
>>>> Patches apply and work as expected. Nevertheless I have two comments.
>>>>
>>>>>
>>>>> -errno_t
>>>>> -sdap_get_ad_tokengroups_initgroups_recv(struct tevent_req *req)
>>>>> +int sdap_ad_tokengroups_initgroups_recv(struct tevent_req *req)
>>>>>   {
>>>>
>>>> is there a reason to switch from errno_t to int here?
>>>
>>> None. Fixed.
>>>
>>>>> +struct tevent_req *
>>>>> +sdap_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
>>>>> +                                    struct tevent_context *ev,
>>>>> +                                    struct sdap_id_ctx *id_ctx,
>>>>> +                                    struct sdap_options *opts,
>>>>> +                                    struct sysdb_ctx *sysdb,
>>>>> +                                    struct sss_domain_info *domain,
>>>>> +                                    struct sdap_handle *sh,
>>>>> +                                    const char *name,
>>>>> +                                    const char *orig_dn,
>>>>> +                                    int timeout,
>>>>> +                                    bool use_id_mapping)
>>>>> +{
>>>>> +    struct sdap_ad_tokengroups_initgroups_state *state = NULL;
>>>>> +    struct tevent_req *req = NULL;
>>>>> +    struct tevent_req *subreq = NULL;
>>>>> +    errno_t ret;
>>>>> +
>>>>> +    req = tevent_req_create(mem_ctx, &state,
>>>>> +                            struct sdap_ad_tokengroups_initgroups_state);
>>>>> +    if (req == NULL) {
>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("tevent_req_create() failed\n"));
>>>>> +        return NULL;
>>>>> +    }
>>>>> +
>>>>> +    state->use_id_mapping = use_id_mapping;
>>>>> +
>>>>> +    if (state->use_id_mapping) {
>>>>> +        subreq = sdap_ad_tokengroups_initgr_mapping_send(state, ev, opts,
>>>>> +                                                         sysdb, domain, sh,
>>>>> +                                                         name, orig_dn,
>>>>> +                                                         timeout);
>>>>> +    } else {
>>>>> +        subreq = sdap_ad_tokengroups_initgr_posix_send(state, ev, id_ctx, opts,
>>>>> +                                                       sysdb, domain, sh,
>>>>> +                                                       name, orig_dn,
>>>>> +                                                       timeout);
>>>>> +    }
>>>>
>>>> I think it would avoid a bit of code duplication if this check would be
>>>> done after the tokenGroups is read from the server.
>>>
>>> You would avoid a little code duplication, but I believe it is not
>>> worth it, because the duplication is only in calling
>>> sdap_get_ad_tokengroups_send() and the amount of glue code would be
>>> approximately the same.
>>>
>>> Also I think, the current code flow is easier to follow.
>>
>> I guess it's a matter of taste :-), nevertheless ACK.
>>
>> bye,
>> Sumit
>
> I see a number of compilation warnings with the patches (using clang-3.3)
>
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:9: warning: variable 'in_transaction' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>      if (ret != EOK) {
>          ^~~~~~~~~~
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:9: note: uninitialized use occurs here
>      if (in_transaction) {
>          ^~~~~~~~~~~~~~
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:5: note: remove the 'if' if its condition is always false
>      if (ret != EOK) {
>      ^~~~~~~~~~~~~~~~~
> /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:778:9: warning: variable 'in_transaction' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>      if (groups == NULL) {
>          ^~~~~~~~~~~~~~

I also seen following warning with gcc and higher optimization level:
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c: 
In function 'sdap_ad_tokengroups_initgr_posix_tg_done':
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:1008:20: 
warning: 'num_sids' may be used uninitialized in this function 
[-Wmaybe-uninitialized]
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c: 
In function 'sdap_ad_tokengroups_initgr_mapping_done':
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:8: 
warning: 'in_transaction' may be used uninitialized in this function 
[-Wuninitialized]
/home/pbrezina/workspace/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:777:14: 
warning: 'num_sids' may be used uninitialized in this function 
[-Wmaybe-uninitialized]

New patches attached. Here's the diff.

--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -746,14 +746,14 @@ static void 
sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
      const char *name = NULL;
      const char *sid = NULL;
      char **sids = NULL;
-    size_t num_sids;
+    size_t num_sids = 0;
      size_t i;
      time_t now;
      gid_t gid;
      char **groups = NULL;
      size_t num_groups;
      errno_t ret, sret;
-    bool in_transaction;
+    bool in_transaction = false;

      tmp_ctx = talloc_new(NULL);
      if (tmp_ctx == NULL) {
@@ -977,7 +977,7 @@ sdap_ad_tokengroups_initgr_posix_tg_done(struct 
tevent_req *subreq)
      const char *name = NULL;
      char *sid = NULL;
      char **sids = NULL;
-    size_t num_sids;
+    size_t num_sids = 0;
      char **valid_groups = NULL;
      size_t num_valid_groups;
      char **missing_sids = NULL;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ad-refactor-tokengroups-initgroups.patch
Type: text/x-patch
Size: 27787 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131212/9535a130/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ad-use-tokengroups-even-when-id-mapping-is-disabled.patch
Type: text/x-patch
Size: 23643 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131212/9535a130/attachment-0001.bin>


More information about the sssd-devel mailing list