Bug in ulogd2 when destroying a stack that failed to start (with fix attached)

Bug in ulogd2 when destroying a stack that failed to start (with fix attached)

[Gérald Colangelo]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Hello everyone,

While working on a ulogd2 deployment, i built a stack that consists in:
  - A "home-made" INPUT plugin that launchs a pcap capture upon
start() and passes a fd to ulogd2.
  - Some regular ulogd2 filters
  - JSON output plugin that writes to a Unix socket.

Everything works fine, except if unix socket is not available.
In that case, when the stack is started at launch, the JSON output
start() function returns -1 and  ulogd.c consider that it fails
starting the stack and then destroy it.
Unfortunately, ulogd.c only free() the stack context without calling
stop() functions on plugins that were correctly start()-ed.
In my case, it results in my pcap input triggering the stack even if
it was destroyed.
This ends with a segmentation fault for the ulogd process.

I think we should consider calling stop() functions before destroying the stack.
A patch implementing this is attached (for version 2.0.7, but this
part of the code didn't changed on latest).

Best regards,

-- 
Gerald Colangelo

[-- Attachment #2: ulogd2_stop_plugins_before_destroying_stack.patch --]
[-- Type: text/x-patch, Size: 972 bytes --]

--- ulogd2-2.0.7/src/ulogd.c	2018-04-27 01:10:42.316872034 +0200
+++ ulogd2-2.0.7-patched/src/ulogd.c	2023-12-14 18:04:06.994661722 +0100
@@ -931,7 +931,7 @@
 static int create_stack_start_instances(struct ulogd_pluginstance_stack *stack)
 {
 	int ret;
-	struct ulogd_pluginstance *pi;
+	struct ulogd_pluginstance *pi, *stop;
 
 	/* start from input to output plugin */
 	llist_for_each_entry(pi, &stack->list, list) {
@@ -945,11 +945,27 @@
 				ulogd_log(ULOGD_ERROR, 
 					  "error starting `%s'\n",
 					  pi->id);
-				return ret;
+				goto cleanup_fail;
 			}
 		}
 	}
 	return 0;
+cleanup_fail:
+	stop = pi;
+	llist_for_each_entry(pi, &stack->list, list) {
+		if (pi == stop)
+			/* the one that failed, stops the cleanup here */
+			break;
+		if (!pi->plugin->stop) 
+			continue;
+		ret = pi->plugin->stop(pi);
+		if (ret < 0) {
+			ulogd_log(ULOGD_ERROR,
+			"error stopping `%s'\n",
+			pi->id);
+		}
+	}
+	return -1;
 }
 
 /* create a new stack of plugins */

Re: Bug in ulogd2 when destroying a stack that failed to start (with fix attached)

[Florian Westphal]

Gérald Colangelo <gerald.colangelo@weblib.eu> wrote:
> +cleanup_fail:
> +	stop = pi;
> +	llist_for_each_entry(pi, &stack->list, list) {
> +		if (pi == stop)
> +			/* the one that failed, stops the cleanup here */
> +			break;
> +		if (!pi->plugin->stop) 
> +			continue;
> +		ret = pi->plugin->stop(pi);
> +		if (ret < 0) {
> +			ulogd_log(ULOGD_ERROR,
> +			"error stopping `%s'\n",
> +			pi->id);
> +		}
> +	}
> +	return -1;

Looks good, but I think you also need to add a second loop to free()
the stack elements, as done in stop_pluginstances().