Do not try to close conns on the thrmgr thread after setting event callbacks and/or socket connect

Use strncpy() instead of memcpy(), to limit max size with dest buffer

protoautossl.c

@@ -111,7 +111,7 @@ protoautossl_peek_and_upgrade(pxy_conn_ctx_t *ctx)
 	return 0;
 }
 
-static void NONNULL(1)
+static int NONNULL(1)
 protoautossl_conn_connect(pxy_conn_ctx_t *ctx)
 {
 #ifdef DEBUG_PROXY
@@ -121,12 +121,12 @@ protoautossl_conn_connect(pxy_conn_ctx_t *ctx)
 #endif /* DEBUG_PROXY */
 
 	if (prototcp_setup_dst(ctx) == -1) {
-		return;
+		return -1;
 	}
 
 	/* create server-side socket and eventbuffer */
 	if (prototcp_setup_srvdst(ctx) == -1) {
-		return;
+		return -1;
 	}
 	
 	bufferevent_setcb(ctx->dst.bev, pxy_bev_readcb, pxy_bev_writecb, pxy_bev_eventcb, ctx);
@@ -143,8 +143,9 @@ protoautossl_conn_connect(pxy_conn_ctx_t *ctx)
 		log_dbg_level_printf(LOG_DBG_MODE_FINE, "protoautossl_conn_connect: bufferevent_socket_connect for srvdst failed, fd=%d\n", fd);
 #endif /* DEBUG_PROXY */
 
-		// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect. Just return.
+		// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect. Just return 0.
 	}
+	return 0;
 }
 
 static void NONNULL(1)

protopassthrough.c

@@ -105,7 +105,7 @@ protopassthrough_engage(pxy_conn_ctx_t *ctx)
 	pxy_fd_readcb(ctx->fd, 0, ctx);
 }
 
-static void NONNULL(1)
+static int NONNULL(1)
 protopassthrough_conn_connect(pxy_conn_ctx_t *ctx)
 {
 #ifdef DEBUG_PROXY
@@ -115,7 +115,7 @@ protopassthrough_conn_connect(pxy_conn_ctx_t *ctx)
 #endif /* DEBUG_PROXY */
 
 	if (prototcp_setup_srvdst(ctx) == -1) {
-		return;
+		return -1;
 	}
 
 	// @attention Sometimes dst write cb fires but not event cb, especially if this listener cb is not finished yet, so the conn stalls.
@@ -129,8 +129,9 @@ protopassthrough_conn_connect(pxy_conn_ctx_t *ctx)
 		log_dbg_level_printf(LOG_DBG_MODE_FINE, "protopassthrough_conn_connect: bufferevent_socket_connect for srvdst failed, fd=%d\n", fd);
 #endif /* DEBUG_PROXY */
 
-		// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect. Just return.
+		// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect. Just return 0.
 	}
+	return 0;
 }
 
 static void NONNULL(1)

protossl.c

@@ -1079,7 +1079,7 @@ protossl_setup_srvdst_new_bev_ssl_connecting(pxy_conn_ctx_t *ctx)
 	return 0;
 }
 
-void
+int
 protossl_conn_connect(pxy_conn_ctx_t *ctx)
 {
 #ifdef DEBUG_PROXY
@@ -1089,12 +1089,12 @@ protossl_conn_connect(pxy_conn_ctx_t *ctx)
 #endif /* DEBUG_PROXY */
 
 	if (prototcp_setup_dst(ctx) == -1) {
-		return;
+		return -1;
 	}
 
 	/* create server-side socket and eventbuffer */
 	if (protossl_setup_srvdst(ctx) == -1) {
-		return;
+		return -1;
 	}
 
 	bufferevent_setcb(ctx->dst.bev, pxy_bev_readcb, pxy_bev_writecb, pxy_bev_eventcb, ctx);
@@ -1115,8 +1115,9 @@ protossl_conn_connect(pxy_conn_ctx_t *ctx)
 		log_dbg_level_printf(LOG_DBG_MODE_FINE, "protossl_conn_connect: bufferevent_socket_connect for srvdst failed, fd=%d\n", fd);
 #endif /* DEBUG_PROXY */
 
-		// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect. Just return.
+		// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect. Just return 0.
 	}
+	return 0;
 }
 
 int

protossl.h

@@ -41,7 +41,7 @@ SSL *protossl_dstssl_create(pxy_conn_ctx_t *) NONNULL(1);
 
 void protossl_free(pxy_conn_ctx_t *) NONNULL(1);
 void protossl_fd_readcb(evutil_socket_t, short, void *);
-void protossl_conn_connect(pxy_conn_ctx_t *) NONNULL(1);
+int protossl_conn_connect(pxy_conn_ctx_t *) NONNULL(1);
 void protossl_connect_child(pxy_conn_child_ctx_t *) NONNULL(1);
 
 int protossl_setup_src_ssl(pxy_conn_ctx_t *) NONNULL(1);

prototcp.c

@@ -152,7 +152,7 @@ prototcp_setup_srvdst(pxy_conn_ctx_t *ctx)
 	return 0;
 }
 
