* Prevent concurrent logins on CP to not be recorded on the DB.

* Make the locking more complex to avoid locking exclusively during pruning task which would hurt a lot CP performance.
* Retire the disconnect_client and make all the disconnect functions use the sessionid as identifier.

All this was triggered by: http://forum.pfsense.org/index.php/topic,33879.0.html

4 files changed, 92 insertions, 97 deletions

diff --git a/etc/inc/captiveportal.inc b/etc/inc/captiveportal.inc
index f414d9a..480b268 100644
--- a/etc/inc/captiveportal.inc
+++ b/etc/inc/captiveportal.inc
@@ -715,11 +715,11 @@ function captiveportal_prune_old() {
 	!isset($config['captiveportal']['radiussession_timeout']) && !isset($config['voucher']['enable']))
 		return;
 
+	$radiusservers = captiveportal_get_radius_servers();
+
 	/* read database */
 	$cpdb = captiveportal_read_db();
 
-	$radiusservers = captiveportal_get_radius_servers();
-
 	/*	To make sure we iterate over ALL accounts on every run the count($cpdb) is moved
 	 *	outside of the loop. Otherwise the loop would evaluate count() on every iteration
 	 *	and since $i would increase and count() would decrement they would meet before we
@@ -783,7 +783,7 @@ function captiveportal_prune_old() {
 		if ($timedout) {
 			captiveportal_disconnect($cpdb[$i], $radiusservers,$term_cause,$stop_time);
 			captiveportal_logportalauth($cpdb[$i][4], $cpdb[$i][3], $cpdb[$i][2], "TIMEOUT");
-			$unsetindexes[$i] = $i;
+			$unsetindexes[] = $cpdb[$i][5];
 		}
 
 		/* do periodic RADIUS reauthentication? */
@@ -831,17 +831,14 @@ function captiveportal_prune_old() {
 				if ($auth_list['auth_val'] == 3) {
 					captiveportal_disconnect($cpdb[$i], $radiusservers, 17);
 					captiveportal_logportalauth($cpdb[$i][4], $cpdb[$i][3], $cpdb[$i][2], "RADIUS_DISCONNECT", $auth_list['reply_message']);
-					$unsetindexes[$i] = $i;
+					$unsetindexes[] = $cpdb[$i][5];
 				}
 			}
 		}
 	}
-	/* This is a kludge to overcome some php weirdness */
-	foreach($unsetindexes as $unsetindex)
-	unset($cpdb[$unsetindex]);
 
 	/* write database */
-	captiveportal_write_db($cpdb);
+	captiveportal_write_db($cpdb, false, $unsetindexes);
 }
 
 /* remove a single client according to the DB entry */
@@ -885,26 +882,29 @@ function captiveportal_disconnect($dbent, $radiusservers,$term_cause = 1,$stop_t
 
 }
 
