mysqli_prepare can return false

[jdarwood007]

Description

Review the following code:

function smf_db_error_insert($error_array)
{
	global $db_prefix, $db_connection;
	static $mysql_error_data_prep;

	// without database we can't do anything
	if (empty($db_connection))
		return;

	if (empty($mysql_error_data_prep))
		$mysql_error_data_prep = mysqli_prepare($db_connection,
			'INSERT INTO ' . $db_prefix . 'log_errors
				(id_member, log_time, ip, url, message, session, error_type, file, line, backtrace)
			VALUES( ?, ?, unhex(?), ?, ?, ?, ?, ?, ?, ?)'
		);

	if (filter_var($error_array[2], FILTER_VALIDATE_IP) !== false)
		$error_array[2] = bin2hex(inet_pton($error_array[2]));
	else
		$error_array[2] = null;
	mysqli_stmt_bind_param($mysql_error_data_prep, 'iissssssis',
		$error_array[0], $error_array[1], $error_array[2], $error_array[3], $error_array[4], $error_array[5], $error_array[6],
		$error_array[7], $error_array[8], $error_array[9]);
	mysqli_stmt_execute($mysql_error_data_prep);
}

According to documentation on mysqli_prepare, it can return false on error. This will result in mysqli_stmt_bind_param returning a php error because $mysql_error_data_prep contains a bool, instead of the expected type mysqli_stmt

We need to address handling this case better. @albertlast as you wrote this code initially your inputs are useful.

I'm thinking we need to check if $mysql_error_data_prep returned boolean of false. If so, we fall back to using a standard insert. If that fails, we can't do anything. I'm thinking we should do a fatal error then since we don't have any database connection.

[jdarwood007]

Also, note since #7815 has merged, I haven't tested to see how it affects the ease of reproducing it.

[albertlast]

in my eyes the fallback scenario is not a valid option, since the content could be dangerous. So a bad guys could try to force here fallback scenario, add dangerous stuff in the fallback scenario, when our fallback scenario is normal insert.

alternative fallback could be, that we write into db_last_error.php, maybe wrap error stuff into a json string?

[jdarwood007]

The fallback was trying to do a standard query. But I realize the problem is that the database connection has failed. It wouldn't work either.

I can't link the source as its a private discussion. What is happening is that the database connection is being blocked/rejected after the initial connection is made, SMF is trying to write to the error log and another error is occurring. The loop starts. Luckily we have some loop protection in the code and that is stopping things.

I'm thinking the simple method for now would be to just bail out if we can't write to the error log. The bad here is that the error could be a non fatal error that would otherwise return silently and continue on. With such a change, the issue always becomes a fatal error.

I like the idea of db_last_error but maybe a separate file. Then let a scheduled task check it and populate it into the error log. I think that is a 2.2 thing though.

[albertlast]

Any way, like you notice, when this prepare approache failed, than your smf is in very critical situation, which means you ressources you can access is very limited, so trying any thing fancy in this point of time would failed also.

the loop detection came also from me i believe.

[Sesquipedalian]

I have also seen this error occur when trying to log an error that had a very large backtrace string. Specifically, the backtrace string was too large to fit in the backtrace field in the table, and so mysqli_prepare() returned false.

I mention this because it means that connection problems are not the only way that this issue can be triggered. I don't know whether that information makes any difference regarding possible solutions, but it is worth knowing.

[jdarwood007]

Well in that case, truncating the backtrace would make sense. To do it safely, we would want to pop off the first 10 items of the backtrace. Everything after that is most likely a loop.

[Sesquipedalian]

Oh, shortening the backtrace in that case wasn't a problem and I easily solved it. (It was for custom code, and not relevant here.)

The point of mentioning it was to say that there are multiple reasons why the prepare statement might return false.

[albertlast]

backtracer length is a mysql issue.

[live627]

here we go, here we go

https://www.simplemachines.org/community/index.php?topic=586815.0

[gregorklaric]

Also https://www.simplemachines.org/community/index.php?topic=588435