From 64e5de4c8cc45a4a35dc0f7ccdc320d975324266 Mon Sep 17 00:00:00 2001
From: Bram Matthys <syzop@vulnscan.org>
Date: Sun, 1 Jan 2023 09:40:38 +0100
Subject: [PATCH] ExtBanAdd: Actually enforce conv_param as a required event.
 This was documented as optional in include/modules.h but on
 https://www.unrealircd.org/docs/Dev:Extended_Bans_API it was always mentioned
 as required. In practice, I know of no module that does not have this, in
 UnrealIRCd or third party (doing zero filtering is quite a bad idea).
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Anyway, long story short: this also means we can remove some
(flawed) logic in src/api-extban.c in case conv_param was
NULL, which raised a compiler warning:

api-extban.c: In function ‘extban_conv_param_nuh_or_extban’:
cc1: error: function may return address of local variable [-Werror=return-local-addr]
api-extban.c:382:14: note: declared here
  382 |         char tmpbuf[USERLEN + NICKLEN + HOSTLEN + 32];
      |              ^~~~~~
---
 include/modules.h |  2 +-
 src/api-extban.c  | 33 ++++++++++++++-------------------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/include/modules.h b/include/modules.h
index 38e8a1112..74ecc678e 100644
--- a/include/modules.h
+++ b/include/modules.h
@@ -434,7 +434,7 @@ struct Extban {
 
 	int (*is_ok)(BanContext *b);
 
-	/** Convert input parameter to output [optional].
+	/** Convert input parameter to output.
 	 * like with normal bans '+b blah' gets '+b blah!*@*', and it allows
 	 * you to limit the length of the ban too.
 	 * return value: pointer to output string (temp. storage)
diff --git a/src/api-extban.c b/src/api-extban.c
index 58c1d6e71..c38b15ee9 100644
--- a/src/api-extban.c
+++ b/src/api-extban.c
@@ -169,6 +169,15 @@ Extban *ExtbanAdd(Module *module, ExtbanInfo req)
 		return NULL;
 	}
 
+	if (!req.conv_param)
+	{
+		module->errorcode = MODERR_INVALID;
+		unreal_log(ULOG_ERROR, "module", "EXTBANADD_API_ERROR", NULL,
+			   "ExtbanAdd(): conv_param event missing. Module: $module_name",
+			   log_data_string("module_name", module->header->name));
+		return NULL;
+	}
+
 	for (e=extbans; e; e = e->next)
 	{
 		if (e->letter == req.letter)
@@ -420,25 +429,11 @@ const char *extban_conv_param_nuh_or_extban(BanContext *b, Extban *self_extban)
 		return NULL;
 	}
 
-	if (extban->conv_param)
-	{
-		//BanContext *b = safe_alloc(sizeof(BanContext));
-		//b->banstr = mask; <-- this is redundant right? we can use existing 'b' context??
-		extban_recursion++;
-		ret = extban->conv_param(b, extban);
-		extban_recursion--;
-		ret = prefix_with_extban(ret, b, extban, retbuf, sizeof(retbuf));
-		//safe_free(b);
-		return ret;
-	}
-	/* I honestly don't know what the deal is with the 80 char cap in clean_ban_mask is about. So I'm leaving it out here. -- aquanight */
-	/* I don't know why it's 80, but I like a limit anyway. A ban of 500 characters can never be good... -- Syzop */
-	if (strlen(b->banstr) > 80)
-	{
-		strlcpy(retbuf, b->banstr, 128);
-		return retbuf;
-	}
-	return b->banstr;
+	extban_recursion++;
+	ret = extban->conv_param(b, extban);
+	extban_recursion--;
+	ret = prefix_with_extban(ret, b, extban, retbuf, sizeof(retbuf));
+	return ret;
 }
 
 char *prefix_with_extban(const char *remainder, BanContext *b, Extban *extban, char *buf, size_t buflen)