Ticket #544. Restore locking, seems w+ migh already lock the file sometimes. While there improve the locking to a read/write locking schema. Make the default locking a read only lock and if explicitly specified a write locking can be specified through LOCK_EX optional parameter to lock(). During config manipulation do the filesystem mounting in rw, if needed, before doing any locking to avoid possible problems and also to be consistent through out the code on the method used. Also update calls to config to lock exclusively where required.

2 files changed, 16 insertions, 19 deletions

diff --git a/etc/inc/config.lib.inc b/etc/inc/config.lib.inc
index d604ffc..0fa2315 100644
--- a/etc/inc/config.lib.inc
+++ b/etc/inc/config.lib.inc
@@ -338,8 +338,7 @@ function conf_mount_ro() {
 
 	clear_subsystem_dirty('mount');
 	/* sync data, then force a remount of /cf */
-	mwexec("/bin/sync");
-	mwexec("/bin/sync");
+	mwexec("/bin/sync; /bin/sync");
 	mwexec("/sbin/mount -u -r -f {$g['cf_path']}");
 	mwexec("/sbin/mount -u -r -f /");
 }
@@ -474,13 +473,12 @@ function write_config($desc="Unknown", $backup = true) {
 	$config['revision']['description'] = "{$_SESSION['Username']}: " . $desc;
 	$config['revision']['username'] = $_SESSION["Username"];
 
-	$lockkey = lock('config');
+	conf_mount_rw();
+	$lockkey = lock('config', LOCK_EX);
 
 	/* generate configuration XML */
 	$xmlconfig = dump_xml_config($config, $g['xml_rootobj']);
 
-	conf_mount_rw();
-
 	/* write new configuration */
 	if (!safe_write_file("{$g['cf_conf_path']}/config.xml", $xmlconfig, false)) {
 		log_error("WARNING: Config contents could not be save. Could not open file!");
@@ -506,14 +504,13 @@ function write_config($desc="Unknown", $backup = true) {
 		fclose($fd);
 	}
 
-	/* tell kernel to sync fs data */
-	if (!$g['booting'])
-		conf_mount_ro();
-
 	unlock($lockkey);
 
 	unlink_if_exists("/usr/local/pkg/pf/carp_sync_client.php");
 
+	/* tell kernel to sync fs data */
+	conf_mount_ro();
+
 	/* sync carp entries to other firewalls */
 	carp_sync_client();
 
@@ -534,9 +531,9 @@ function write_config($desc="Unknown", $backup = true) {
 function reset_factory_defaults($lock = false) {
 	global $g;
 
-	if (!$lock)
-		$lockkey = lock('config');
 	conf_mount_rw();
+	if (!$lock)
+		$lockkey = lock('config', LOCK_EX);
 
 	/* create conf directory, if necessary */
 	safe_mkdir("{$g['cf_conf_path']}");
@@ -555,9 +552,9 @@ function reset_factory_defaults($lock = false) {
 
 	/* call the wizard */
 	touch("/conf/trigger_initial_wizard");
-	conf_mount_ro();
 	if (!$lock)
 		unlock($lockkey);
+	conf_mount_ro();
 
 	return 0;
 }
@@ -572,7 +569,7 @@ function config_restore($conffile) {
 
 	conf_mount_rw();
 	
-	$lockkey = lock('config');
+	$lockkey = lock('config', LOCK_EX);
 
 	copy($conffile, "{$g['cf_conf_path']}/config.xml");
 	unlink_if_exists("{$g['tmp_path']}/config.cache");
@@ -603,7 +600,7 @@ function config_install($conffile) {
 		log_error("Installing configuration ....");
 
 	conf_mount_rw();
-	$lockkey = lock('config');
+	$lockkey = lock('config', LOCK_EX);
 
 	copy($conffile, "{$g['conf_path']}/config.xml");
 
diff --git a/etc/inc/util.inc b/etc/inc/util.inc
index 0fc10d7..930f9ac 100644
--- a/etc/inc/util.inc
+++ b/etc/inc/util.inc
@@ -119,18 +119,18 @@ function config_unlock() {
 }
 
 /* lock configuration file */
-function lock($lock) {
+function lock($lock, $op = LOCK_SH) {
 	global $g, $cfglckkeyconsumers;
 	if (!$lock)
 		die("WARNING: You must give a name as parameter to lock() function.");
 	if (!file_exists("{$g['tmp_path']}/{$lock}.lock"))
 		@touch("{$g['tmp_path']}/{$lock}.lock");
 	$cfglckkeyconsumers++;
-	if ($fp = fopen("{$g['tmp_path']}/{$lock}.lock", "w+")) {
-		//if (flock($fp, LOCK_EX))
+	if ($fp = fopen("{$g['tmp_path']}/{$lock}.lock", "w")) {
+		if (flock($fp, $op))
 			return $fp;
-		//else
-		//	fclose($fp);
+		else
+			fclose($fp);
 	}
 }