inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

`with_timeout` does not work as expected with search #63

Closed slavak closed 3 years ago

slavak commented 3 years ago

The with_timeout method should cause the next operation to run with the specified timeout. When that operation is a call to search this does not work as expected. Specifically, because internally search is implemented as a wrapper around streaming_search_with, it seems only a part of the required operations are actually executed with the specified timeout.

If I use iptables to DROP packets to the LDAP port before running the search operation, the search will block forever, despite being ostensibly run with a timeout.

The stream.ldap.timeout field gets reset to None by the take in Ldap::op_call (line 121 in ldap.rs), called from SearchSteam::start_inner, called from SearchStream::start. Then, SearchStream::next_inner executes an operation using the timeout from self.ldap.timeout, which is now None.

I suspect the bug is that SearchStream::next_inner should be using self.timeout, and not self.ldap.timeout, but I don't understand the code enough and would need some confirmation.

inejge commented 3 years ago

You're right, ldap.timeout isn't re-initialized and the connection hangs. Try this patch:

--- a/src/search.rs
+++ b/src/search.rs
@@ -663,6 +663,7 @@ where
         }
         self.ldap.op_call(LdapOp::Search(tx), req).await.map(|_| {
             self.state = StreamState::Active;
+            self.ldap.timeout = self.timeout;
         })
     }

I'll have to re-analyze adapter chaining, but the purpose of setting self.ldap.timeout to the initial value is to preserve that value for use by adapters. Using self.timeout in next_inner() may well be correct, regardless.

inejge commented 3 years ago

I went with using self.timeout (the value from the stream instance) in next_inner(), since it works as well.

slavak commented 3 years ago

Awesome, thank you!