Topic: remove last error code from Settings.php

[MrPhil]

How about writing the latest error code ($db_last_error) to a new file, separate from Settings.php? See this thread for a description of the problem. If two users get an error at the same time and PHP tries to write both to Settings.php, it often happens that the file gets emptied or otherwise corrupted. If this file is only to be written to by the Admin functions, corruption is much less likely and it can be left writable. A wiped out "LastError.php" could be automatically repaired (restored) without bringing down the whole forum.

[karlbenson]

Indeed it seems rather an oddity to have it in there.

Currently I make settings.php read only to prevent it from happening. (although there are negative side effects)

[MrPhil]

Well, since it's still broken in 1.1.5, would anyone like to pick up the ball and run with the following code (for 1.1.5)? I haven't actually tried it on a live system, so it may need some tweaking, but it should be "mostly there". The basic idea is to isolate the "db_last_error" timestamp into its own one-line file, so that if it gets crunched, it's not a catastrophe.


/Settings.php line 55

find

Code: [Select]

  $db_last_error = 0;replace

Code: [Select]

  // timestamp of last database error detected
  include($boarddir . '/LastError.php');
  // despite our best efforts, was this file emptied out?
  if (!isset($db_last_error)) {
    updateLastError(0);  // just give it a 0 value
    // might also/instead try to repair DB here
  }
/LastError.php  new file

Code: [Select]

<?php
$db_last_error = 0;
?>
/Sources/admin.php 1043
add new function after updateSettingsFile():

Code: [Select]

// Avoid calling updateSettingsFile for $db_last_error. If there's one error,
// there's often multiple errors, and two updateSettingsFile() calls too close
// together can be disastrous, especially if a second one is invoked just
// after Settings.php is truncated (zeroed out) and before the new copy is
// written. There's no guarantee that LastError.php won't get similarly hosed,
// but at least it can be easily automatically fixed.
function updateLastError($time) {
  global $boarddir;

  // Blank out the file - done to fix an oddity with some servers.
  $fp = @fopen($boarddir . '/LastError.php', 'w');

  // Is it even writable, though?
  if ($fp)
  {
    fclose ($fp);

    $fp = fopen($boarddir . '/LastError.php', 'r+');
    fwrite($fp, "<?php\n");  #### that should be backslash-n these 3 lines
    fwrite($fp, '$db_last_error = ' . $time . ";\n");
    fwrite($fp, '?' . ">\n");
    fclose($fp);
  }
}
(Why are \n turned into n within the [code] tag?)

/Sources/Errors.php  line 195

find

Code: [Select]

  updateSettingsFile(array('db_last_error' => time())));replace

Code: [Select]

  updateLastError(time());
/Sources/Subs-Auth.php  line 364

find

Code: [Select]

  updateSettingsFile(array('db_last_error' => time())));replace

Code: [Select]

  updateLastError(time());
Edit: fix minor typo, ran on live system and nothing blew up... , haven't tried simulating a DB error

[karlbenson]

I don't think there should be any difference for any 1.1.x version.

[MrPhil]

I'm curious... has anyone besides me tried running with these changes? I don't get enough traffic (forum is in unreleased test mode) and my host has a pretty solid database, so I can't test this thing under combat conditions. Considering how many people report their Settings.php wiped out, these changes could be quite useful -- if they work well. So far, they seem to do no harm, but I'd like to know if they do any good. Anyone with a sandbox where they could induce database errors with multiple visitors running?

The basic problem in the original code is that in writing the timestamp of the last database error, for some reason (on certain servers?) the entire file has to be truncated (cleared out). This leaves a window where the file contents are in some script variables, while the file proper is empty. If right at this time, another user experiences a database error and kicks off the timestamp write, they'll start working on an empty Settings.php. The first visitor may end up rewriting the file properly, but the second one will rewrite it with empty data, leaving it finally empty for good. My code tries to limit the damage by restricting this rewriting business to just a one-line file. If it gets munged anyway, it can be recreated (0 timestamp) by default.

After writing the above, I wonder if a simpler solution would be to check that the file contents read in (by the second user) are empty, and if so, don't do the truncate, and bail out right there. Someone want to look at that solution?

[karlbenson]

I've never had it happen on my forum.
With sometimes 100+ guests/spiders/users online.

I believe this only happens with very cheap hosts or ones having lots of issues.
Having a blank settings.php might be seen as indicative that your host is having some issues (mysql going down alot).

[MrPhil]

Having a blank settings.php might be seen as indicative that your host is having some issues (mysql going down alot).

Maybe so, but a lot of unhappy and frantic forum owners end up here, begging for help. That can't help SMF's reputation. "Suffering builds character" doesn't seem to appeal to a lot of people. If it's a recoverable database issue, there's no need to crash the forum. I don't think it was done deliberately, but it shouldn't have been left in to fester for so long.

[MrPhil]

Well, I see that 1.1.6 is now out, and that the developers have chosen not to address this critical problem. I say "critical" because not a day goes by that some frantic SMF owner isn't posting here or on my hosting service's boards that certain things aren't defined. It is because their Settings.php has been wiped out by a database error, and all this suffering (and bad press for SMF) is unnecessary. Earlier I posted a fix for it, and there's probably an even easier fix.

Settings.php gets rewritten (updateSettingsFile() in Admin.php) with a new "last error" timestamp code whenever a database error is discovered. The problem is the way this is done: during the process, due to one particular server's problem, the author chose to empty out the Settings file (after reading in and storing its contents). It's done by opening for writing, and immediately closing the file. Then the revised contents are written to the (now empty) file.

If you have two members running the forum at the same time, and they both experience a database error within a very short time (not an uncommon event), it appears to me that this could cause the problem. Member 1 gets an error and runs updateSettingsFile(). It reads in Settings.php's contents, and then empties out Settings.php. At this moment, Member 2 is also running updateSettingsFile() in response to a database error (if Member 1 just experienced an error, it should be common for Member 2 to also). If the timing of events is just wrong, Member 2 reads the contents of Settings.php -- which at this moment is empty -- and the check for good content (see comment "Make sure we got a good file.") does not result in an early exit. Member 1 rewrites Settings.php with a revised content, and a moment later, Member 2 empties out the file and rewrites it with EMPTY content. From that moment on, all SMF users are screwed.

Can anyone tell me if this is the correct analysis? It sure looks like it's happening, and it would be good to fix it once and for all. My earlier suggested fix involved moving the vulnerable part to a separate included file, so only one line gets wiped out at worst (and can be "repaired" easily). If that is not satisfactory, updateSettingsFile() should somehow lock Settings.php so a Member running SMF can't try reading it while another Member is emptying it out and rewriting it. Another method would be to improve the code that tries to detect whether an empty Settings.php was read in. My guess is that it's not working in all cases. The check for $settingsArray having fewer than 12 elements ought to do the job, but I suspect it's not. Could someone please take a look at the 1.1.6 code and see if there's any way one could get through the checks after reading an empty Settings.php file, and still be allowed to rewrite it?

Unfortunately, without a way to repeatably kick off updateSettingsFile() at just the right interval, it may never be possible to prove that my theory is correct. However, from the number of reports of Settings.php files being wiped out, something is going on!

[Jiveturkey]

I've had this happen twice this week and both were host issues.  The site goes down for 15 minutes and when it comes back up the settings.php file is gone.  I switched it to read only and will let you know if it comes back.

I never mess with those settings so I hope that this gets me around the issue.