Fixed GH-13167 Fixed the behavior of bindValue and bindParam.

[SakiTakamachi]

Closes #13167

[kamil-tekiela]

Is this correct? You are binding the param as an int, so why is this trying to convert it to a string?

[kamil-tekiela]

What happens if it's bindParam instead? Does the type of the variable get cast too?

[kamil-tekiela]

My initial thought is that this is a fix, but this whole thing should probably be properly cleaned up. I think we should merge your fix and think if and how we can tidy it all up.

Ideally, PDO_MySQL should attempt to cast to the appropriate type. We need to make sure that there is no data loss, though.

I appreciate your research. It helps a lot.

[SakiTakamachi]

My initial thought is that this is a fix, but this whole thing should probably be properly cleaned up. I think we should merge your fix and think if and how we can tidy it all up.

Ideally, PDO_MySQL should attempt to cast to the appropriate type. We need to make sure that there is no data loss, though.

Yes, I also think that this is not a fundamental solution.

I mentioned this on the mailing list before, but it is also related to the following issue:
#12603

Anyway, there is a "variable type" and a "type specified by the parameter" here, and the behavior when they conflict is really messed up. Of course, as you mentioned, there is no need for same behavior all drivers.

However, if we modify these, it will definitely result in a BC Break, so I may need an RFC...

[kamil-tekiela]

Hi Saki,

I have been thinking about it and I have some points to consider. First, let me preface this by stating that I was not able to find any functional specification for PDO bindParam() or bindValue. The PHP manual doesn't explain it well either. So let's assume that what has been implemented was done according to some spec and should not be changed without RFC apart from bugs.

The current PDO logic that applies to all drivers consists of these 3 actions:

• if param type hint is PARAM_STR and not PARAM_INPUT_OUTPUT, and the value is not NULL value -> convert to string
• if param type hint is PARAM_INT and value TRUE/FALSE -> convert to integer
• if param type hint is PARAM_BOOL and value is INTEGER -> convert to bool

When using bindValue it only converts the argument's value. When using bindParam it casts the variable that was bound by reference.

I am not an expert on other DB types so I will focus on MySQL only. In PDO_MySQL we mostly ignore the type hint. Apart from PARAM_NULL and PARAM_LOB, we do not have/need extra logic, thus the type hints are unused. But mysqlnd has optimizations for doubles and integers. I believe this is why PDO_MySQL uses the variable type and tries to bind it as an INT or DOUBLE when the param type hint is not PARAM_STR. As the code comment says, I am not sure how wise this is, but it was like that forever, so we should keep it.

When emulation mode is switched on, a different part of the code is used. Instead of binding the parameter values to the execute command, the values are passed to the quote() function and then concatenated into the SQL. The logic is as follows:
If NOT LOB:

• If PARAM_BOOL -> replace with 1 or 0
• If PARAM_INT -> convert the value to an integer and then convert the integer to a string
• If PARAM_NULL -> replace with 'NULL'
• Else Try to convert it into a string and quote the string using DB-specific quoter

PDO implements an EVENT system for PDO drivers to hook some logic, so it is possible for other PDO drivers to modify the value or type hint prior to this. I believe PGSQL does this in case of boolean as an example. MySQL doesn't use this feature.

---

Now, when it comes to your PR and the reported issue, I believe it's a bug that MySQL silently borks when a non-scalar and non-null value is bound. When using emulation, it correctly throws an error or at least a warning due to the "else" part above. Binding a STRINGABLE object in non-emulation mode causes the statement to silently fail. In emulation mode it converts the object to 1, albeit with a warning. I don't know which is worse. We could implement support for Stringable objects, but this creates a bit of an issue.

We should ideally not touch the emulation mode logic, and in non-emulation mode, we should respect the variable type according to the current logic. So when using PARAM_INT the following happens now:

Value type:	Emulation	Non-emulation
null	null	null
scalar	int(normal conversion rules apply)	the variable is bound as either string, bool, double or int
array	UNDEFINED BEHAVOUR (int) $array = 0 for empty array, 1 for non-empty	the statement silently borks
resource	the resource ID resulting from (int) $resource	the statement silently borks
object	UNDEFINED BEHAVOUR (int) $object which results in 1	the statement silently borks

With your current fix, we would get this:

Value type:	Emulation	Non-emulation
null	null	null
scalar	int(normal conversion rules apply)	the variable is bound as either string, bool, double or int
array	UNDEFINED BEHAVOUR (int) $array = 0 for empty array, 1 for non-empty	"Array" + Warning: Array to string conversion
resource	the resource ID resulting from (int) $resource	"Resource id #N"
object	UNDEFINED BEHAVOUR (int) $object which results in 1	The result of __toString if present else Error: Object of class stdClass could not be converted to string

So the question here is what should happen when a non-scalar value is bound using PARAM_INT in PDO_MySQL. As of now, there is no defined behaviour. If we try to convert the value to an integer to avail of the binary's protocol optimizations, it would be wrong. We cannot convert an array, resource, or object to an int in any meaningful way. If we say that the value should be converted to a string, we'd also be wrong as the documentation says "directly converting an array, object, or resource to a string does not provide any useful information about the value". We could implement a driver error, such as:

pdo_raise_impl_error(stmt->dbh, stmt, "HY105", "Expected a scalar value or null");

but then we would exclude Stringable objects.

