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 <horms(a)verge.net.au>
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) {