Skip to content

Fix mysqlnd : number that is bigger than INT64_MAX will be casted to INT64_MAX rather than string. #7837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Fix mysqlnd : number that is bigger than INT64_MAX will be casted to INT64_MAX rather than string. #7837

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1592344
Fix value that is bigger than INT64_MAX will be casted to INT64_MAX r…
NathanFreeman Dec 27, 2021
d73b8b9
unit test
NathanFreeman Dec 28, 2021
cfed58c
use marco.
NathanFreeman Dec 28, 2021
d36d8de
unit test.
NathanFreeman Dec 28, 2021
8e8e553
revert it.
NathanFreeman Dec 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ext/mysqlnd/mysqlnd_wireprotocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1614,9 +1614,9 @@ php_mysqlnd_rowp_read_text_protocol(MYSQLND_ROW_BUFFER * row_buffer, zval * fiel
} else {
uint64_t v =
#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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_strtoui64() is available as of VS 2015 at least, and used from the Zend Engine, so the Windows specific code is not necessary.

Copy link
Contributor Author

@NathanFreeman NathanFreeman Dec 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 Thanks for your review!
I use ZEND_ATOL and ZEND_STRTOUL marco to make it compatible with different os.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will revert it as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted it. @cmb69

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bool uns = fields_metadata[i].flags & UNSIGNED_FLAG? TRUE:FALSE;
/* We have to make it ASCIIZ temporarily */
Expand Down
55 changes: 55 additions & 0 deletions ext/pdo_mysql/tests/unsigned_bigint.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
--TEST--
Fix value that is bigger than INT64_MAX will be casted to INT64_MAX rather than string.
--EXTENSIONS--
pdo
pdo_mysql
mysqlnd
--SKIPIF--
<?php
require_once(__DIR__ . DIRECTORY_SEPARATOR . 'mysql_pdo_test.inc');
MySQLPDOTest::skip();
if (!MySQLPDOTest::isPDOMySQLnd()) die('skip only for mysqlnd');
?>
--FILE--
<?php
require_once(__DIR__ . DIRECTORY_SEPARATOR . 'mysql_pdo_test.inc');
$pdo = MySQLPDOTest::factory();

$tbl = "test";
$pdo->query("DROP TABLE IF EXISTS $tbl");
$pdo->query("CREATE TABLE $tbl (`ubigint` bigint unsigned NOT NULL) ENGINE=InnoDB");
$pdo->query("INSERT INTO $tbl (`ubigint`) VALUES (18446744073709551615)");
$pdo->query("INSERT INTO $tbl (`ubigint`) VALUES (9223372036854775808)");
$pdo->query("INSERT INTO $tbl (`ubigint`) VALUES (9223372036854775807)");
$pdo->query("INSERT INTO $tbl (`ubigint`) VALUES (1)");
$result = $pdo->query("SELECT ubigint FROM $tbl")->fetchAll(PDO::FETCH_ASSOC);
var_dump($result);
?>
--CLEAN--
<?php
require dirname(__FILE__) . '/mysql_pdo_test.inc';
MySQLPDOTest::dropTestTable();
?>
--EXPECT--
array(4) {
[0]=>
array(1) {
["ubigint"]=>
string(20) "18446744073709551615"
}
[1]=>
array(1) {
["ubigint"]=>
string(19) "9223372036854775808"
}
[2]=>
array(1) {
["ubigint"]=>
int(9223372036854775807)
}
[3]=>
array(1) {
["ubigint"]=>
int(1)
}
}