Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements new, more robust version of updateSettingsFile() #5815

Open
wants to merge 5 commits into
base: release-2.1
from

Conversation

@Sesquipedalian
Copy link
Member

commented Sep 30, 2019

While working in 2.0.x to implement yet another attempt to fix the race condition bug that can sometimes cause Settings.php to get wiped out under situations of server stress, I came to the realization that the current version of the entire updateSettingsFile() function in both 2.0.x and 2.1 is fundamentally fragile and awkward in a number of ways.

First, the fact that we have been manually escaping values using addcslashes() and then prepending and appending literal quote characters is just silly, as well as error prone. The right way to do that sort of thing is to use var_export().

Second, in terms of reading and processing content, the current version does fine when the data in Settings.php is formatted as expected, but it only takes adding or removing a little bit of whitespace in unexpected places to cause issues.

Third, although moving $db_last_error into its own file greatly reduced the likelihood of the race condition happening, the current method of writing to Settings.php is still vulnerable to such race conditions. If we can eliminate that possibility entirely, that would be a very good idea.

So, I wrote this PR.

It uses an atomic method of making backups and writing to the file in order to prevent race conditions from ever occurring. It correctly formats all variable types for inclusion in Settings.php by using var_export(). Not only can it robustly handle even the messiest formatting, but it will fix it automatically. Moreover, if somehow the content of the file does get mangled or erased by some other process, it will effortlessly restore the correct values on the fly.

I've been testing it for quite a while now, and I have it at the place where its only requirements are:

  1. that the content of Settings.php at the beginning of SMF's execution needs to be well-formed enough for PHP itself to be able to parse it,
  2. that the values of whatever $config_vars elements are passed to updateSettingsFile() can be parsed by PHP, and
  3. that the Settings.php file is writable (of course).

Provided those three conditions are met, a valid, pristinely formatted copy of Settings.php should be written out every time.

If you want test it out, I recommend throwing a messy Settings.php like the following at it and seeing what happens.

Before:



	<?php

// Some random code
if ('foo' == 'bar')
	die();

$maintenance = 1;$mtitle = 'Maintenance Mode';	$mmessage = 'Okay faithful users...we\'re attempting to restore an older backup of the database...news will be posted once we\'re back!';

$mbname = 'My Community';$language = 'english';

define(SOME_RANDOM_CONSTANT, 'derp');

$boardurl = 'http://127.0.0.1/smf';

		$webmaster_email = 'noreply@myserver.com';
/**
 * Name of the cookie to set for authentication.
 *
 * @var string
 */
$cookiename = 'SMFCookie11';

	########## Database Info ##########
	/**
	 * The database type
	 * Default options: mysql, postgresql
	 *
	 * @var string
	 */
	$db_type = 'mysql';
/**
 * The database port
 * 0 to use default port for the database type
 *
 * @var int
 */
$db_port = 0;
	$db_server = 'localhost';
		$db_name = 'smf';
			$db_user = 'root';
				$db_passwd = '';
$ssi_db_user = '';$ssi_db_passwd = '';$db_prefix = 'smf_';



					$db_persist = 0;
	$db_error_send = 0;
$db_mb4 = NULL;$cache_accelerator = '';$cache_enable = 0;$cache_memcached = '';$cachedir = '/private/tmp/cache';$image_proxy_enabled = true;$image_proxy_secret = 'smfisawesome';$image_proxy_maxsize = 5192;$boarddir = dirname(__FILE__);$sourcedir = dirname(__FILE__) . '/Sources';
$packagesdir = dirname(__FILE__) . '/Packages';
$tasksdir = $sourcedir . '/tasks';







# Make sure the paths are correct... at least try to fix them.
if (!file_exists($boarddir) && file_exists(dirname(__FILE__) . '/agreement.txt'))
	$boarddir = dirname(__FILE__);
if (!file_exists($sourcedir) && file_exists($boarddir . '/Sources'))
	$sourcedir = $boarddir . '/Sources';