-/* remove a single client by ipfw rule number */
-function captiveportal_disconnect_client($id,$term_cause = 1) {
+/* remove a single client by sessionid */
+function captiveportal_disconnect_client($sessionid, $term_cause = 1, $logoutReason = "LOGOUT") {
 	global $g, $config;
 
-	/* read database */
-	$cpdb = captiveportal_read_db();
 	$radiusservers = captiveportal_get_radius_servers();
+	$unsetindex = array();
+
+	$cpdblck = lock('captiveportaldb', LOCK_EX);
+
+	/* read database */
+	$cpdb = captiveportal_read_db(true);
 
 	/* find entry */
-	foreach ($cpdb as $i => $cpentry) {
-		if ($cpentry[1] == $id) {
-			captiveportal_disconnect($cpentry, $radiusservers, $term_cause);
-			captiveportal_logportalauth($cpentry[4], $cpentry[3], $cpentry[2], "DISCONNECT");
-			unset($cpdb[$i]);
-			break;
-		}
-	}		
+	if (isset($cpdb[$sessionid])) {
+		$cpentry = $cpdb[$sessionid];
+		/* write database */
+		$unsetindex[] = $sessionid;
+		captiveportal_write_db($cpdb, true, $unsetindex);
+		unlock($cpdblck);
 
-	/* write database */
-	captiveportal_write_db($cpdb);
+		captiveportal_disconnect($cpentry, $radiusservers, $term_cause);
+		captiveportal_logportalauth($cpentry[4], $cpentry[3], $cpentry[2], "DISCONNECT");
+	}		
 }
 
 /* send RADIUS acct stop for all current clients */
@@ -1261,38 +1261,51 @@ function radius($username,$password,$clientip,$clientmac,$type) {
 }
 
 /* read captive portal DB into array */
-function captiveportal_read_db() {
-		global $g;
+function captiveportal_read_db($locked = false) {
+	global $g;
 
-		$cpdb = array();
+	$cpdb = array();
 
+	if ($locked == false)
 		$cpdblck = lock('captiveportaldb');
-		$fd = @fopen("{$g['vardb_path']}/captiveportal.db", "r");
-		if ($fd) {
-			while (!feof($fd)) {
-				$line = trim(fgets($fd));
-				if ($line) 
-						$cpdb[] = explode(",", $line);
+	$fd = @fopen("{$g['vardb_path']}/captiveportal.db", "r");
+	if ($fd) {
+		while (!feof($fd)) {
+			$line = trim(fgets($fd));
+			if ($line) {
+				$cpe = explode(",", $line);
+				/* Hash by session id */
+				$cpdb[$cpe[5]] = $cpe;
 			}
-			fclose($fd);
 		}
+		fclose($fd);
+	}
+	if ($locked == false)
 		unlock($cpdblck);
-		return $cpdb;
+	return $cpdb;
 }
 
 /* write captive portal DB */
-function captiveportal_write_db($cpdb) {
-		global $g;
+function captiveportal_write_db($cpdb, $locked = false, $remove = array()) {
+	global $g;
 
+	if ($locked == false)
 		$cpdblck = lock('captiveportaldb', LOCK_EX);
-		$fd = @fopen("{$g['vardb_path']}/captiveportal.db", "w");
-		if ($fd) {
-			foreach ($cpdb as $cpent) {
-					fwrite($fd, join(",", $cpent) . "\n");
-			}
-			fclose($fd);
+
+	if (!empty($remove)) {
+		$cpdb = captiveportal_read_db(true);
+		foreach ($remove as $key => $entr)
+			unset($cpdb[$key]);
+	}
+	$fd = @fopen("{$g['vardb_path']}/captiveportal.db", "w");
+	if ($fd) {
+		foreach ($cpdb as $cpent) {
+			fwrite($fd, join(",", $cpent) . "\n");
 		}
-	unlock($cpdblck);
+		fclose($fd);
+	}
+	if ($locked == false)
+		unlock($cpdblck);
 }
 
 function captiveportal_write_elements() {
@@ -1609,11 +1622,16 @@ function portal_allow($clientip,$clientmac,$username,$password = null, $attribut
 	if (!is_array($attributes))
 		$attributes = array();
 
-	/* read in client database */
-	$cpdb = captiveportal_read_db();
-
 	$radiusservers = captiveportal_get_radius_servers();
 
+	/* Do not allow concurrent login execution. */
+	$cpdblck = lock('captiveportaldb', LOCK_EX);
+
+	unset($sessionid);
+
+	/* read in client database */
+	$cpdb = captiveportal_read_db(true);
+
 	if ($attributes['voucher'])
 		$remaining_time = $attributes['session_timeout'];
 
@@ -1654,33 +1672,32 @@ function portal_allow($clientip,$clientmac,$username,$password = null, $attribut
 		}
 	}
 
-	$nousers = count($cpdb);
-	for ($i = 0; $i < $nousers; $i++) {
+	foreach ($cpdb as $sid => $cpentry) {
 		/* on the same ip */
-		if($cpdb[$i][2] == $clientip) {
-			captiveportal_logportalauth($cpdb[$i][4],$cpdb[$i][3],$cpdb[$i][2],"CONCURRENT LOGIN - REUSING OLD SESSION");
-			$sessionid = $cpdb[$i][5];
+		if($cpentry[2] == $clientip) {
+			captiveportal_logportalauth($cpentry[4],$cpentry[3],$cpentry[2],"CONCURRENT LOGIN - REUSING OLD SESSION");
+			$sessionid = $sid;
 			break;
 		}
-		elseif (($attributes['voucher']) && ($username != 'unauthenticated') && ($cpdb[$i][4] == $username)) {
+		elseif (($attributes['voucher']) && ($username != 'unauthenticated') && ($cpentry[4] == $username)) {
 			// user logged in with an active voucher. Check for how long and calculate 
 			// how much time we can give him (voucher credit - used time)
-			$remaining_time = $cpdb[$i][0] + $cpdb[$i][7] - time();
+			$remaining_time = $cpentry[0] + $cpentry[7] - time();
 			if ($remaining_time < 0)    // just in case. 
 				$remaining_time = 0;
 
 			/* This user was already logged in so we disconnect the old one */
-			captiveportal_disconnect($cpdb[$i],$radiusservers,13);
-			captiveportal_logportalauth($cpdb[$i][4],$cpdb[$i][3],$cpdb[$i][2],"CONCURRENT LOGIN - TERMINATING OLD SESSION");
-			unset($cpdb[$i]);
+			captiveportal_disconnect($cpentry,$radiusservers,13);
+			captiveportal_logportalauth($cpentry[4],$cpentry[3],$cpentry[2],"CONCURRENT LOGIN - TERMINATING OLD SESSION");
+			unset($cpdb[$sessionid]);
 			break;
 		}
 		elseif ((isset($config['captiveportal']['noconcurrentlogins'])) && ($username != 'unauthenticated')) {
 			/* on the same username */
-			if (strcasecmp($cpdb[$i][4], $username) == 0) {
+			if (strcasecmp($cpentry[4], $username) == 0) {
 				/* This user was already logged in so we disconnect the old one */
-				captiveportal_disconnect($cpdb[$i],$radiusservers,13);
-				captiveportal_logportalauth($cpdb[$i][4],$cpdb[$i][3],$cpdb[$i][2],"CONCURRENT LOGIN - TERMINATING OLD SESSION");
+				captiveportal_disconnect($cpentry,$radiusservers,13);
+				captiveportal_logportalauth($cpentry[4],$cpentry[3],$cpentry[2],"CONCURRENT LOGIN - TERMINATING OLD SESSION");
 				unset($cpdb[$i]);
 				break;
 			}
@@ -1717,6 +1734,7 @@ function portal_allow($clientip,$clientmac,$username,$password = null, $attribut
 			if (!is_array($config['captiveportal']['passthrumac']))
 				$config['captiveportal']['passthrumac'] = array();
 			$config['captiveportal']['passthrumac'][] = $mac;
+			unlock($cpdblck);
 			$macrules = captiveportal_passthrumac_configure_entry($mac);
 			file_put_contents("{$g['tmp_path']}/macentry.rules.tmp", $macrules);
 			mwexec("/sbin/ipfw -q {$g['tmp_path']}/macentry.rules.tmp");
@@ -1761,17 +1779,19 @@ function portal_allow($clientip,$clientmac,$username,$password = null, $attribut
 			$cpdb[] = array(time(), $ruleno, $clientip, $clientmac, $username, $sessionid, $bpassword,
 				$attributes['session_timeout'], $attributes['idle_timeout'], $attributes['session_terminate_time']);
 
+			/* rewrite information to database */
+			captiveportal_write_db($cpdb, true);
+			unlock($cpdblck);
+
 			if (isset($config['captiveportal']['radacct_enable']) && !empty($radiusservers)) {
 				$acct_val = RADIUS_ACCOUNTING_START($ruleno,
                                 		$username, $sessionid, $radiusservers, $clientip, $clientmac);
 				if ($acct_val == 1)
 					captiveportal_logportalauth($username,$clientmac,$clientip,$type,"RADIUS ACCOUNTING FAILED");
 			}
-
-			/* rewrite information to database */
-			captiveportal_write_db($cpdb);
 		}
-	}
+	} else
+		unlock($cpdblck);
 
 	if ($writecfg == true)
 		write_config();
@@ -1812,33 +1832,6 @@ function portal_allow($clientip,$clientmac,$username,$password = null, $attribut
 }
 
 
-
-/* remove a single client by session ID
- *  by Dinesh Nair
- */
-function disconnect_client($sessionid, $logoutReason = "LOGOUT", $term_cause = 1) {
-    global $g, $config;
-
-    /* read database */
-    $cpdb = captiveportal_read_db();
-
-    $radiusservers = captiveportal_get_radius_servers();
-
-    /* find entry */
-    $dbcount = count($cpdb);
-    for ($i = 0; $i < $dbcount; $i++) {
-        if ($cpdb[$i][5] == $sessionid) {
-            captiveportal_disconnect($cpdb[$i],$radiusservers, $term_cause);
-            captiveportal_logportalauth($cpdb[$i][4],$cpdb[$i][3],$cpdb[$i][2],$logoutReason);
-            unset($cpdb[$i]);
-            break;
-        }
-    }
-
-    /* write database */
-    captiveportal_write_db($cpdb);
-}
-
 /*
  * Used for when pass-through credits are enabled.
  * Returns true when there was at least one free login to deduct for the MAC.
@@ -1931,4 +1924,4 @@ function captiveportal_write_usedmacs_db($usedmacs) {
 	unlock($cpumaclck);
 }
 
-?>
\ No newline at end of file
+?>
diff --git a/usr/local/captiveportal/index.php b/usr/local/captiveportal/index.php
index 30dec37..996cdeb 100755
--- a/usr/local/captiveportal/index.php
+++ b/usr/local/captiveportal/index.php
@@ -122,7 +122,7 @@ setTimeout('window.close();',5000) ;
 </HTML>
 
 EOD;
-	disconnect_client($_POST['logout_id']);
+	captiveportal_disconnect_client($_POST['logout_id']);
 	exit;
 } else if ($clientmac && $radmac_enable && portal_mac_radius($clientmac,$clientip)) {
     /* radius functions handle everything so we exit here since we're done */
diff --git a/usr/local/www/status_captiveportal.php b/usr/local/www/status_captiveportal.php
index 9560041..8913367 100755
--- a/usr/local/www/status_captiveportal.php
+++ b/usr/local/www/status_captiveportal.php
@@ -81,9 +81,10 @@ $concurrent = count($cpcontents);
 
 foreach ($cpcontents as $cpcontent) {
 	$cpent = explode(",", $cpcontent);
+	$sessionid = $cpent[5];
 	if ($_GET['showact'])
 		$cpent[5] = captiveportal_get_last_activity($cpent[2]);
-	$cpdb[] = $cpent;
+	$cpdb[$sessionid] = $cpent;
 }
 if ($_GET['order']) {
 	if ($_GET['order'] == "ip")
@@ -131,7 +132,7 @@ if ($_GET['order']) {
 	<?php endif; ?>
     <td class="list sort_ignore"></td>
   </tr>
-<?php foreach ($cpdb as $cpent): ?>
+<?php foreach ($cpdb as $sid => $cpent): ?>
   <tr>
     <td class="listlr"><?=$cpent[2];?></td>
     <td class="listr"><?=$cpent[3];?>&nbsp;</td>
@@ -141,7 +142,7 @@ if ($_GET['order']) {
     <td class="listr"><?php if ($cpent[5]) echo htmlspecialchars(date("m/d/Y H:i:s", $cpent[5]));?></td>
 	<?php endif; ?>
 	<td valign="middle" class="list" nowrap>
-	<a href="?order=<?=$_GET['order'];?>&showact=<?=htmlspecialchars($_GET['showact']);?>&act=del&id=<?=$cpent[1];?>" onclick="return confirm('<?=gettext("Do you really want to disconnect this client?");?>')"><img src="./themes/<?= $g['theme']; ?>/images/icons/icon_x.gif" width="17" height="17" border="0" title="<?=gettext("Disconnect");?>"></a></td>
+	<a href="?order=<?=$_GET['order'];?>&showact=<?=htmlspecialchars($_GET['showact']);?>&act=del&id=<?=$sid;?>" onclick="return confirm('<?=gettext("Do you really want to disconnect this client?");?>')"><img src="./themes/<?= $g['theme']; ?>/images/icons/icon_x.gif" width="17" height="17" border="0" title="<?=gettext("Disconnect");?>"></a></td>
   </tr>
 <?php endforeach; ?>
 </table>
diff --git a/usr/local/www/widgets/widgets/captive_portal_status.widget.php b/usr/local/www/widgets/widgets/captive_portal_status.widget.php
index d240d69..829515f 100644
--- a/usr/local/www/widgets/widgets/captive_portal_status.widget.php
+++ b/usr/local/www/widgets/widgets/captive_portal_status.widget.php
@@ -68,9 +68,10 @@ $concurrent = count($cpcontents);
 
 foreach ($cpcontents as $cpcontent) {
 	$cpent = explode(",", $cpcontent);
+	$sessionid = $cpent[5];
 	if ($_GET['showact'])
 		$cpent[5] = captiveportal_get_last_activity($cpent[2]);
-	$cpdb[] = $cpent;
+	$cpdb[$sessionid] = $cpent;
 }
 
 if ($_GET['order']) {
@@ -97,7 +98,7 @@ if ($_GET['order']) {
     <td class="listhdrr"><a href="?order=start&showact=<?=$_GET['showact'];?>"><?=gettext("Last activity");?></a></td>
 	<?php endif; ?>
   </tr>
-<?php foreach ($cpdb as $cpent): ?>
+<?php foreach ($cpdb as $sid => $cpent): ?>
   <tr>
     <td class="listlr"><?=$cpent[2];?></td>
     <td class="listr"><?=$cpent[3];?>&nbsp;</td>
@@ -107,7 +108,7 @@ if ($_GET['order']) {
     <td class="listr"><?php if ($cpent[5]) echo htmlspecialchars(date("m/d/Y H:i:s", $cpent[5]));?></td>
 	<?php endif; ?>
 	<td valign="middle" class="list" nowrap>
-	<a href="?order=<?=$_GET['order'];?>&showact=<?=$_GET['showact'];?>&act=del&id=<?=$cpent[1];?>" onclick="return confirm('Do you really want to disconnect this client?')"><img src="./themes/<?= $g['theme']; ?>/images/icons/icon_x.gif" width="17" height="17" border="0"></a></td>
+	<a href="?order=<?=$_GET['order'];?>&showact=<?=$_GET['showact'];?>&act=del&id=<?=$sid;?>" onclick="return confirm('Do you really want to disconnect this client?')"><img src="./themes/<?= $g['theme']; ?>/images/icons/icon_x.gif" width="17" height="17" border="0"></a></td>
   </tr>
 <?php endforeach; ?>
 </table>