[Samba] RPC: Problem Deleting LDAP-Entries in pdb_ldap.c

[Andrew Bartlett]

On Fri, 2004-02-20 at 20:22, Yohann Fourteau wrote:
> Arg : here is the patch (for Samba 3.0.2):
> ----------------------------------8<---------------------
> --- a/smbldap.c	Thu Feb 19 15:52:00 2004
> +++ b/smbldap.c	Thu Feb 19 15:52:13 2004
> @@ -970,22 +970,166 @@
>  	int 		rc = LDAP_SERVER_DOWN;
>  	int 		attempts = 0;
>  	char           *utf8_dn;
> +	/* Yohann */

Can we have a few more comments than this?  I know Samba doesn't do
comments well, but I need to understand what's going on here...

> +	BOOL		do_rename = False;
> +	BOOL		naming_deleted = False;
> +	BOOL		naming_more_value = False;
> +	int		i,j,k,new_rdn_len,new_dn_len;
> +	char	       *rdn_attribut;
> +	char 	      **rdn;
> +	char	       *new_rdn;
> +	char	       *new_dn;
> +	char	       *utf8_new_rdn;
> +	char	       *utf8_new_dn;
> +	char	       *new_rdn_value;
> +	char	       *rdn_value;
> +	
> +	rdn = ldap_explode_dn(dn,0);
> +	rdn_attribut = strdup(strtok(rdn[0],"="));
> +	rdn_value = strdup(strtok(NULL,","));

Given the warning in the Linux strtok manpage, this worries me...

> +	DEBUG(5,("smbldap_modify: dn => [%s]\n", dn ));
> +	
> +        for (i = 0; attrs[i] != NULL; i++) {
> +		if ( ( attrs[i]->mod_op == LDAP_MOD_DELETE )
> +			&&  strequal(attrs[i]->mod_type,rdn_attribut) ) {
> +			for (j=0;attrs[i]->mod_values[j] != NULL; j++) {
> +				if (strequal(attrs[i]->mod_values[j],rdn_value)) {

We have to watch what character set this is in.  We probably need a
strequal_utf8() (or if this must be the same case, a normal strcmp()
call... )

> +					SAFE_FREE(attrs[i]->mod_values[j]);
> +					attrs[i]->mod_values[j]=attrs[i]->mod_values[j+1];
> +					for (k=j+1;attrs[i]->mod_values[k] != NULL; k++)
> +						attrs[i]->mod_values[k]=attrs[i]->mod_values[k+1];
> +					naming_deleted = True;
> +				} else {
> +					naming_more_value = True;
> +				}
> +			}
> +
> +			if (!naming_more_value) {
> +				SAFE_FREE(attrs[i]->mod_type);
> +				for (j=0;attrs[i]->mod_values[j] != NULL; j++)
> +					SAFE_FREE(attrs[i]->mod_values[j]);
> +				SAFE_FREE(attrs[i]->mod_values);
> +				SAFE_FREE(attrs[i]); 
> +				attrs[i]=attrs[i+1];
> +				for (j=i+1; attrs[j] != NULL; j++) {
> +					attrs[j]=attrs[j+1];
> +				}
> +			}
> +		}
> +
> +		if ( ( attrs[i] != NULL ) 
> +		      && ( ( attrs[i]->mod_op == LDAP_MOD_ADD && naming_deleted ) ||
> attrs[i]->mod_op == LDAP_MOD_REPLACE )
> +		      && ( attrs[i]->mod_values[0] != NULL)
> +		      && ( strequal(attrs[i]->mod_type,rdn_attribut) ) ) {
> +			do_rename = True;
> +			new_rdn_value = strdup(attrs[i]->mod_values[0]);
> +			if (!new_rdn_value) {
> +				SAFE_FREE(rdn_attribut);
> +				return LDAP_NO_MEMORY;
> +			}
> +			
> +			new_rdn_len= strlen(rdn_attribut) + strlen(new_rdn_value) + 2;
> +			new_rdn = malloc( new_rdn_len );
> +			if (!new_rdn) {
> +				SAFE_FREE(rdn_attribut);
> +				SAFE_FREE(new_rdn_value);
> +				return LDAP_NO_MEMORY;
> +			}
> +
> +			new_rdn[0] = '\0';
> +			safe_strcat( new_rdn, rdn_attribut, new_rdn_len);
> +			safe_strcat( new_rdn, "=" , new_rdn_len);
> +			safe_strcat( new_rdn, new_rdn_value, new_rdn_len);

This is what asprintf() is for.

> +			if (push_utf8_allocate(&utf8_new_rdn, new_rdn) == (size_t)-1) {
> +				SAFE_FREE(rdn_attribut);
> +				SAFE_FREE(new_rdn_value);
> +				SAFE_FREE(new_rdn);
> +				return LDAP_NO_MEMORY;
> +			}
> +			
> +			new_dn_len=strlen(new_rdn);
> +			for(j=1; rdn[j] != NULL; j++) {
> +				new_dn_len += strlen(rdn[j]) + 1;
> +			}
> +			new_dn=malloc(new_dn_len + 1);
> +			if (!new_dn) {
> +				SAFE_FREE(rdn_attribut);
> +				SAFE_FREE(new_rdn_value);
> +				SAFE_FREE(new_rdn);
> +				SAFE_FREE(utf8_new_rdn);
> +				return LDAP_NO_MEMORY;
> +			}
> +			new_dn[0]='\0';
> +			safe_strcat(new_dn,new_rdn,new_dn_len);
> +			for(j=1; rdn[j] != NULL; j++) {
> +				safe_strcat(new_dn,",",new_dn_len);
> +				safe_strcat(new_dn,rdn[j],new_dn_len);
> +			}

I realise this is 'alright', but it scares me...  We really should
always safe_strcat() with a constant length, into fixed-size buffers
only.  Things like asprintf() would be better, even if we waste a
(small) amount of ram building the string.

> +			if (push_utf8_allocate(&utf8_new_dn, new_dn) == (size_t)-1) {
> +				SAFE_FREE(rdn_attribut);
> +				SAFE_FREE(new_rdn_value);
> +				SAFE_FREE(new_rdn);
> +				SAFE_FREE(new_dn);
> +				SAFE_FREE(utf8_new_rdn);
> +				return LDAP_NO_MEMORY;

With that many SAFE_FREE() calls, I think it's time to move this whole
function to talloc()...

Apart from that, this patch looks good - you certainly seem to know what
you are doing.  We just need have this patch cleaned up so that others
can also understand it :-)  (And I'm sure somebody will call me on my
code at some point here...)

This might finally mean we can rename users in Samba - something that
has been broken for *way* too long... (and still breaks with tdbsam and
smbpasswd.  Both could/should also be fixed...)

Andrew Bartlett

-- 
Andrew Bartlett                                 abartlet at pcug.org.au
Manager, Authentication Subsystems, Samba Team  abartlet at samba.org
Student Network Administrator, Hawker College   abartlet at hawkerc.net
http://samba.org     http://build.samba.org     http://hawkerc.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20040220/1ab2eddf/attachment.bin