Redesign m_ldap to no longer rely on undefined behavior

Accessing the same LDAP* from multiple threads at once is always
undefined, even if one thread is just polling ldap_result.

Instead keep one thread per connection and issue blocking queries on the
thread.

1 files changed, 44 insertions, 92 deletions

diff --git a/modules/extra/m_ldap_authentication.cpp b/modules/extra/m_ldap_authentication.cpp
index 01bafac45..23f47c5e6 100644
--- a/modules/extra/m_ldap_authentication.cpp
+++ b/modules/extra/m_ldap_authentication.cpp
@@ -30,36 +30,29 @@ struct IdentifyInfo
 
 class IdentifyInterface : public LDAPInterface
 {
-	std::map<LDAPQuery, IdentifyInfo *> requests;
+	IdentifyInfo *ii;
 
  public:
-	IdentifyInterface(Module *m) : LDAPInterface(m) { }
+	IdentifyInterface(Module *m, IdentifyInfo *i) : LDAPInterface(m), ii(i) { }
 
-	void Add(LDAPQuery id, IdentifyInfo *ii)
+	~IdentifyInterface()
 	{
-		std::map<LDAPQuery, IdentifyInfo *>::iterator it = this->requests.find(id);
-		if (it != this->requests.end())
-			delete it->second;
-		this->requests[id] = ii;
+		delete ii;
 	}
 
-	void OnResult(const LDAPResult &r) anope_override
+	void OnDelete() anope_override
 	{
-		std::map<LDAPQuery, IdentifyInfo *>::iterator it = this->requests.find(r.id);
-		if (it == this->requests.end())
-			return;
-		IdentifyInfo *ii = it->second;
-		this->requests.erase(it);
+		delete this;
+	}
 
+	void OnResult(const LDAPResult &r) anope_override
+	{
 		if (!ii->lprov)
-		{
-			delete ii;
 			return;
-		}
 
 		switch (r.type)
 		{
-			case LDAPResult::QUERY_SEARCH:
+			case QUERY_SEARCH:
 			{
 				if (!r.empty())
 				{
@@ -68,9 +61,9 @@ class IdentifyInterface : public LDAPInterface
 						const LDAPAttributes &attr = r.get(0);
 						ii->dn = attr.get("dn");
 						Log(LOG_DEBUG) << "m_ldap_authenticationn: binding as " << ii->dn;
-						LDAPQuery id = ii->lprov->Bind(this, ii->dn, ii->req->GetPassword());
-						this->Add(id, ii);
-						return;
+
+						ii->lprov->Bind(new IdentifyInterface(this->owner, ii), ii->dn, ii->req->GetPassword());
+						ii = NULL;
 					}
 					catch (const LDAPException &ex)
 					{
@@ -79,7 +72,7 @@ class IdentifyInterface : public LDAPInterface
 				}
 				break;
 			}
-			case LDAPResult::QUERY_BIND:
+			case QUERY_BIND:
 			{
 				if (ii->admin_bind)
 				{
@@ -87,10 +80,9 @@ class IdentifyInterface : public LDAPInterface
 					try
 					{
 						Log(LOG_DEBUG) << "m_ldap_authentication: searching for " << sf;
-						LDAPQuery id = ii->lprov->Search(this, basedn, sf);
-						this->Add(id, ii);
+						ii->lprov->Search(new IdentifyInterface(this->owner, ii), basedn, sf);
 						ii->admin_bind = false;
-						return;
+						ii = NULL;
 					}
 					catch (const LDAPException &ex)
 					{
@@ -120,40 +112,28 @@ class IdentifyInterface : public LDAPInterface
 			default:
 				break;
 		}
-
-		delete ii;
 	}
 
 	void OnError(const LDAPResult &r) anope_override
 	{
-		std::map<LDAPQuery, IdentifyInfo *>::iterator it = this->requests.find(r.id);
-		if (it == this->requests.end())
-			return;
-		IdentifyInfo *ii = it->second;
-		this->requests.erase(it);
-		delete ii;
 	}
 };
 
 class OnIdentifyInterface : public LDAPInterface
 {
-	std::map<LDAPQuery, Anope::string> requests;
+	Anope::string uid;
 
  public:
-	OnIdentifyInterface(Module *m) : LDAPInterface(m) { }
+	OnIdentifyInterface(Module *m, const Anope::string &i) : LDAPInterface(m), uid(i) { }
 
-	void Add(LDAPQuery id, const Anope::string &nick)
+	void OnDelete() anope_override
 	{
-		this->requests[id] = nick;
+		delete this;
 	}
 
 	void OnResult(const LDAPResult &r) anope_override
 	{
-		std::map<LDAPQuery, Anope::string>::iterator it = this->requests.find(r.id);
-		if (it == this->requests.end())
-			return;
-		User *u = User::Find(it->second);
-		this->requests.erase(it);
+		User *u = User::Find(uid);
 
 		if (!u || !u->Account() || r.empty())
 			return;
@@ -180,7 +160,6 @@ class OnIdentifyInterface : public LDAPInterface
 
 	void OnError(const LDAPResult &r) anope_override
 	{
-		this->requests.erase(r.id);
 		Log(this->owner) << r.error;
 	}
 };
@@ -201,11 +180,9 @@ class OnRegisterInterface : public LDAPInterface
 	}
 };
 
-class NSIdentifyLDAP : public Module
+class ModuleLDAPAuthentication : public Module
 {
 	ServiceReference<LDAPProvider> ldap;
-	IdentifyInterface iinterface;
-	OnIdentifyInterface oninterface;
 	OnRegisterInterface orinterface;
 
 	PrimitiveExtensibleItem<Anope::string> dn;
@@ -214,11 +191,10 @@ class NSIdentifyLDAP : public Module
 	Anope::string disable_register_reason;
 	Anope::string disable_email_reason;
  public:
-	NSIdentifyLDAP(const Anope::string &modname, const Anope::string &creator) :
-		Module(modname, creator, EXTRA | VENDOR), ldap("LDAPProvider", "ldap/main"), iinterface(this), oninterface(this), orinterface(this),
+	ModuleLDAPAuthentication(const Anope::string &modname, const Anope::string &creator) :
+		Module(modname, creator, EXTRA | VENDOR), ldap("LDAPProvider", "ldap/main"), orinterface(this),
 		dn(this, "m_ldap_authentication_dn")
 	{
-
 		me = this;
 	}
 
@@ -271,16 +247,7 @@ class NSIdentifyLDAP : public Module
 			return;
 
 		IdentifyInfo *ii = new IdentifyInfo(u, req, this->ldap);
-		try
-		{
-			LDAPQuery id = this->ldap->BindAsAdmin(&this->iinterface);
-			this->iinterface.Add(id, ii);
-		}
-		catch (const LDAPException &ex)
-		{
-			delete ii;
-			Log(this) << ex.GetReason();
-		}
+		this->ldap->BindAsAdmin(new IdentifyInterface(this, ii));
 	}
 
 	void OnNickIdentify(User *u) anope_override
@@ -292,15 +259,7 @@ class NSIdentifyLDAP : public Module
 		if (!d || d->empty())
 			return;
 
-		try
-		{
-			LDAPQuery id = this->ldap->Search(&this->oninterface, *d, "(" + email_attribute + "=*)");
-			this->oninterface.Add(id, u->nick);
-		}
-		catch (const LDAPException &ex)
-		{
-			Log(this) << ex.GetReason();
-		}
+		this->ldap->Search(new OnIdentifyInterface(this, u->GetUID()), *d, "(" + email_attribute + "=*)");
 	}
 
 	void OnNickRegister(User *, NickAlias *na, const Anope::string &pass) anope_override
@@ -308,37 +267,30 @@ class NSIdentifyLDAP : public Module
 		if (!this->disable_register_reason.empty() || !this->ldap)
 			return;
 
-		try
-		{
-			this->ldap->BindAsAdmin(NULL);
-
-			LDAPMods attributes;
-			attributes.resize(4);
-
-			attributes[0].name = "objectClass";
-			attributes[0].values.push_back("top");
-			attributes[0].values.push_back(object_class);
+		this->ldap->BindAsAdmin(NULL);
 
-			attributes[1].name = username_attribute;
-			attributes[1].values.push_back(na->nick);
+		LDAPMods attributes;
+		attributes.resize(4);
 
-			if (!na->nc->email.empty())
-			{
-				attributes[2].name = email_attribute;
-				attributes[2].values.push_back(na->nc->email);
-			}
+		attributes[0].name = "objectClass";
+		attributes[0].values.push_back("top");
+		attributes[0].values.push_back(object_class);
 
-			attributes[3].name = this->password_attribute;
-			attributes[3].values.push_back(pass);
+		attributes[1].name = username_attribute;
+		attributes[1].values.push_back(na->nick);
 
-			Anope::string new_dn = username_attribute + "=" + na->nick + "," + basedn;
-			this->ldap->Add(&this->orinterface, new_dn, attributes);
-		}
-		catch (const LDAPException &ex)
+		if (!na->nc->email.empty())
 		{
-			Log(this) << ex.GetReason();
+			attributes[2].name = email_attribute;
+			attributes[2].values.push_back(na->nc->email);
 		}
+
+		attributes[3].name = this->password_attribute;
+		attributes[3].values.push_back(pass);
+
+		Anope::string new_dn = username_attribute + "=" + na->nick + "," + basedn;
+		this->ldap->Add(&this->orinterface, new_dn, attributes);
 	}
 };
 
-MODULE_INIT(NSIdentifyLDAP)
+MODULE_INIT(ModuleLDAPAuthentication)