Merge pull request #457 from nodogsplash/4.3.3beta

Fix: restart at high loads, Fix: deadlock

openwrt/nodogsplash/files/usr/lib/nodogsplash/restart.sh

@@ -3,6 +3,8 @@
 # Check if nodogsplash is running
 if ndsctl status &> /dev/null; then
   if [ "$(uci -q get nodogsplash.@nodogsplash[0].fwhook_enabled)" = "1" ]; then
+    ndspid=$(ps | grep nodogsplash | awk -F ' ' 'NR==2 {print $1}')
+    echo "fwhook restart request received - restarting " | logger -p "daemon.err" -s -t "nodogsplash[$ndspid]: "
     /etc/init.d/nodogsplash restart
   fi
 fi

src/auth.c

@@ -249,30 +249,42 @@ auth_client_deauth(const unsigned id, const char *reason)
 	return rc;
 }
 
+
+/**
+ * @brief auth_client_auth_nolock authenticate a client without holding the CLIENT_LIST lock
+ * @param id the client id
+ * @param reason can be NULL
+ * @return 0 on success
+ */
 int
-auth_client_auth(const unsigned id, const char *reason)
+auth_client_auth_nolock(const unsigned id, const char *reason)
 {
 	t_client *client;
 	int rc;
 
-	LOCK_CLIENT_LIST();
-
 	client = client_list_find_by_id(id);
 
 	/* Client should already have hit the server and be on the client list */
 	if (client == NULL) {
 		debug(LOG_ERR, "Client %u to authenticate is not on client list", id);
-		rc = -1;
-		goto end;
+		return -1;
 	}
 
 	rc = auth_change_state(client, FW_MARK_AUTHENTICATED, reason);
-
 	if (rc == 0) {
 		authenticated_since_start++;
 	}
 
-end:
+	return rc;
+}
+
+int
+auth_client_auth(const unsigned id, const char *reason)
+{
+	int rc;
+
+	LOCK_CLIENT_LIST();
+	rc = auth_client_auth_nolock(id, reason);
 	UNLOCK_CLIENT_LIST();
 
 	return rc;

src/auth.h

@@ -28,6 +28,7 @@
 
 int auth_client_deauth(unsigned id, const char *reason);
 int auth_client_auth(unsigned id, const char *reason);
+int auth_client_auth_nolock(const unsigned id, const char *reason);
 int auth_client_trust(const char *mac);
 int auth_client_untrust(const char *mac);
 int auth_client_allow(const char *mac);

src/common.h

@@ -30,7 +30,7 @@
 #define MAX_BUF 4096
 
 /* Max length of a query string in bytes */
-#define QUERYMAXLEN 2048
+#define QUERYMAXLEN 4096
 
 /* Separator for Preauth query string */
 #define QUERYSEPARATOR ", "

src/fw_iptables.c

@@ -1002,6 +1002,8 @@ iptables_fw_counters_update(void)
 	config = config_get_config();
 	af = config->ip6 ? AF_INET6 : AF_INET;
 
+	LOCK_CLIENT_LIST();
+
 	/* Look for outgoing traffic of authenticated clients. */
 	safe_asprintf(&script, "%s %s", "iptables", "-v -n -x -t mangle -L " CHAIN_OUTGOING);
 	output = popen(script, "r");
@@ -1026,7 +1028,6 @@ iptables_fw_counters_update(void)
 				continue;
 			}
 			debug(LOG_DEBUG, "Read outgoing traffic for %s: Bytes=%llu", ip, counter);
-			LOCK_CLIENT_LIST();
 			if ((p1 = client_list_find_by_ip(ip))) {
 				if (p1->counters.outgoing < counter) {
 					p1->counters.outgoing = counter;
@@ -1036,7 +1037,6 @@ iptables_fw_counters_update(void)
 			} else {
 				debug(LOG_WARNING, "Could not find %s in client list", ip);
 			}
-			UNLOCK_CLIENT_LIST();
 		}
 	}
 	pclose(output);
@@ -1065,7 +1065,6 @@ iptables_fw_counters_update(void)
 				continue;
 			}
 			debug(LOG_DEBUG, "Read incoming traffic for %s: Bytes=%llu", ip, counter);
-			LOCK_CLIENT_LIST();
 			if ((p1 = client_list_find_by_ip(ip))) {
 				if (p1->counters.incoming < counter) {
 					p1->counters.incoming = counter;
@@ -1074,10 +1073,10 @@ iptables_fw_counters_update(void)
 			} else {
 				debug(LOG_WARNING, "Could not find %s in client list", ip);
 			}
-			UNLOCK_CLIENT_LIST();
 		}
 	}
 	pclose(output);
+	UNLOCK_CLIENT_LIST();
 
 	return 0;
 }

src/http_microhttpd.c

