[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#4148) fix for ITS 4134 reintroduces problem fixed in ITS 3980



Thanks, your diagnosis looks correct. Good spotting.
 
kevins@BMRB.Co.Uk wrote:
> Howard,
>
> I think I've found and hopefully fixed the problem...
>
> I found through observation and inserting a few printf's that there was
> an issue in the section of ppolicy_modify starting at around line 1189,
> where it iterates through each modification to see if it is a delete and
> one of pwdGraceUseTime, pwdAccountLockedTime, pwdFailureTime.
>
> It appeared that it was only spotting the first delete modification -
> the ldif I was using had the following lines...
>
> delete: pwdGraceUseTime
> -
> delete: pwdFailureTime
>
> Neither attribute was present on the replica but it was dropping the
> pwdGraceUseTime modification and not dropping the pwdFailureTime.
>
> [I'm not really a C programmer, so some of the following diagnosis might
> not be quite spot on!] 
>
> I tracked this down to the for loop...
>
> 	for( prev = &op->oq_modify.rs_modlist, ml = *prev; ml;
> 	                        prev = &ml->sml_next, ml = *prev ) {
>
> ...and the drop itself...
>    		  	  if ( drop ) {
>                                 *prev = ml->sml_next;
>                                 ml->sml_next = NULL;
>                                 slap_mods_free( ml, 1 );
>                           }
>
> What I think was happening was that if a modification was dropped
> 'ml->sml_next = NULL' in the drop code causes 'prev = &ml->sml_next, ml
> = *prev' to result in ml being NULL and thus the loop exits without
> considering further attributes.  I also suspect this is referencing
> already freed memory [but I'm assuming what slap_mods_free() does]
> sticking an 'ml = *prev' at the end of the drop code improves this but
> still skips one attribute.
>
> The following patch seems to solve the problem in my limited testing
>
> --- servers/slapd/overlays/ppolicy.c.orig       Sat Nov  5 02:09:40 2005
> +++ servers/slapd/overlays/ppolicy.c    Sat Nov  5 02:06:08 2005
> @@ -1186,8 +1186,7 @@
>                 a_lock = attr_find( e->e_attrs,
> ad_pwdAccountLockedTime );
>                 a_fail = attr_find( e->e_attrs, ad_pwdFailureTime );
>
> -               for( prev = &op->oq_modify.rs_modlist, ml = *prev; ml;
> -                       prev = &ml->sml_next, ml = *prev ) {
> +               for( prev = &op->oq_modify.rs_modlist, ml = *prev; ml; )
> {
>
>                         if ( ml->sml_desc ==
> slap_schema.si_ad_userPassword )
>                                 got_pw = 1;
> @@ -1217,8 +1216,13 @@
>                                         *prev = ml->sml_next;
>                                         ml->sml_next = NULL;
>                                         slap_mods_free( ml, 1 );
> +                                       ml = *prev;
> +                                       continue;
>                                 }
>                         }
> +
> +                       prev = &ml->sml_next;
> +                       ml = *prev;
>                 }
>
>                 /* If we're resetting the password, make sure grace,
> accountlock,
>
>   


-- 
  -- Howard Chu
  Chief Architect, Symas Corp.  http://www.symas.com
  Director, Highland Sun        http://highlandsun.com/hyc
  OpenLDAP Core Team            http://www.openldap.org/project/