On Fri, Jun 14, 2013 at 03:08:28PM +0200, Pavel Březina wrote: > https://fedorahosted.org/sssd/ticket/1894 > > Reproducer: > group-1 -> group-2 -> enough groups that it triggers AD range retrieval > getent group group-1 > > I used 3000 groups and 1500 users. > > There is only one member in group-1 so it is processed with single step. AD > will return maximum 1500 values per attribute by default, so we allocate > space for 1500 groups. Then ASQ is called and returns 4500 members so we end > up writing after allocated space. > Nice catch! > Normally, we split the members to users, groups and unknown objects and > allocate space for groups + unknown. But I don't think it is necessary to do > the split here again. We allocate potentially more space than required and > then shrink it to the correct size. I tend to agree, unless there would be a very long running request with a huge number of members. But then again, the member of the array we allocate are sizeof(struct sysdb_attrs *) so it's not a big deal. Just one question below.. > --- a/src/providers/ldap/sdap_async_nested_groups.c > +++ b/src/providers/ldap/sdap_async_nested_groups.c > @@ -2048,6 +2048,18 @@ sdap_nested_group_deref_direct_process(struct tevent_req *subreq) > DEBUG(SSSDBG_TRACE_INTERNAL, ("Received %d dereference results, " > "about to process them\n", num_entries)); > > + if (num_entries != members->num_values) { Why does this condition say "!=" and not "<=" ? Can we ever allocate to

On 06/16/2013 12:52 PM, Jakub Hrozek wrote: >On Sun, Jun 16, 2013 at 12:29:26PM +0200, Jakub Hrozek wrote: >>On Fri, Jun 14, 2013 at 03:08:28PM +0200, Pavel Březina wrote: >>>https://fedorahosted.org/sssd/ticket/1894 >>> >>>Reproducer: >>>group-1 -> group-2 -> enough groups that it triggers AD range retrieval >>>getent group group-1 >>> >>>I used 3000 groups and 1500 users. >>> >>>There is only one member in group-1 so it is processed with single step. AD >>>will return maximum 1500 values per attribute by default, so we allocate >>>space for 1500 groups. Then ASQ is called and returns 4500 members so we end >>>up writing after allocated space. >>> >> >>Nice catch! >> >>>Normally, we split the members to users, groups and unknown objects and >>>allocate space for groups + unknown. But I don't think it is necessary to do >>>the split here again. We allocate potentially more space than required and >>>then shrink it to the correct size. >> >>I tend to agree, unless there would be a very long running request with >>a huge number of members. But then again, the member of the array we >>allocate are sizeof(struct sysdb_attrs *) so it's not a big deal. >> >>Just one question below.. >> >>>--- a/src/providers/ldap/sdap_async_nested_groups.c >>>+++ b/src/providers/ldap/sdap_async_nested_groups.c >>>@@ -2048,6 +2048,18 @@ sdap_nested_group_deref_direct_process(struct tevent_req *subreq) >>> DEBUG(SSSDBG_TRACE_INTERNAL, ("Received %d dereference results, " >>> "about to process them\n", num_entries)); >>> >>>+ if (num_entries != members->num_values) { >> >>Why does this condition say "!=" and not "<=" ? Can we ever allocate to > >Sorry, the way I propose the change, we'd also have to reverse the >operands and compare with < not <=: > >if (members->num_values < num_entries) { >} You are correct sir. Thanks.