Fix mysqlnd : number that is bigger than INT64_MAX will be casted to INT64_MAX rather than string. #7837
Conversation
…ather than string.
There was a problem hiding this comment.
Thank you for the PR! Since this is a bug fix, it should probably be applied to PHP-8.0+.
@kamil-tekiela, do you see any issues with this patch?
ext/mysqlnd/mysqlnd_wireprotocol.c
Outdated
| #ifndef PHP_WIN32 | ||
| (uint64_t) atoll((char *) p); | ||
| (uint64_t) strtoull((char *) p, NULL, 10); | ||
| #else | ||
| (uint64_t) _atoi64((char *) p); | ||
| (uint64_t) _strtoui64((char *) p, NULL, 10); | ||
| #endif |
There was a problem hiding this comment.
_strtoui64() is available as of VS 2015 at least, and used from the Zend Engine, so the Windows specific code is not necessary.
There was a problem hiding this comment.
@cmb69 Thanks for your review!
I use ZEND_ATOL and ZEND_STRTOUL marco to make it compatible with different os.
There was a problem hiding this comment.
While this would work, it is confusing on 32bit architectures at best, because ZEND_STRTOUL() yields an uint32_t there, so truncation occurs.
Also I misread the code; your original commit (1592344) was fine; I suggest to revert cfed58c.
We can still simplify the whole integer handling in php_mysqlnd_rowp_read_text_protocol() in the master branch later.
There was a problem hiding this comment.
OK, I will revert it as soon as possible.
There was a problem hiding this comment.
I have reverted it. @cmb69
There was a problem hiding this comment.
I am no C expert and I can't compile it on Windows at the moment, but it looks alright to me. It fixes the bug on Linux so I assume it also works on Windows.
Shouldn't we bring this fix to PHP 8.0 as well?
There was a problem hiding this comment.
It seems to me Windows behaves the same as other systems. Without the patch, the test succeeds on x86, but fails on x64:
[0]=>
array(1) {
["ubigint"]=>
005+ int(9223372036854775807)
005- string(20) "18446744073709551615"
}
[1]=>
array(1) {
["ubigint"]=>
010+ int(9223372036854775807)
010- string(19) "9223372036854775808"
}
[2]=>
array(1) {With the patch, the test passed on x64 and x86.
I agree that we should fix this issue for PHP-8.0 as well, but I'm not able to get unstringified results there at all (as of PHP 8.1.0, this is the default). Calling $pdo->setAttribute(PDO::ATTR_STRINGIFY_FETCHES, false) has apparently no effect, what might be a bug on its own.
MySQLi exhibits the same behavior, and there I can call $mysql->options(MYSQLI_OPT_INT_AND_FLOAT_NATIVE, true); to get unstringified results. I have no idea, though, about the behavior of libmysql-client; we could skip the test for these cases, though.
There was a problem hiding this comment.
Yes, this functionality has only been implemented in PDO_MySQL since PHP 8.1 hence why this PR is only fixing this in PHP 8.1. The bug was present in PHP 8.0 also, but would only come up when using mysqli. I tested it using PHP 8.0 and mysqli and this patch fixes it.
I don't think we should be concerned about libmysql because it doesn't support PHP native types anyway. The bug was on mysqlnd so we should only test for mysqlnd.
NathanFreeman
changed the title
There was a problem hiding this comment.
Thanks for the patch, @NathanFreeman!
And thanks for the clarification, @kamil-tekiela! I'm going to apply to PHP-8.0 then (with a test for mysqli), and will merge upward (plus the PDO test).
|
@cmb69 @kamil-tekiela |
mysqlnd will cast the value that is bigger than INT64_MAX (2^63 - 1) to INT64_MAX rather than string.