-static void NONNULL(1)
+static int NONNULL(1)
 prototcp_conn_connect(pxy_conn_ctx_t *ctx)
 {
 #ifdef DEBUG_PROXY
@@ -162,12 +162,12 @@ prototcp_conn_connect(pxy_conn_ctx_t *ctx)
 #endif /* DEBUG_PROXY */
 
 	if (prototcp_setup_dst(ctx) == -1) {
-		return;
+		return -1;
 	}
 
 	/* create server-side socket and eventbuffer */
 	if (prototcp_setup_srvdst(ctx) == -1) {
-		return;
+		return -1;
 	}
 
 	// @attention Do not try to term/close conns on the thrmgr thread after setting event callbacks and/or socket connect, i.e. from this point on.
@@ -192,9 +192,10 @@ prototcp_conn_connect(pxy_conn_ctx_t *ctx)
 #endif /* DEBUG_PROXY */
 
 		// @attention Do not try to close the conn here on the thrmgr thread, otherwise both pxy_conn_connect() and eventcb try to free the conn using pxy_conn_free(),
-		// they are running on different threads, causing multithreading issues, e.g. signal 10. Just return.
+		// they are running on different threads, causing multithreading issues, e.g. signal 10. Just return 0.
 		// @todo Should we use thrmgr->mutex? Can we? Seems impossible or too difficult.
 	}
+	return 0;
 }
 
 int

pxyconn.c

@@ -382,11 +382,12 @@ pxy_conn_ctx_free(pxy_conn_ctx_t *ctx, int by_requestor)
 		if (idletime > (ctx->opts->user_timeout / 2)) {
 			struct userdbkeys *keys = malloc(sizeof(userdbkeys_t));
 			if (keys) {
+				// Zero out for NULL termination
 				memset(keys, 0, sizeof(userdbkeys_t));
-				// @todo Should limit copy with max dest size?
-				memcpy(keys->ip, ctx->srchost_str, strlen(ctx->srchost_str));
-				memcpy(keys->user, ctx->user, strlen(ctx->user));
-				memcpy(keys->ether, ctx->ether, strlen(ctx->ether));
+				// Leave room for NULL to make sure the strings are always NULL terminated
+				strncpy(keys->ip, ctx->srchost_str, sizeof(keys->ip) - 1);
+				strncpy(keys->user, ctx->user, sizeof(keys->user) - 1);
+				strncpy(keys->ether, ctx->ether, sizeof(keys->ether) - 1);
 
 				if (privsep_client_update_atime(ctx->clisock, keys) == -1) {
 #ifdef DEBUG_PROXY
@@ -1605,11 +1606,13 @@ pxy_conn_connect(pxy_conn_ctx_t *ctx)
 		}
 	}
 
-	ctx->protoctx->connectcb(ctx);
-
-	// @attention Do not try to close conns on the thrmgr thread after setting event callbacks and/or socket connect.
-	if (ctx->term || ctx->enomem) {
-		pxy_conn_free(ctx, ctx->term ? ctx->term_requestor : 1);
+	if (ctx->protoctx->connectcb(ctx) == -1) {
+		// @attention Do not try to close conns on the thrmgr thread after setting event callbacks and/or socket connect.
+		// The return value of -1 from connectcb indicates that there was a fatal error before event callbacks were set, so we can terminate the connection.
+		// Otherwise, it is up to the event callbacks to terminate the connection. This is necessary to avoid multithreading issues.
+		if (ctx->term || ctx->enomem) {
+			pxy_conn_free(ctx, ctx->term ? ctx->term_requestor : 1);
+		}
 	}
 }
 
@@ -1632,6 +1635,12 @@ pxy_fd_readcb(evutil_socket_t fd, UNUSED short what, void *arg)
 	ctx->protoctx->fd_readcb(fd, what, arg);
 }
 
+/*
+ * @attention Do not try to close conns on the thrmgr thread after setting event callbacks and/or socket connect.
+ * fd_readcb calls pxy_conn_connect() which sets event callbacks and connects sockets.
+ * The return value of -1 from call_fd_readcb indicates that there was a fatal error before event callbacks were set, so we can terminate the connection.
+ * Otherwise, it is up to the event callbacks to terminate the connection. This is necessary to avoid multithreading issues.
+ */
 static int NONNULL(1)
 call_fd_readcb(pxy_conn_ctx_t *ctx)
 {
@@ -1747,6 +1756,8 @@ redirect:
 	}
 
 	if (call_fd_readcb(ctx) == -1) {
+		// The return value of -1 from call_fd_readcb indicates that there was a fatal error before event callbacks were set,
+		// so we can terminate the connection.
 		goto memout;
 	}
 	return;
@@ -2031,6 +2042,8 @@ pxy_conn_setup(evutil_socket_t fd,
 		goto out;
 	} else {
 		if (call_fd_readcb(ctx) == -1) {
+			// The return value of -1 from call_fd_readcb indicates that there was a fatal error before event callbacks were set,
+			// so we can terminate the connection.
 			goto memout;
 		}
 		return;

pxyconn.h

@@ -62,7 +62,7 @@
 typedef struct pxy_conn_child_ctx pxy_conn_child_ctx_t;
 
 typedef void (*fd_readcb_func_t)(evutil_socket_t,  short, void *);
-typedef void (*connect_func_t)(pxy_conn_ctx_t *);
+typedef int (*connect_func_t)(pxy_conn_ctx_t *);
 
 typedef void (*callback_func_t)(struct bufferevent *, void *);
 typedef void (*eventcb_func_t)(struct bufferevent *, short, void *);