BIGINT as an output param no longer results in value out of range exception when the returned value is larger than a maximum integer #567

yukiwongky · 2017-10-12T01:04:16Z

When user initialize an integer in php: $output = 0; php interpret this variable as a zend_long. zend_long has a member of c type long to contain the value. A c long is 4 bytes and the max and min size are 2,147,483,647 and -2,147,483,647 respectively.

If the user use $output to bind parameter:

$stmt = sqlsrv_prepare($conn, "{call storedproc (?)}", array(array($output, SQLSRV_PARAM_OUT)));
sqlsrv_execute($stmt);

Everything will be fine if the output of calling the stored procedure is < 2,147,483,647 and > -2,147,483,647. If otherwise however, a value out of range exception is thrown.

The source of misbehavior comes from 2 places:

In the source code, the SQLSRV and PDO_SQLSRV drivers derive the sqltype passed to the MSODBC driver from the zend type if the sqltype is not provided by the user. If the zend type is zend_long, the sqltype is set to sql_int. Therefore is the output is greater than max int, the value out of range exception is thrown from SQL Server.
Even if the sqltype is forced to be sql_bigint and SQL Server no longer throws an exception, the output variable zend_long is still not big enough to fit the bigint.

To tackle these two problems, when $output is first passed to bind parameter, convert it to a zend_string. Then the zend type is zend_string, the sqltype set by the SQLSRV and PDO_SQLSRV drivers are SQL_* character strings types depending on the encoding. Passing a character strings type to MSODBC has no problem since almost every datatypes can be converted to a character string.

