Fix for Issue #16910 #16911
Fix for Issue #16910 #16911
Conversation
Prevent truncation of fractional seconds in binary protocol if server sends AUTO_SEC_PART_DIGITS (value=39) in decimals value of metadata for TIME or DATETIME types.
| if (field->decimals > 0 && (field->decimals <= MYSQLND_SEC_PART_DIGITS || | ||
| (field->decimals == MYSQLND_AUTO_SEC_PART_DIGITS && t.second_part))) { | ||
| uint8_t decimals= (field->decimals == MYSQLND_AUTO_SEC_PART_DIGITS) ? MYSQLND_SEC_PART_DIGITS : field->decimals; |
| if (field->decimals > 0 && (field->decimals <= MYSQLND_SEC_PART_DIGITS || | ||
| (field->decimals == MYSQLND_AUTO_SEC_PART_DIGITS && t.second_part))) { | ||
| uint8_t decimals= (field->decimals == MYSQLND_AUTO_SEC_PART_DIGITS) ? MYSQLND_SEC_PART_DIGITS : field->decimals; |
There was a problem hiding this comment.
I think you should probably replace field->decimals with decimals in zend_strpprintf below.
| @@ -0,0 +1,40 @@ | |||
| --TEST-- | |||
| GH-xxxxx (AUTO_SEC_PART_DIGITS (field->decimal value=39) ignored in binary protocol) | |||
There was a problem hiding this comment.
| GH-xxxxx (AUTO_SEC_PART_DIGITS (field->decimal value=39) ignored in binary protocol) | |
| GH-16910 (AUTO_SEC_PART_DIGITS (field->decimal value=39) ignored in binary protocol) |
bukka
requested review from
kocsismate,
dstogov,
arnaud-lb,
nielsdos,
Girgias,
derickr,
adoy and
TimWolla
as code owners
|
This seems to be based against old master - I recently did some security fixes in |
bukka
removed request for
adoy,
derickr,
TimWolla,
arnaud-lb,
dstogov,
kocsismate,
nielsdos and
Girgias
|
Sorry for adding the reviewers - thought that the conflict is because of the wrong base but it was actually just based on old master... |
| --TEST-- | ||
| GH-xxxxx (AUTO_SEC_PART_DIGITS (field->decimal value=39) ignored in binary protocol) | ||
| --EXTENSIONS-- | ||
| mysqli | ||
| --SKIPIF-- | ||
| <?php | ||
| require_once 'skipifconnectfailure.inc'; | ||
| ?> | ||
| --FILE-- | ||
| <?php | ||
| require_once 'connect.inc'; | ||
|
|
||
| if (!$link = my_mysqli_connect($host, $user, $passwd, $db, $port, $socket)) { | ||
| printf("Cannot connect to the server using host=%s, user=%s, passwd=***, dbname=%s, port=%s, socket=%s\n", | ||
| $host, $user, $db, $port, $socket); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Values from binary and text protocol should be the same | ||
| $stmt = $link->prepare("select FROM_UNIXTIME('1992.1'), FROM_UNIXTIME('1992.0')"); | ||
| $stmt->execute(); | ||
| $stmt->bind_result($binval[0],$binval[1]); | ||
| $stmt->fetch(); | ||
| $stmt->close(); | ||
|
|
||
| $result = $link->query("select FROM_UNIXTIME('1992.1'), FROM_UNIXTIME('1992.0')"); | ||
| $txtval = $result->fetch_row(); | ||
| $result->close(); | ||
| $err_cnt= 0; | ||
|
|
||
| for ($i=0; $i < 2; $i++) | ||
| if ($txtval[$i] != $binval[$i]) | ||
| printf("[%03d] Expected '%s' (text protocol), got '%s' (binary protocol)", | ||
| $err_cnt++, $txtval[$i], $binval[$i]); | ||
|
|
||
| if ($err_cnt == 0) | ||
| print "done!"; | ||
| ?> | ||
| --EXPECT-- | ||
| done! |
There was a problem hiding this comment.
So after checking the code I had a little suspicion about this test so I gave it a try and it passes without the patch so it doesn't look like it tests the actual behaviour - at least not with MySQL version (8.40). And seems like it won't probably fail in CI either. If it's DB dependent and not easy to recreate using DB in CI, then please add a direct protocol test using the fake server that I recently added. Also see an example for datetime stmt (binary protocol) and generic query usage. The main part is always in fake_server though.
Prevent truncation of fractional seconds in binary protocol if server sends AUTO_SEC_PART_DIGITS (value=39) in decimals value of metadata for TIME or DATETIME types.