Re: bug: Partial results from DB lookup causing parsing problems

On Mon, Oct 27, 2008 at 11:25:40AM +0000, Simon Fraser wrote:
> 
> I initially hit 'reply' rather than 'reply to all', so I'll re-send this
> to the list.
> 
> > I don't have an LDAP db handy to test against, so could
> > you please test the following change for me to see if
> > it helps your problem. If not, I'll dig deeper into the problem.
> 
> Many thanks for getting back to me so quickly.  I've patched a clean
> version of 1.17.1, and tested. 
> 
> > +     if (returns[0])
> > +             user_str = &returns[0];
> > +     if (returns[1])
> > +             server_str = &returns[1];
> > +     if (returns[2])
> > +             port_str = &returns[2];
> 
> The lookup was failing, and I've traced it to the lines above.  These
> pointers are directed to &returns, but a few lines later, all the values
> of returns are passed to free():
> 
>         for (count = 0; count < attrcount; count++)
>                 if (returns[count] != NULL)
>                         free(returns[count]);
>         free(returns);
> 
> I've replaced the lines quoted above with ones like:
> 
> if (returns[1])
>         *server_str = strdup(returns[1]);
> 
> This works fine, but my C is rusty enough that I don't know if this will
> create memory problems over time. 

Sorry about that. I think that your fix is ok, though
I think I will go with the change below that just makes
sure the strings aren't freed if we make it to the bottom
cleanly.

The key is the following snippet

       leave:
-       if (returns) {
+       if (returns && status) {
                for (count = 0; count < attrcount; count++)

I actually think that the code could be restructured to get rid of
returns[]. Which may simplify things, but on the other hand
it may make error handling trickier.

> With strdup() in place, outgoing_server is used if the LDAP directory is
> missing either username or server, but port is filled in appropriately
> if it is missing. Since a missing username means no entry in LDAP for
> that user, and the lookup falls through correctly for a missing server
> entry, this is now working as I'd hoped for.

Great.

> Incidentally, I've ventured into nasty kludge territory to work around a
> bug that appears not to be in your code, but either in libldap or
> libldap's interaction with Active Directory.  ldap_init, ldap_set_option
> and ldap_bind_s are all successful, but ldap_search_s does not return
> LDAP_SUCCESS.  Instead, it returns LDAP_SERVER_DOWN, _even if_ the
> search was successful. It seems insane, but if I comment out the check
> for (err != LDAP_SUCCESS) and just check for returned attributes, all
> the data is there.  I can replicate this in my own test program using
> libldap both from debian-etch (2.1.30) and the openldap-2.4.11 source
> package, so I'm confident the problem isn't in your code, but it's
> probably worth knowing about.

That does sound bizarre - but believable.

Could you send a patch just to confirm the change / workaround that you are
suggesting?

-- 
Simon Horman
  VA Linux Systems Japan K.K., Sydney, Australia Satellite Office
  H: www.vergenet.net/~horms/             W: www.valinux.co.jp/en

Allow the ldap db to return any one or more of: username, server and port.

This is done by switching the ldap module over to use db_getsever2() instead
of db_getserver(), which allows these elements to be returned individually,
rather than as a string that then has to be broken down into its parts -
which assumes that server is always present.

Signed-off-by: Simon Horman <[email protected]>
Index: perdition-1.17.1/perdition/db/ldap/perditiondb_ldap.c
===================================================================
--- perdition-1.17.1.orig/perdition/db/ldap/perditiondb_ldap.c	2007-10-31 13:43:25.000000000 +1100
+++ perdition-1.17.1/perdition/db/ldap/perditiondb_ldap.c	2008-10-28 11:41:52.000000000 +1100
@@ -285,25 +285,26 @@ static int pldap_scan_exts(char **exts, 
 
 
 /**********************************************************************
- * dbserver_get
+ * dbserver_get2
  * Read the server (value) from an LDAP directory given the user (key)
  * pre: key_str: Key as a null terminated string
- *      options_str: Options string. 
+ *      options_str: Options string.
  *                   Ignored if NULL
  *                   Used as the map to open otherwise
- *      str_return: value is returned here
- *      len_return: length of value is returned here
- * post: The str_key is looked up in the gdbm map and the
- *       corresponding value is returned in str_return and len_return.
+ *      user_str: string for user is returned here
+ *      server_str: string for server is returned here
+ *      port_str: string for port is returned here
+ * post: The str_key is looked up in the ldap database and the
+ *       corresponding values are returned in user_str, server_str and
+ *       port_str.
  * return:  0 on success
  *         -1 on LDAP access error
  *         -2 if key cannot be found in map
  *         -3 on other error
  **********************************************************************/
 
-int dbserver_get(const char *key_str,
-		 const char *options_str,
-		 char **str_return, int *len_return)
+int dbserver_get2(const char *key_str, const char *options_str,
+		 char **user_str, char **server_str, char **port_str)
 {
 	LDAPURLDesc *lud = NULL;
 	LDAP *connection = NULL;
@@ -322,9 +323,6 @@ int dbserver_get(const char *key_str,
 
 	extern options_t opt;
 
-	*len_return = 0;
-
-
 	/* get filter string */
 	if (pldap_get_filter(key_str, pldap_filter, &lud) < 0) {
 		VANESSA_LOGGER_DEBUG("pldap_get_filter");
@@ -432,7 +430,6 @@ int dbserver_get(const char *key_str,
 		goto leave;
 	}
 
-	*len_return = 0;
 	for (pstr = ldap_first_attribute(connection, mptr, &ber);
 	     pstr != NULL;
 	     pstr = ldap_next_attribute(connection, mptr, ber)) {
@@ -442,7 +439,6 @@ int dbserver_get(const char *key_str,
 			if (strcasecmp(lud->lud_attrs[count], pstr) != 0) {
 				continue;
 			}
-			*len_return += strlen(*bv_val);
 			if (returns[count] != NULL) {
 				free(returns[count]);
 			}
@@ -465,41 +461,18 @@ int dbserver_get(const char *key_str,
 	ber_free(ber, 0);
 	ber = NULL;
 
-	/* Add in some extra for the separators and terminating NULL */
-	*len_return += 2 + strlen(opt.domain_delimiter);
-
-	if ((*str_return = (char *) calloc(1, *len_return)) == NULL) {
-		VANESSA_LOGGER_DEBUG_ERRNO("str_return calloc");
-		status = -3;
-		goto leave;
-	}
-
 	/* Build the return string */
-	if (attrcount > 0 && returns[0]) {
-		strcat(*str_return, returns[0]);
-	}
-	if (attrcount > 1 && returns[1]) {
-		if (returns[0]) {
-			strcat(*str_return, opt.domain_delimiter);
-		}
-		strcat(*str_return, returns[1]);
-	}
-	if (attrcount > 2 && returns[2] && returns[1]) {
-		strcat(*str_return, ":");
-		strcat(*str_return, returns[2]);
-	}
-	for (count = 0; count < attrcount; count++) {
-		if (!returns[count]) {
-			continue;
-		}
-		free(returns[count]);
-		returns[count] = NULL;
-	}
+	if (returns[0])
+		user_str = &returns[0];
+	if (returns[1])
+		server_str = &returns[1];
+	if (returns[2])
+		port_str = &returns[2];
 
 	status = 0;
 
       leave:
-	if (returns) {
+	if (returns && status) {
 		for (count = 0; count < attrcount; count++)
 			if (returns[count] != NULL)
 				free(returns[count]);
Index: perdition-1.17.1/perdition/getserver.c
===================================================================
--- perdition-1.17.1.orig/perdition/getserver.c	2005-06-22 15:50:05.000000000 +1000
+++ perdition-1.17.1/perdition/getserver.c	2008-10-28 11:39:33.000000000 +1100
@@ -291,9 +291,21 @@ do_getserver(
 	}
 
 	/* Check for an empty result */
-	if(!server_str || *server_str=='\0') {
-		VANESSA_LOGGER_DEBUG("dbserver_get returned empty string");
-		goto fail;
+	if(dbserver_get) {
+		if(!server_str || *server_str=='\0') {
+			VANESSA_LOGGER_DEBUG("dbserver_get returned empty "
+					     "string");
+			goto fail;
+		}
+	}
+	else {
+		if((!user_str || *user_str=='\0') &&
+		   (!server_str || *server_str=='\0') &&
+		   (!port_str || *port_str=='\0')) {
+			VANESSA_LOGGER_DEBUG("dbserver_get returned empty "
+					     "string");
+			goto fail;
+		}
 	}
 
 	if(dbserver_get) {
______________________________________________
Perdition-users mailing list
[email protected]
http://lists.vergenet.net/listinfo/perdition-users