When the output it retrieved, the drivers now compare to see if the retrieved value is less than max int. If it is, convert it back to zend_long (the reason to convert back is so it doesn't affect the existing users). If it is larger than max int, leave it as a zend_string, since zend_long will not hold the value properly.

This change is

…s than max int

coveralls · 2017-10-12T01:19:28Z

Coverage increased (+0.07%) to 73.163% when pulling a0e8862 on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

… output param

coveralls · 2017-10-12T18:57:11Z

Coverage increased (+0.1%) to 73.232% when pulling c30f2d6 on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

…_bigint_outparam.phpt

coveralls · 2017-10-12T19:30:29Z

Coverage increased (+0.1%) to 73.232% when pulling 628e04e on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

yukiwongky · 2017-10-12T20:26:18Z

source/shared/core_stmt.cpp

-                match = Z_TYPE_P( param_z ) == IS_LONG;
+                if( zval_was_long ){
+                    convert_to_string( param_z );
+					encoding = SQLSRV_ENCODING_SYSTEM;


will fix indentation

yukiwongky · 2017-10-12T20:26:39Z

source/shared/core_stmt.cpp

@@ -551,7 +565,7 @@ void core_sqlsrv_bind_param( _Inout_ sqlsrv_stmt* stmt, _In_ SQLUSMALLINT param_
                                                   buffer, buffer_len TSRMLS_CC );

                // save the parameter to be adjusted and/or converted after the results are processed
-				sqlsrv_output_param output_param( param_ref, encoding, param_num, static_cast<SQLUINTEGER>( buffer_len ));
+				sqlsrv_output_param output_param( param_ref, encoding, param_num, static_cast<SQLUINTEGER>( buffer_len ), zval_was_long );


will fix indentation

david-puglielli · 2017-10-12T22:18:50Z

source/shared/core_stmt.cpp

+                if( zval_was_long ){
+                    convert_to_string( param_z );
+                    encoding = SQLSRV_ENCODING_SYSTEM;
+                }


Do any encoding issues relating to use of commas as separators crop up? I should think that convert_to_string just takes the int and converts to a string of digits, eg. 1,000,000 becomes '1000000'. But if not, do we run into any issues handling commas when calling convert_to_double or convert_to_long below?

I've tried testing with both encodings (utf-8 and char) and they both are fine. From wheat I researched, user can only retrieve a comma separated integer by explicitly casting an int to a string. If that is the case, the php_out_type would not be INT anymore, instead it would be a string, and the problem with bigint to int conversion would not exist.

As for the second question "do we run into any issues handling commas when calling convert_to_double or convert_to_long below", this will not be a problem since the string retrieved will not have any comma separators.

david-puglielli · 2017-10-12T22:20:41Z

test/functional/pdo_sqlsrv/pdo_bigint_outparam.phpt

+$tbname = "bigint_table";
+createTable($conn, $tbname, array("c1_bigint" => "bigint"));
+
+// Create a Store Procedure


*stored

(here and elsewhere)

david-puglielli · 2017-10-12T22:36:11Z

test/functional/pdo_sqlsrv/pdo_bigint_outparam.phpt

+$conn->exec("TRUNCATE TABLE $tbname");
+
+// Insert a small bigint
+insertRow($conn, $tbname, array("c1_bigint" => 922337203));


Is there any particular reason for choosing this value?

this is just a random value truncated from the bigint input

perhaps add that to comment?

coveralls · 2017-10-13T23:16:17Z

Coverage increased (+0.1%) to 73.232% when pulling 83ca7b4 on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

coveralls · 2017-10-13T23:29:17Z

Coverage increased (+0.1%) to 73.232% when pulling 83ca7b4 on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

codecov-io · 2017-10-16T16:42:22Z

Codecov Report

Merging #567 into dev will decrease coverage by 0.64%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev     #567      +/-   ##
==========================================
- Coverage   73.67%   73.02%   -0.65%     
==========================================
  Files         100       75      -25     
  Lines       30384    23054    -7330     
==========================================
- Hits        22384    16835    -5549     
+ Misses       8000     6219    -1781

Impacted Files	Coverage Δ
...x64/php-7.0.24-src/ext/sqlsrv/shared/core_sqlsrv.h	`33.18% <0%> (ø)`	⬆️
...php-7.0.24-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h	`34.84% <0%> (ø)`	⬆️
...php/x64/php-7.1.10-src/ext/pdo_sqlsrv/pdo_stmt.cpp
...rojects/php/x64/php-7.1.10-src/ext/sqlsrv/stmt.cpp
...php-7.1.10-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h
...p-7.1.10-src/ext/pdo_sqlsrv/shared/core_stream.cpp
...x64/php-7.1.10-src/ext/sqlsrv/shared/core_util.cpp
...php-7.1.10-src/ext/pdo_sqlsrv/shared/core_conn.cpp
...php-7.1.10-src/ext/pdo_sqlsrv/shared/core_stmt.cpp
...4/php-7.1.10-src/ext/sqlsrv/shared/core_stream.cpp
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed16852...6a4f625. Read the comment docs.

coveralls · 2017-10-16T16:53:09Z

Coverage increased (+0.1%) to 73.232% when pulling e190bca on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

coveralls · 2017-10-16T20:51:46Z

Coverage increased (+0.1%) to 73.232% when pulling e190bca on v-kaywon:bigintOutput into edac5b2 on Microsoft:dev.

coveralls · 2017-10-17T22:48:18Z

Coverage increased (+0.09%) to 73.182% when pulling 6b223a7 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-17T22:54:08Z

Coverage increased (+0.1%) to 73.219% when pulling 9e847c1 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-17T23:03:26Z

Coverage increased (+0.3%) to 73.355% when pulling f67240e on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-17T23:11:36Z

Coverage increased (+0.1%) to 73.22% when pulling f67240e on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-17T23:16:41Z

Coverage increased (+0.1%) to 73.22% when pulling a5125df on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-17T23:28:47Z

Coverage increased (+0.09%) to 73.186% when pulling 68df220 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-17T23:43:46Z

Coverage increased (+0.09%) to 73.186% when pulling 68df220 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T16:42:27Z

Coverage increased (+0.09%) to 73.186% when pulling 3f17147 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T19:14:05Z

Coverage increased (+0.1%) to 73.232% when pulling b73539c on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T20:05:55Z

Coverage increased (+0.1%) to 73.198% when pulling f4c736a on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T21:21:13Z

Coverage increased (+0.08%) to 73.177% when pulling 78da835 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T21:57:52Z

Coverage increased (+0.08%) to 73.177% when pulling 78da835 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T22:17:13Z

Coverage increased (+0.08%) to 73.177% when pulling 78da835 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

coveralls · 2017-10-18T23:50:29Z

Coverage increased (+0.1%) to 73.198% when pulling 6a4f625 on v-kaywon:bigintOutput into ed16852 on Microsoft:dev.

convert long output param to string, then convert back to long if les…

a0e8862

…s than max int

yukiwongky changed the title ~~convert long output param to string, then convert back to long if les…~~ convert long output param to string, then convert back to long if less than max int Oct 12, 2017

change encoding to SYSTEM when converting int to string for inout and…

c30f2d6

… output param

fix spacing in expected output in pdo_bigint_outparam.phpt and sqlsrv…

628e04e

…_bigint_outparam.phpt

yukiwongky commented Oct 12, 2017

View reviewed changes

yukiwongky requested review from david-puglielli and lilgreenbird October 12, 2017 20:27

david-puglielli reviewed Oct 12, 2017

View reviewed changes

lilgreenbird approved these changes Oct 13, 2017

View reviewed changes

yukiwongky added 2 commits October 13, 2017 15:50

cosmetic changes

b5a0d1f

add comment do pdo_bigint_outparam.phpt and sqlsrv_bitint_outparam.phpt

83ca7b4

david-puglielli approved these changes Oct 13, 2017

View reviewed changes

fix bigint input in sqlsrv_bigint_outparam.phpt

e190bca

yukiwongky closed this Oct 16, 2017

yukiwongky reopened this Oct 16, 2017

yukiwongky closed this Oct 16, 2017

yukiwongky reopened this Oct 16, 2017

yukiwongky added 2 commits October 17, 2017 11:27

Merge remote-tracking branch 'upstream/dev' into bigintOutput

fd5c2b9

detect Appveyor problem

9b12fe3

detect appveyor problem

9e847c1

change encoding to converting from long to string to UTF8

f67240e

change output biging to encoding utf8

d7212e6

undo changes in appveyor.yml

a5125df

undo changes in appveyor.yml

68df220

add contrinue_after_cpp_exception option to OpenCppCoverage

3f17147

remove setting encoding in core_stmt.cpp

b73539c

only change encoding if it is invalid

f4c736a

yukiwongky added 3 commits October 18, 2017 13:08

change encoding

f2f3bc1

change appveyor SQL2016 to run on x86 platform

02b3d6d

move opencppcoverage.exe down in appveyor.xml

78da835

change encoding

6a4f625

yukiwongky merged commit def61df into microsoft:dev Oct 19, 2017

yukiwongky changed the title ~~convert long output param to string, then convert back to long if less than max int~~ BIGINT as an output param no longer results in value out of range exception when the returned value is larger than a maximum integer Oct 20, 2017

yukiwongky deleted the bigintOutput branch January 15, 2018 20:41

david-puglielli mentioned this pull request Apr 11, 2018

Reverted change handling bigint output parameters #744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIGINT as an output param no longer results in value out of range exception when the returned value is larger than a maximum integer #567

BIGINT as an output param no longer results in value out of range exception when the returned value is larger than a maximum integer #567

yukiwongky commented Oct 12, 2017 •

edited

Loading

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

yukiwongky Oct 12, 2017

yukiwongky Oct 12, 2017

david-puglielli Oct 12, 2017

yukiwongky Oct 13, 2017

david-puglielli Oct 12, 2017 •

edited

Loading

david-puglielli Oct 12, 2017

yukiwongky Oct 13, 2017

lilgreenbird Oct 13, 2017 •

edited

Loading

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017

codecov-io commented Oct 16, 2017 •

edited

Loading

coveralls commented Oct 16, 2017

coveralls commented Oct 16, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

BIGINT as an output param no longer results in value out of range exception when the returned value is larger than a maximum integer #567

BIGINT as an output param no longer results in value out of range exception when the returned value is larger than a maximum integer #567

Conversation

yukiwongky commented Oct 12, 2017 • edited Loading

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

coveralls commented Oct 12, 2017

yukiwongky Oct 12, 2017

Choose a reason for hiding this comment

yukiwongky Oct 12, 2017

Choose a reason for hiding this comment

david-puglielli Oct 12, 2017

Choose a reason for hiding this comment

yukiwongky Oct 13, 2017

Choose a reason for hiding this comment

david-puglielli Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

david-puglielli Oct 12, 2017

Choose a reason for hiding this comment

yukiwongky Oct 13, 2017

Choose a reason for hiding this comment

lilgreenbird Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017

codecov-io commented Oct 16, 2017 • edited Loading

Codecov Report

coveralls commented Oct 16, 2017

coveralls commented Oct 16, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017 • edited Loading

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 17, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017

yukiwongky commented Oct 12, 2017 •

edited

Loading

david-puglielli Oct 12, 2017 •

edited

Loading

lilgreenbird Oct 13, 2017 •

edited

Loading

codecov-io commented Oct 16, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading