Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

promised fix for pagedSearch and few other problems I noticed #11

Merged
merged 4 commits into from

2 participants

@huancz

No description provided.

Petr Běhan added some commits
Petr Běhan fix unfinished error code paths introduced with pagedSearch 5b61d43
Petr Běhan misc documentation fixes 994028e
Petr Běhan bugfix: wrong interpretation of ldap_result return value
didn't cause problems because LDAP.js ignores 'disconnected' event now
102c2de
Petr Běhan fix suspicious use of ldap file descriptors
One problem: when openldap library handles reconnect and succeeds, there
is no guarantee that the file descriptor will stay the same - but binding
didn't detect this.

Second problem: calling ev_io_set on c->read_watcher_ that may be active is
explicitly forbidden in libev docs.
217d879
@jeremycx jeremycx merged commit 99d0dfb into jeremycx:master
@jeremycx
Owner

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 6, 2011
  1. misc documentation fixes

    Petr Běhan authored
  2. bugfix: wrong interpretation of ldap_result return value

    Petr Běhan authored
    didn't cause problems because LDAP.js ignores 'disconnected' event now
  3. fix suspicious use of ldap file descriptors

    Petr Běhan authored
    One problem: when openldap library handles reconnect and succeeds, there
    is no guarantee that the file descriptor will stay the same - but binding
    didn't detect this.
    
    Second problem: calling ev_io_set on c->read_watcher_ that may be active is
    explicitly forbidden in libev docs.
This page is out of date. Refresh to see the latest.
Showing with 90 additions and 38 deletions.
  1. +15 −13 README.md
  2. +75 −25 src/LDAP.cc
