Fix bug GH-8058 - mysqlnd segfault when prepare fails

[kamil-tekiela]

This is an attempt to fix #8058. We can't set the prepared statement status to INITTED because it's already prepared and has bound variables. We have to keep the error message though when it fails.
However, one existing test case fails because it expects the statement to be in non-executable state. I'd say that expectation is invalid, but I would appreciate second opinion.

[cmb69]

An alternative might be to reset the respective result and result_binds:

 ext/mysqlnd/mysqlnd_ps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ext/mysqlnd/mysqlnd_ps.c b/ext/mysqlnd/mysqlnd_ps.c
index f3dee49d88..3de5c86a6e 100644
--- a/ext/mysqlnd/mysqlnd_ps.c
+++ b/ext/mysqlnd/mysqlnd_ps.c
@@ -504,6 +504,12 @@ fail:
 	if (stmt_to_prepare != stmt && s_to_prepare) {
 		s_to_prepare->m->dtor(s_to_prepare, TRUE);
 	}
+	mysqlnd_stmt_separate_result_bind(s);
+	if (stmt->result) {
+		stmt->result->m.free_result_contents(stmt->result);
+		stmt->result = NULL;
+	}
+	stmt->field_count = 0;
 	stmt->state = MYSQLND_STMT_INITTED;
 
 	DBG_INF("FAIL");

That would let mysqli_stmt_affected_rows.phpt fail with:

002+ [043] Expecting boolean/false, got boolean\1
     mysqli_stmt object is already closed
     done!

The boolean\1 would match libmysqli-client's behavior:

php-src/ext/mysqli/tests/mysqli_stmt_affected_rows.phpt

Lines 205 to 211 in 0052af2

	if ($IS_MYSQLND) {
	if (false !== ($tmp = mysqli_stmt_store_result($stmt)))
	printf("[043] Expecting boolean/false, got %s\%s\n", gettype($tmp), $tmp);
	} else {
	if (true !== ($tmp = mysqli_stmt_store_result($stmt)))
	printf("[043] Libmysql does not care if the previous statement was bogus, expecting boolean/true, got %s\%s\n", gettype($tmp), $tmp);
	}

[kamil-tekiela]

@cmb69 Yeah, I thought about resetting the prepared statement, but I don't know if there is an appropriate method for doing this. The way you suggested is not enough as the PS could have already been executed. It would leave the connection in an inconsistent state.

What bothers me is why are we creating a temporary statement if we expect the original one to be reset? Shouldn't we just work with the existing one? There must be a reason, otherwise the code would just work with the existing PS.

I think I need to study this code a little bit more to understand the design.

[kamil-tekiela]

@cmb69 I have removed the temporary prepared statement. What do you think about this? This fulfils the expectation of the PS being non-executable upon failed prepare. I really do not know what the purpose of the temporary PS was. The only reason I can think of was to keep the previous state intact, but then the stmt->state = MYSQLND_STMT_INITTED at the end would do the opposite. Was it done to avoid non-const parameter? I tested it without it and it seems to work as expected. What am I missing?

[cmb69]

I have no idea why there is a temporary PS, either. Maybe it was to better simulate libmysql-client's behavior? I think dropping that temp PS makes sense, but if we want to be conservative, maybe we should only do that for master, and apply your first patch to PHP-8.0/8.1.

[cmb69]

It seems that the Azure pipelines parallelism issue is still unresolved.

[kamil-tekiela]

It seems that the Azure pipelines parallelism issue is still unresolved.

I have restarted it again and it seems to have executed, at least partially

[kamil-tekiela]

if we want to be conservative, maybe we should only do that for master, and apply your first patch to PHP-8.0/8.1.

I thought that this is a great idea, but then I thought about it more. I did some more debugging today and I came to the conclusion that the previous behaviour was buggy. The test I changed had an incorrect expectation and my fix fixed that bug also.

I think we can pull the whole fix into PHP 8.0. It also follows what libmysql does, although I think they reset the PS a little differently. It doesn't seem to be causing any serious adverse effects. It is showing the error message now when prepare fails. Because the order of operations changed, we are now discarding any potentially unfetched result sets; previously there was out of sync error. I doubt anyone would ever complain about this change...

In conclusion, I think that the temporary statement was buggy behaviour and it was not according to spec. The fixed behaviour is correct, fixes the memory leak, and retains the error message from the failed query.

[mdsnins]

I just watched this PR :P
Thanks Kamil for cool patch, and I strongly agree that temporal PS is useless and removing it doesn't make any side effects.

[cmb69]

Thank you for the explanation!