socket.c: plugin/plugout SIGSEGV and memleak fixes

2 files changed, 30 insertions, 12 deletions

diff --git a/NEWS b/NEWS
index 41db8457..5bdca44b 100644
--- a/NEWS
+++ b/NEWS
@@ -112,6 +112,10 @@ fetchmail-6.4.22 (not yet released):
   command continuation)
 * On IMAP connections, When --auth external is requested but not advertised by 
   the server, log a proper error message.
+* Fetchmail no longer crashes when attempting a connection with --plugin "" or 
+  --plugout "".
+* Fetchmail no longer leaks memory when processing the arguments of --plugin or 
+  --plugout on connections.
 
 # CHANGES:
 * IMAP: When fetchmail is in not-authenticated state and the server volunteers 
diff --git a/socket.c b/socket.c
index 326dc9cb..d844a33b 100644
--- a/socket.c
+++ b/socket.c
@@ -82,7 +82,20 @@ extern int h_errno;
 #endif /* ndef h_errno */
 
 #ifdef HAVE_SOCKETPAIR
-static char *const *parse_plugin(const char *plugin, const char *host, const char *service)
+static void free_plugindata(char **argvec)
+{
+    if (argvec) {
+	xfree(*argvec);
+	xfree(argvec);
+    }
+}
+
+/** parse plugin and interpolate %h and %p with single-quoted host and service.  
+ * Returns a malloc()ed pointer to a NULL-terminated vector of pointers, of 
+ * which the first is also malloc()ed and the 2nd and later ones (if present) 
+ * are pointers into the same memory region - these serve as input for the 
+ * argument vector of execvp() in handle_plugin. */
+static char **parse_plugin(const char *plugin, const char *host, const char *service)
 {
 	char **argvec;
 	const char *c, *p;
@@ -106,12 +119,7 @@ static char *const *parse_plugin(const char *plugin, const char *host, const cha
 
 	/* we need to discount 2 bytes for each placeholder */
 	plugin_copy_len = plugin_len + (host_len - 2) * host_count + (service_len - 2) * service_count;
-	plugin_copy = (char *)malloc(plugin_copy_len + 1);
-	if (!plugin_copy)
-	{
-		report(stderr, GT_("fetchmail: malloc failed\n"));
-		return NULL;
-	}
+	plugin_copy = (char *)xmalloc(plugin_copy_len + 1);
 
 	while (plugin_offset < plugin_len && plugin_copy_offset < plugin_copy_len)
 	{	if ((plugin[plugin_offset] == '%') && (plugin[plugin_offset + 1] == 'h'))
@@ -142,6 +150,7 @@ static char *const *parse_plugin(const char *plugin, const char *host, const cha
 		return NULL;
 	}
 	memset(argvec, 0, vecsiz);
+	argvec[0] = plugin_copy; /* make sure we can free() it in every case */
 	for (p = cp = plugin_copy, i = 0; *cp; cp++)
 	{	if ((!isspace((unsigned char)*cp)) && (cp == p ? 1 : isspace((unsigned char)*p))) {
 			argvec[i] = cp;
@@ -161,21 +170,29 @@ static int handle_plugin(const char *host,
 /* get a socket mediated through a given external command */
 {
     int fds[2];
-    char *const *argvec;
+    char **argvec;
 
     /*
      * The author of this code, Felix von Leitner <felix@convergence.de>, says:
      * he chose socketpair() instead of pipe() because socketpair creates 
      * bidirectional sockets while allegedly some pipe() implementations don't.
      */
+    argvec = parse_plugin(plugin,host,service);
+    if (!argvec || !*argvec[0]) {
+	free_plugindata(argvec);
+	report(stderr, GT_("fetchmail: plugin for host %s service %s is empty, cannot run!\n"), host, service);
+	return -1;
+    }
     if (socketpair(AF_UNIX,SOCK_STREAM,0,fds))
     {
 	report(stderr, GT_("fetchmail: socketpair failed\n"));
+	free_plugindata(argvec);
 	return -1;
     }
     switch (fork()) {
 	case -1:
 		/* error */
+		free_plugindata(argvec);
 		report(stderr, GT_("fetchmail: fork failed\n"));
 		return -1;
 	case 0:	/* child */
@@ -190,15 +207,12 @@ static int handle_plugin(const char *host,
 		(void) close(fds[0]);
 		if (outlevel >= O_VERBOSE)
 		    report(stderr, GT_("running %s (host %s service %s)\n"), plugin, host, service);
-		argvec = parse_plugin(plugin,host,service);
-		if (argvec == NULL)
-			_exit(EXIT_FAILURE);
 		execvp(*argvec, argvec);
 		report(stderr, GT_("execvp(%s) failed\n"), *argvec);
 		_exit(EXIT_FAILURE);
 		break;
 	default:	/* parent */
-		/* NOP */
+		free_plugindata(argvec);
 		break;
     }
     /* fds[0] is the child's end; close it for proper EOF detection */