diff mbox series

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

Message ID CAEqktHuJ7T8R6CmYd-R7tufUPdLBT22S6G3_-_PG9s9c_4t5EA@mail.gmail.com
State New
Headers show
Series Bug in ulogd2 when destroying a stack that failed to start (with fix attached) | expand

Commit Message

Gérald Colangelo Dec. 14, 2023, 5:10 p.m. UTC 
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,
diff mbox series

Patch

--- 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 */