View
28 README.md
@@ -47,7 +47,7 @@ Open Example
throw new Error("Unable to connect to server");
}
-Connection.SimpleBind([dn, password,] callback(msgid, err));
+Connection.simpleBind([dn, password,] callback(msgid, err));
-----------------------------------
Authenticates to the server. When the response is ready, or the
@@ -55,7 +55,7 @@ timeout occurs, will execute the callback with the error value set.
dn and password must be omited, when doing anonymous bind.
-Connection.Search(base, scope, filter, attrs, function(msgid, err, data))
+Connection.search(base, scope, filter, attrs, function(msgid, err, data))
---------------------------------------------
Searches LDAP within the given base for entries matching the given
@@ -72,26 +72,28 @@ Scopes are specified as one of the following integers:
If a disconnect or other server error occurs, the backing library will
attempt to reconnect automatically, and if this reconnection fails,
-Connection.Open() will return -1.
+Connection.open() will return -1.
See also "man 3 ldap" for details.
-Connection.SearchPaged(base, scope, filter, attrs, pageSize, function(msgid, err, data) [, cookie])
+Connection.searchPaged(base, scope, filter, attrs, pageSize, function(msgid, err, data) [, cookie])
---------------------------------------------------------------------------------------------------
-LDAP servers are usually limited in how many items they are willing to return - 1024 or 4096 are
-some typical values. For larger LDAP directories, you need to either partition your results
-with filter, or use paged search.
+LDAP servers are usually limited in how many items they are willing to return -
+1024 or 4096 are some typical values. For larger LDAP directories, you need to
+either partition your results with filter, or use paged search.
-Note that it's only extension to the protocol, server doesn't have to support it. In such
-case, callback will be called with nonzero err (actually, it would be nice if someone could
-verify this, the server it was tested on had this feature).
+Note that it's only extension to the protocol, server doesn't have to support
+it. In such case, callback will be called with nonzero err (actually, it would
+be nice if someone could verify this, the server it was tested on had this
+feature).
Cookie parameter is only for internal use, leave it undefined in your calls.
-Results are passed to callback function as they arrive in the same format as for simple Search.
-Request for next page is sent only after the callback returns. After all data has arrived,
-callback is called once more, with data equal to null.
+Results are passed to callback function as they arrive in the same format
+as for simple search. Request for next page is sent only after the callback
+returns. After all data has arrived, callback is called once more, with data
+equal to null.
Search Example
--------------
View
100 src/LDAP.cc
@@ -115,8 +115,9 @@ class LDAPConnection : public EventEmitter
LDAPConnection * c = new LDAPConnection();
c->Wrap(args.This());
- ev_init(&(c->read_watcher_), c->io_event);
+ ev_init(&(c->read_watcher_), LDAPConnection::io_event);
c->read_watcher_.data = c;
+ c->read_watcher_.fd = -1;
c->ld = NULL;
@@ -194,21 +195,24 @@ class LDAPConnection : public EventEmitter
ARG_STR(attrs_str, 3);
if (args.Length() >= 5) {
+ // process optional arguments: [, pageSize [, cookie] ]
ENFORCE_ARG_NUMBER(4);
page_size = args[4]->Int32Value();
- if (args.Length() >= 6) {
- if (args[5]->IsObject()) {
- cookieObj = args[5]->ToObject();
- if (cookieObj->InternalFieldCount() != 1
- || (cookie = reinterpret_cast<berval*>(cookieObj->GetPointerFromInternalField(0)) ) == NULL)
- {
- // TODO throw
- }
- // TODO: check that parameters match to those passed to first call
- cookieObj->SetPointerInInternalField(0, NULL);
- } else {
- // TODO throw
+ if (args.Length() >= 6 && !args[5]->IsUndefined()) {
+ // we have the cookie, too
+ if (!args[5]->IsObject()) {
+ THROW("invalid cookie object for paged search");
+ }
+ cookieObj = args[5]->ToObject();
+ if (cookieObj->InternalFieldCount() != 1) {
+ THROW("invalid cookie object for paged search");
}
+ cookie = static_cast<berval*>(
+ cookieObj->GetPointerFromInternalField(0));
+ if (cookie == NULL) {
+ THROW("invalid cookie object for paged search");
+ }
+ cookieObj->SetPointerInInternalField(0, NULL);
}
}
@@ -238,10 +242,13 @@ class LDAPConnection : public EventEmitter
cookie = NULL;
}
if (rc != LDAP_SUCCESS) {
- abort();
- // TODO: free other stuff, throw ?
+ free(bufhead);
+ RETURN_INT(-1);
}
+ } else if (cookie) {
+ ber_bvfree(cookie);
+ cookie = NULL;
}
rc = ldap_search_ext(c->ld, *base, searchscope, *filter, attrs, 0,
@@ -254,8 +261,13 @@ class LDAPConnection : public EventEmitter
msgid = -1;
} else {
ldap_get_option(c->ld, LDAP_OPT_DESC, &fd);
- ev_io_set(&(c->read_watcher_), fd, EV_READ);
- ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ if (c->read_watcher_.fd != fd) {
+ if (ev_is_active(&c->read_watcher_)) {
+ ev_io_stop(&c->read_watcher_);
+ }
+ ev_io_set(&(c->read_watcher_), fd, EV_READ);
+ ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ }
}
free(bufhead);
@@ -395,8 +407,13 @@ class LDAPConnection : public EventEmitter
msgid = ldap_add(c->ld, *dn, ldapmods);
ldap_get_option(c->ld, LDAP_OPT_DESC, &fd);
- ev_io_set(&(c->read_watcher_), fd, EV_READ);
- ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ if (c->read_watcher_.fd != fd) {
+ if (ev_is_active(&c->read_watcher_)) {
+ ev_io_stop(&c->read_watcher_);
+ }
+ ev_io_set(&(c->read_watcher_), fd, EV_READ);
+ ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ }
if (msgid == LDAP_SERVER_DOWN) {
c->Emit(symbol_disconnected, 0, NULL);
@@ -436,8 +453,13 @@ class LDAPConnection : public EventEmitter
}
ldap_get_option(c->ld, LDAP_OPT_DESC, &fd);
- ev_io_set(&(c->read_watcher_), fd, EV_READ);
- ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ if (c->read_watcher_.fd != fd) {
+ if (ev_is_active(&c->read_watcher_)) {
+ ev_io_stop(&c->read_watcher_);
+ }
+ ev_io_set(&(c->read_watcher_), fd, EV_READ);
+ ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ }
RETURN_INT(msgid);
@@ -472,8 +494,13 @@ class LDAPConnection : public EventEmitter
c->Emit(symbol_disconnected, 0, NULL);
} else {
ldap_get_option(c->ld, LDAP_OPT_DESC, &fd);
- ev_io_set(&(c->read_watcher_), fd, EV_READ);
- ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ if (c->read_watcher_.fd != fd) {
+ if (ev_is_active(&c->read_watcher_)) {
+ ev_io_stop(&c->read_watcher_);
+ }
+ ev_io_set(&(c->read_watcher_), fd, EV_READ);
+ ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ }
}
free(binddn);
@@ -550,7 +577,30 @@ class LDAPConnection : public EventEmitter
return;
}
- if ((res = ldap_result(c->ld, LDAP_RES_ANY, 1, &ldap_tv, &ldap_res)) < 1) {
+ res = ldap_result(c->ld, LDAP_RES_ANY, 1, &ldap_tv, &ldap_res);
+ {
+ // if ldap silently handled reconnect, fd may now be different
+ int fd = -1;
+ ldap_get_option(c->ld, LDAP_OPT_DESC, &fd);
+ if (c->read_watcher_.fd != fd) {
+ if (ev_is_active(&c->read_watcher_)) {
+ ev_io_stop(&c->read_watcher_);
+ }
+ ev_io_set(&(c->read_watcher_), fd, EV_READ);
+ ev_io_start(EV_DEFAULT_ &(c->read_watcher_));
+ }
+ }
+
+ if (res == 0) {
+ // No complete messages were available. In theory, this will happen when
+ // reply consists of more packets, and not all have arrived yet.
+ //
+ // In practice, code ends here over 10 times per result packet, even for
+ // searches where answer consists only of two packets. Not sure why,
+ // might be worth investigating, hiting this case too often could harm
+ // performance a bit.
+ return;
+ } else if (res < 0) {
c->Emit(symbol_disconnected, 0, NULL);
return;
}
@@ -586,6 +636,7 @@ class LDAPConnection : public EventEmitter
struct berval* cookie = NULL;
ldap_parse_page_control(c->ld, srv_controls, NULL, &cookie);
if (!cookie || cookie->bv_val == NULL || !*cookie->bv_val) {
+ // no more paged results, signal end to user code
if (cookie) {
ber_bvfree(cookie);
}
@@ -593,7 +644,6 @@ class LDAPConnection : public EventEmitter
} else {
Local<Object> cookieObj(cookie_template->NewInstance());
cookieObj->SetPointerInInternalField(0, cookie);
- cookieObj->Set(v8::String::New("cookie"), v8::String::New("cookie") );
args[3] = cookieObj;
}
c->Emit(symbol_search_paged, 4, args);
Something went wrong with that request. Please try again.