@@ -64,7 +64,7 @@ static int show_preauthpage(struct MHD_Connection *connection, const char *query
 static int encode_and_redirect_to_splashpage(struct MHD_Connection *connection, t_client *client, const char *originurl, const char *querystr);
 static int redirect_to_splashpage(struct MHD_Connection *connection, t_client *client, const char *host, const char *url);
 static int send_error(struct MHD_Connection *connection, int error);
-static int send_redirect_temp(struct MHD_Connection *connection, const char *url);
+static int send_redirect_temp(struct MHD_Connection *connection, t_client *client, const char *url);
 static int send_refresh(struct MHD_Connection *connection);
 static int is_foreign_hosts(struct MHD_Connection *connection, const char *host);
 static int is_splashpage(const char *host, const char *url);
@@ -337,7 +337,7 @@ libmicrohttpd_cb(void *cls,
 	/* only allow get */
 	if (0 != strcmp(method, "GET")) {
 		debug(LOG_DEBUG, "Unsupported http method %s", method);
-		return send_error(connection, 503);
+		return send_error(connection, 403);
 	}
 
 	/* switch between preauth, authenticated */
@@ -361,7 +361,7 @@ libmicrohttpd_cb(void *cls,
 	if (!client) {
 		client = add_client(mac, ip);
 		if (!client) {
-			return send_error(connection, 503);
+			return send_error(connection, 403);
 		}
 	}
 
@@ -516,7 +516,7 @@ static int authenticate_client(struct MHD_Connection *connection,
 	}
 
 	if (redirect_url) {
-		return send_redirect_temp(connection, redirect_url);
+		return send_redirect_temp(connection, client, redirect_url);
 	} else {
 		return send_error(connection, 200);
 	}
@@ -547,7 +547,12 @@ static int authenticated(struct MHD_Connection *connection,
 	char *fasurl = NULL;
 	int ret;
 
-	MHD_get_connection_values(connection, MHD_HEADER_KIND, get_host_value_callback, &host);
+	ret = MHD_get_connection_values(connection, MHD_HEADER_KIND, get_host_value_callback, &host);
+
+	if (ret < 1) {
+		debug(LOG_ERR, "authenticated: Error getting host");
+		return ret;
+	}
 
 	/* check if this is a late request, meaning the user tries to get the internet, but ended up here,
 	 * because the iptables rule came too late */
@@ -559,15 +564,15 @@ static int authenticated(struct MHD_Connection *connection,
 	if (check_authdir_match(url, config->denydir)) {
 		auth_client_deauth(client->id, "client_deauth");
 		snprintf(redirect_to_us, sizeof(redirect_to_us), "http://%s/", config->gw_address);
-		return send_redirect_temp(connection, redirect_to_us);
+		return send_redirect_temp(connection, client, redirect_to_us);
 	}
 
 	if (check_authdir_match(url, config->authdir)) {
 		if (config->fas_port && !config->preauth) {
 			safe_asprintf(&fasurl, "%s?clientip=%s&gatewayname=%s&gatewayaddress=%s&status=authenticated",
 				config->fas_url, client->ip, config->gw_name, config->gw_address);
 			debug(LOG_DEBUG, "fasurl %s", fasurl);
-			ret = send_redirect_temp(connection, fasurl);
+			ret = send_redirect_temp(connection, client, fasurl);
 			free(fasurl);
 			return ret;
 		} else if (config->fas_port && config->preauth) {
@@ -610,7 +615,7 @@ static int show_preauthpage(struct MHD_Connection *connection, const char *query
 	char enc_user_agent[256] = {0};
 
 	// Encoded querystring could be bigger than the unencoded version
-	char enc_query[QUERYMAXLEN + QUERYMAXLEN/4] = {0};
+	char enc_query[QUERYMAXLEN + QUERYMAXLEN/2] = {0};
 
 	int rc;
 	int ret;
@@ -682,26 +687,31 @@ static int preauthenticated(struct MHD_Connection *connection,
 		return ret;
 	}
 
-	MHD_get_connection_values(connection, MHD_HEADER_KIND, get_host_value_callback, &host);
+	ret = MHD_get_connection_values(connection, MHD_HEADER_KIND, get_host_value_callback, &host);
 
-	debug(LOG_DEBUG, "Preauthenticated - Requested Host is [ %s ]", host);
-	debug(LOG_DEBUG, "Preauthenticated - Requested url is [ %s ]", url);
-	debug(LOG_DEBUG, "Preauthenticated - Gateway Address is [ %s ]", config->gw_address);
-	debug(LOG_DEBUG, "Preauthenticated - Gateway Port is [ %u ]", config->gw_port);
+	if (ret < 1) {
+		debug(LOG_ERR, "preauthenticated: Error getting host");
+		return ret;
+	}
+
+	debug(LOG_DEBUG, "preauthenticated: Requested Host is [ %s ]", host);
+	debug(LOG_DEBUG, "preauthenticated: Requested url is [ %s ]", url);
+	debug(LOG_DEBUG, "preauthenticated: Gateway Address is [ %s ]", config->gw_address);
+	debug(LOG_DEBUG, "preauthenticated: Gateway Port is [ %u ]", config->gw_port);
 
 	/* check if this is an attempt to directly access the basic splash page when FAS is enabled  */
 	if (config->fas_port) {
 		snprintf(portstr, MAX_HOSTPORTLEN, ":%u", config->gw_port);
 
-		debug(LOG_DEBUG, "Preauthenticated - FAS is enabled");
-		debug(LOG_DEBUG, "Preauthenticated - NDS port ID is [ %s ]", portstr);
-		debug(LOG_DEBUG, "Preauthenticated - NDS port ID search result is [ %s ]", strstr(host, portstr));
+		debug(LOG_DEBUG, "preauthenticated: FAS is enabled");
+		debug(LOG_DEBUG, "preauthenticated: NDS port ID is [ %s ]", portstr);
+		debug(LOG_DEBUG, "preauthenticated: NDS port ID search result is [ %s ]", strstr(host, portstr));
 
 		if (check_authdir_match(url, config->authdir) || strstr(host, "/splash.css") == NULL) {
-			debug(LOG_DEBUG, "Preauthenticated - splash.css or authdir detected");
+			debug(LOG_DEBUG, "preauthenticated: splash.css or authdir detected");
 		} else {
 			if (strstr(host, portstr) != NULL) {
-				debug(LOG_DEBUG, "Preauthenticated -  403 Direct Access Forbidden");
+				debug(LOG_DEBUG, "preauthenticated:  403 Direct Access Forbidden");
 				ret = send_error(connection, 403);
 				return ret;
 			}
@@ -821,7 +831,7 @@ static int encode_and_redirect_to_splashpage(struct MHD_Connection *connection,
 
 	debug(LOG_INFO, "splashpageurl: %s", splashpageurl);
 
-	ret = send_redirect_temp(connection, splashpageurl);
+	ret = send_redirect_temp(connection, client, splashpageurl);
 	free(splashpageurl);
 	return ret;
 }
@@ -927,7 +937,7 @@ add_client(const char *mac, const char *ip)
 	return client;
 }
 
-int send_redirect_temp(struct MHD_Connection *connection, const char *url)
+int send_redirect_temp(struct MHD_Connection *connection, t_client *client, const char *url)
 {
 	struct MHD_Response *response;
 	int ret;
@@ -937,8 +947,9 @@ int send_redirect_temp(struct MHD_Connection *connection, const char *url)
 
 	safe_asprintf(&redirect, redirect_body, url, url);
 
-	response = MHD_create_response_from_buffer(strlen(redirect), redirect, MHD_RESPMEM_MUST_COPY);
-	free (redirect);
+	debug(LOG_DEBUG, "send_redirect_temp: MHD_create_response_from_buffer");
+
+	response = MHD_create_response_from_buffer(strlen(redirect), redirect, MHD_RESPMEM_MUST_FREE);
 
 	if (!response) {
 		return send_error(connection, 503);
@@ -948,11 +959,25 @@ int send_redirect_temp(struct MHD_Connection *connection, const char *url)
 	ret = MHD_add_response_header(response, "Location", url);
 
 	if (ret == MHD_NO) {
-		debug(LOG_DEBUG, "Error adding Location header");
+		debug(LOG_ERR, "send_redirect_temp: Error adding Location header to redirection page");
 	}
 
-	MHD_add_response_header(response, "Connection", "close");
+	ret = MHD_add_response_header(response, "Connection", "close");
+
+	if (ret == MHD_NO) {
+		debug(LOG_ERR, "send_redirect_temp: Error adding Connection header to redirection page");
+	}
+
+	debug(LOG_INFO, "send_redirect_temp: Queueing response for %s, %s", client->ip, client->mac);
+
 	ret = MHD_queue_response(connection, MHD_HTTP_TEMPORARY_REDIRECT, response);
+
+	if (ret == MHD_NO) {
+		debug(LOG_ERR, "send_redirect_temp: Error queueing response for %s, %s", client->ip, client->mac);
+	} else {
+		debug(LOG_DEBUG, "send_redirect_temp: Response is Queued");
+	}
+
 	MHD_destroy_response(response);
 
 	return ret;

src/ndsctl_thread.c

@@ -304,14 +304,15 @@ ndsctl_auth(FILE *fp, char *arg)
 	LOCK_CLIENT_LIST();
 	client = client_list_find_by_any(arg, arg, arg);
 	id = client ? client->id : 0;
-	UNLOCK_CLIENT_LIST();
 
 	if (id) {
-		rc = auth_client_auth(id, "ndsctl_auth");
+		rc = auth_client_auth_nolock(id, "ndsctl_auth");
 	} else {
 		debug(LOG_DEBUG, "Client not found.");
 		rc = -1;
 	}
+	UNLOCK_CLIENT_LIST();
+
 
 	if (rc == 0) {
 		fprintf(fp, "Yes");