if (!file_exists($cachedir) && file_exists($boarddir . '/cache'))
	$cachedir = $boarddir . '/cache';

######### Legacy Settings #########
# UTF-8 is now the only character set supported in 2.1.
$db_character_set = 'utf8';

########## Error-Catching ##########
# Note: You shouldn't touch these settings.
if (file_exists((isset($cachedir) ? $cachedir : dirname(__FILE__)) . '/db_last_error.php'))
	include((isset($cachedir) ? $cachedir : dirname(__FILE__)) . '/db_last_error.php');

if (!isset($db_last_error))
{
	// File does not exist so lets try to create it
	file_put_contents((isset($cachedir) ? $cachedir : dirname(__FILE__)) . '/db_last_error.php', '<' . '?' . "php\n" . '$db_last_error = 0;' . "\n" . '?' . '>');
	$db_last_error = 0;
}

if (file_exists(dirname(__FILE__) . '/install.php'))
{
	$secure = false;
	if (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on')
		$secure = true;
	elseif (!empty($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https' || !empty($_SERVER['HTTP_X_FORWARDED_SSL']) && $_SERVER['HTTP_X_FORWARDED_SSL'] == 'on')
		$secure = true;

	header('location: http' . ($secure ? 's' : '') . '://' . (empty($_SERVER['HTTP_HOST']) ? $_SERVER['SERVER_NAME'] . (empty($_SERVER['SERVER_PORT']) || $_SERVER['SERVER_PORT'] == '80' ? '' : ':' . $_SERVER['SERVER_PORT']) : $_SERVER['HTTP_HOST']) . (strtr(dirname($_SERVER['PHP_SELF']), '\\', '/') == '/' ? '' : strtr(dirname($_SERVER['PHP_SELF']), '\\', '/')) . '/install.php');
	exit;
}

			?>



After:

<?php

/**
 * The settings file contains all of the basic settings that need to be present when a database/cache is not available.
 *
 * Simple Machines Forum (SMF)
 *
 * @package SMF
 * @author Simple Machines http://www.simplemachines.org
 * @copyright 2019 Simple Machines and individual contributors
 * @license http://www.simplemachines.org/about/smf/license.php BSD
 *
 * @version 2.1 RC2
 */

// Some random code
if ('foo' == 'bar')
	die();

########## Maintenance ##########
/**
 * The maintenance "mode"
 * Set to 1 to enable Maintenance Mode, 2 to make the forum untouchable. (you'll have to make it 0 again manually!)
 * 0 is default and disables maintenance mode.
 *
 * @var int 0, 1, 2
 * @global int $maintenance
 */
$maintenance = 1;
/**
 * Title for the Maintenance Mode message.
 *
 * @var string
 * @global int $mtitle
 */
$mtitle = 'Maintenance Mode';
/**
 * Description of why the forum is in maintenance mode.
 *
 * @var string
 * @global string $mmessage
 */
$mmessage = 'Okay faithful users...we\'re attempting to restore an older backup of the database...news will be posted once we\'re back!';

########## Forum Info ##########
/**
 * The name of your forum.
 *
 * @var string
 */
$mbname = 'My Community';
/**
 * The default language file set for the forum.
 *
 * @var string
 */
$language = 'english';

define(SOME_RANDOM_CONSTANT, 'derp');

/**
 * URL to your forum's folder. (without the trailing /!)
 *
 * @var string
 */
$boardurl = 'http://127.0.0.1/smf';

/**
 * Email address to send emails from. (like noreply@yourdomain.com.)
 *
 * @var string
 */
$webmaster_email = 'noreply@myserver.com';
/**
 * Name of the cookie to set for authentication.
 *
 * @var string
 */
$cookiename = 'SMFCookie11';

########## Database Info ##########
/**
 * The database type
 * Default options: mysql, postgresql
 *
 * @var string
 */
$db_type = 'mysql';
/**
 * The database port
 * 0 to use default port for the database type
 *
 * @var int
 */
$db_port = 0;
/**
 * The server to connect to (or a Unix socket)
 *
 * @var string
 */
$db_server = 'localhost';
/**
 * The database name
 *
 * @var string
 */
$db_name = 'smf';
/**
 * Database username
 *
 * @var string
 */
$db_user = 'root';
/**
 * Database password
 *
 * @var string
 */
$db_passwd = '';
/**
 * Database user for when connecting with SSI
 *
 * @var string
 */
$ssi_db_user = '';
/**
 * Database password for when connecting with SSI
 *
 * @var string
 */
$ssi_db_passwd = '';
/**
 * A prefix to put in front of your table names.
 * This helps to prevent conflicts
 *
 * @var string
 */
$db_prefix = 'smf_';

/**
 * Use a persistent database connection
 *
 * @var int|bool
 */
$db_persist = 0;
/**
 *
 * @var int|bool
 */
$db_error_send = 0;
/**
 * Override the default behavior of the database layer for mb4 handling
 * null keep the default behavior untouched
 *
 * @var null|bool
 */
$db_mb4 = NULL;

########## Cache Info ##########
/**
 * Select a cache system. You want to leave this up to the cache area of the admin panel for
 * proper detection of apc, memcached, output_cache, smf, or xcache
 * (you can add more with a mod).
 *
 * @var string
 */
$cache_accelerator = '';
/**
 * The level at which you would like to cache. Between 0 (off) through 3 (cache a lot).
 *
 * @var int
 */
$cache_enable = 0;
/**
 * This is only used for memcache / memcached. Should be a string of 'server:port,server:port'
 *
 * @var array
 */
$cache_memcached = '';
/**
 * This is only for the 'smf' file cache system. It is the path to the cache directory.
 * It is also recommended that you place this in /tmp/ if you are going to use this.
 *
 * @var string
 */
$cachedir = '/private/tmp/cache';

########## Image Proxy ##########
# This is done entirely in Settings.php to avoid loading the DB while serving the images
/**
 * Whether the proxy is enabled or not
 *
 * @var int|bool
 */
$image_proxy_enabled = true;

/**
 * Secret key to be used by the proxy
 *
 * @var string
 */
$image_proxy_secret = 'smfisawesome';

/**
 * Maximum file size (in KB) for individual files
 *
 * @var int
 */
$image_proxy_maxsize = 5192;

########## Directories/Files ##########
# Note: These directories do not have to be changed unless you move things.
/**
 * The absolute path to the forum's folder. (not just '.'!)
 *
 * @var string
 */
$boarddir = '/private/tmp';
/**
 * Path to the Sources directory.
 *
 * @var string
 */
$sourcedir = '/private/tmp/Sources';
/**
 * Path to the Packages directory.
 *
 * @var string
 */
$packagesdir = '/private/tmp/Packages';
/**
 * Path to the tasks directory.
 *
 * @var string
 */
$tasksdir = '/private/tmp/Sources/tasks';

# Make sure the paths are correct... at least try to fix them.
if (!file_exists($boarddir) && file_exists(dirname(__FILE__) . '/agreement.txt'))
	$boarddir = dirname(__FILE__);
if (!file_exists($sourcedir) && file_exists($boarddir . '/Sources'))
	$sourcedir = $boarddir . '/Sources';
if (!file_exists($cachedir) && file_exists($boarddir . '/cache'))
	$cachedir = $boarddir . '/cache';

######### Legacy Settings #########
# UTF-8 is now the only character set supported in 2.1.
$db_character_set = 'utf8';

########## Error-Catching ##########
# Note: You shouldn't touch these settings.
if (file_exists((isset($cachedir) ? $cachedir : dirname(__FILE__)) . '/db_last_error.php'))
	include((isset($cachedir) ? $cachedir : dirname(__FILE__)) . '/db_last_error.php');

if (!isset($db_last_error))
{
	// File does not exist so lets try to create it
	file_put_contents((isset($cachedir) ? $cachedir : dirname(__FILE__)) . '/db_last_error.php', '<' . '?' . "php\n" . '$db_last_error = 0;' . "\n" . '?' . '>');
	$db_last_error = 0;
}

if (file_exists(dirname(__FILE__) . '/install.php'))
{
	$secure = false;
	if (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on')
		$secure = true;
	elseif (!empty($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https' || !empty($_SERVER['HTTP_X_FORWARDED_SSL']) && $_SERVER['HTTP_X_FORWARDED_SSL'] == 'on')
		$secure = true;

	header('location: http' . ($secure ? 's' : '') . '://' . (empty($_SERVER['HTTP_HOST']) ? $_SERVER['SERVER_NAME'] . (empty($_SERVER['SERVER_PORT']) || $_SERVER['SERVER_PORT'] == '80' ? '' : ':' . $_SERVER['SERVER_PORT']) : $_SERVER['HTTP_HOST']) . (strtr(dirname($_SERVER['PHP_SELF']), '\\', '/') == '/' ? '' : strtr(dirname($_SERVER['PHP_SELF']), '\\', '/')) . '/install.php');
	exit;
}

?>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian Sesquipedalian added this to the Final milestone Sep 30, 2019
@jdarwood007

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

Starting to read it over and it looks good.

I've got a few things I would like to test/note

First note is that Settings.php is common: https://github.com/search?q=filename%3A%22Settings.php%22&type=Code
I wouldn't say 100% this could happen, but a integration could possibly conflict here.

Second note is I generally take my $db_passwd and put it in another file, then include that file in Settings.php. Simply as a fail safe should php fail and start offering text downloads instead. This will break in this case.

Third note is we do something similar in upgrade.php. We should make this code match or have the upgrade.php piggy back off this code if it can safely.

Fourth note is sometimes integrations are adding stuff to the Settings.php.

For the second case and fourth case this is something that needs to be handled. I think the second can be handled via some override trick. The fourth maybe we simply add all the extra stuff to the bottom that is a valid variable?

@Sesquipedalian

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

Starting to read it over and it looks good.

I've got a few things I would like to test/note

First note is that Settings.php is common: https://github.com/search?q=filename%3A%22Settings.php%22&type=Code
I wouldn't say 100% this could happen, but a integration could possibly conflict here.

Second note is I generally take my $db_passwd and put it in another file, then include that file in Settings.php. Simply as a fail safe should php fail and start offering text downloads instead. This will break in this case.

Third note is we do something similar in upgrade.php. We should make this code match or have the upgrade.php piggy back off this code if it can safely.

Fourth note is sometimes integrations are adding stuff to the Settings.php.

For the second case and fourth case this is something that needs to be handled. I think the second can be handled via some override trick. The fourth maybe we simply add all the extra stuff to the bottom that is a valid variable?

Regarding note 1, if that happened we'd be hooped anyway. I don't think this PR can or should do anything about that sort of situation.

Regarding note 2, I suppose I could simplify the code for the integrate_update_settings_file hook to allow it to alter the standard settings definitions, too. Then you could use a custom mod to hook into that and unset $settings_defs['db_passwd'] in order to prevent it from being re-added to Settings.php.

Regarding note 3, I agree. I'll follow up with a commit doing that soon(-ish).

Regarding note 4, this code already does that. Anything unexpected is preserved as-is. Moreover, its relative position in the code is maintained, which is important. You can see this being demonstrated with the // Some random code... and define(SOME_RANDOM_CONSTANT, 'derp'); bits in my example.

Previously, the could add their own but could not change SMF's standard ones at all

Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
Signed-off-by: Jon Stovell <jonstovell@gmail.com>
@Sesquipedalian

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

In theory this should do it. It still needs testing, particularly with the installer and upgrader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.