Which is best? One way or another, we would have a different logic than what emulation mode offers, and I think that's ok. Casting to string or int is technically meaningless. The only special case is casting to string a stringable object. But if the user explicitly type hinted the param as an int, I doubt they expected it to get cast to a string. I'd assume 99.99% of users would be using PARAM_STR when binding a stringable object. The 0.01% that accidentally type-hinted it as an integer should see an error. Therefore, as of now, I have come to the conclusion that it would be better to just implement a driver error. I think we could even put the fix in a patch release, but it's probably safer to put it on master only. After all, we are adding an error in a place where there was none before.

---

In regards to what I said earlier about the variable getting changed when using bindParam(). If we want to proceed with the current fix, then you should first make a copy of the ZVAL. There is no precedent for a driver-specific logic changing user variable. This only happens in PDO generic code and even then it's probably wrong.

[SakiTakamachi]

@kamil-tekiela
Thanks for the detailed investigation!

We should ideally not touch the emulation mode logic, and in non-emulation mode, we should respect the variable type according to the current logic.

Casting to string or int is technically meaningless. The only special case is casting to string a stringable object. But if the user explicitly type hinted the param as an int, I doubt they expected it to get cast to a string. I'd assume 99.99% of users would be using PARAM_STR when binding a stringable object. The 0.01% that accidentally type-hinted it as an integer should see an error. Therefore, as of now, I have come to the conclusion that it would be better to just implement a driver error. I think we could even put the fix in a patch release, but it's probably safer to put it on master only. After all, we are adding an error in a place where there was none before.

Agree. I think it's a good idea to raise an error for anything that is not a scalar value, including Stringable objects. (except for null and blob cases)
That is, it should look like this:

default:
	pdo_raise_impl_error(stmt->dbh, stmt, "HY105", "Expected a scalar value or null");
	PDO_DBG_RETURN(0);

Is my understanding correct?

---

In regards to what I said earlier about the variable getting changed when using bindParam(). If we want to proceed with the current fix, then you should first make a copy of the ZVAL. There is no precedent for a driver-specific logic changing user variable. This only happens in PDO generic code and even then it's probably wrong.

This probably won't change.

The following code:

<?php

$db = new PDO(/* mysql */);
$stmt = $db->prepare('SELECT ?');

$val = '1';
$stmt->bindParam(1, $val, PDO::PARAM_INT);

$val = '5';
$stmt->execute();

var_dump($stmt->fetchAll(PDO::FETCH_ASSOC), $val);

result:

array(1) {
  [0]=>
  array(1) {
    [5]=>
    int(5)
  }
}
string(1) "5"

In this way, if we change the value of a variable after bindParam is executed, it will be reflected. And although PARAM_INT is specified, $val remains string.

[kamil-tekiela]

Is my understanding correct?

Yes, you are correct.

---

In this way, if we change the value of a variable after bindParam is executed, it will be reflected. And although PARAM_INT is specified, $val remains string.

Wow, I completely forgot how this works. I apologize as this is a large gap in my analysis. However, I still stand by my recommendation to raise an implementation error. The rest is probably out of the scope for this bug fix and I will rethink this more carefully.

[kamil-tekiela]

Considering that there could be a difference between and after when using bindParam with PARAM_STR, I think it would be wise to add support for Stringable objects to PDO_MySQL.
Something like:

	case IS_OBJECT:
		if(zend_class_implements_interface(Z_OBJCE_P(parameter), zend_ce_stringable)) {
			mysqlnd_stmt_bind_one_param(S->stmt, param->paramno, parameter, MYSQL_TYPE_VAR_STRING);
			break;
		}
		ZEND_FALLTHROUGH;
	default:
		pdo_raise_impl_error(stmt->dbh, stmt, "HY105", "Expected a scalar value or null");
		PDO_DBG_RETURN(0);

What do you think?

[SakiTakamachi]

@kamil-tekiela

Oh, that's nice! Makes sense to me.

[SakiTakamachi]

This error should behave the same even if we don't specify PARAM_INT, right?

If it is not specified (that is, when it is PARAM_STR), a "warning" is displayed saying that the object cannot be converted to a string, so I will take a closer look.

[SakiTakamachi]

Probably here. This is a common specification for pdo, so we may not need to change it when using PARAM_STR and don't worry about it...

php-src/ext/pdo/pdo_stmt.c

Line 427 in 0941507

if (!really_register_bound_param(&param, stmt, 1)) {

---

In the future, it may be necessary to change this from a warning to an error, but it seems better to discuss that separately.

[SakiTakamachi]

I modified it a bit and the test passed with libmysql as well.

[kamil-tekiela]

Can you also add a test like this and after it passes this PR can be merged.

echo "Stringable object, value set after bindParam:\n";
try {
    $stmt = $db->prepare('SELECT ?');
    $param = 'foo';
    $stmt->bindParam(1, $param, PDO::PARAM_STR);
    $param = $stringableObject;
    $stmt->execute();
    var_dump(is_object($param), $param === $stringableObject);
} catch (Throwable $e) {
    echo $e->getMessage();
}

with expectation:

bool(true)
bool(true)

@nielsdos Could you have a second look over it, please, just to make sure it makes sense?

[nielsdos]

Makes sense to me.
Tested with both mysqlnd and libmysqlclient, works fine.

[SakiTakamachi]

@kamil-tekiela
Thanks!