From 78ba40f3a23bd3185bd87aa6b50d81283160ba6b Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Mon, 22 Jun 2020 12:34:31 -0700 Subject: [PATCH 1/2] Check exception when finalizing output parameters --- source/shared/core_sqlsrv.h | 2 +- source/shared/core_stmt.cpp | 10 +- source/shared/core_util.cpp | 2 +- .../pdo_ae_output_param_errors.phpt | 138 +++++++++++++++++ .../sqlsrv/sqlsrv_ae_output_param_errors.phpt | 143 ++++++++++++++++++ 5 files changed, 290 insertions(+), 5 deletions(-) create mode 100644 test/functional/pdo_sqlsrv/pdo_ae_output_param_errors.phpt create mode 100644 test/functional/sqlsrv/sqlsrv_ae_output_param_errors.phpt diff --git a/source/shared/core_sqlsrv.h b/source/shared/core_sqlsrv.h index 0dae9a0ea..72925e1cb 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -1913,7 +1913,7 @@ enum error_handling_flags { // 3/message) driver specific error message // The fetch type determines if the indices are numeric, associative, or both. bool core_sqlsrv_get_odbc_error( _Inout_ sqlsrv_context& ctx, _In_ int record_number, _Inout_ sqlsrv_error_auto_ptr& error, - _In_ logging_severity severity, _In_ bool check_warning = false ); + _In_ logging_severity severity, _In_opt_ bool check_warning = false ); // format and return a driver specfic error void core_sqlsrv_format_driver_error( _In_ sqlsrv_context& ctx, _In_ sqlsrv_error_const const* custom_error, diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index 54a49f147..bbf06bfce 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -112,7 +112,7 @@ void col_cache_dtor( _Inout_ zval* data_z ); void field_cache_dtor( _Inout_ zval* data_z ); int round_up_decimal_numbers(_Inout_ char* buffer, _In_ short decimal_pos, _In_ short decimals_places, _In_ short offset, _In_ short lastpos); void format_decimal_numbers(_In_ SQLSMALLINT decimals_places, _In_ SQLSMALLINT field_scale, _Inout_updates_bytes_(*field_len) char*& field_value, _Inout_ SQLLEN* field_len); -void finalize_output_parameters( _Inout_ sqlsrv_stmt* stmt ); +void finalize_output_parameters( _Inout_ sqlsrv_stmt* stmt, _In_opt_ bool exception_thrown = false ); void get_field_as_string( _Inout_ sqlsrv_stmt* stmt, _In_ SQLUSMALLINT field_index, _Inout_ sqlsrv_phptype sqlsrv_php_type, _Inout_updates_bytes_(*field_len) void*& field_value, _Inout_ SQLLEN* field_len ); stmt_option const* get_stmt_option( sqlsrv_conn const* conn, _In_ zend_ulong key, _In_ const stmt_option stmt_opts[] ); @@ -820,7 +820,7 @@ SQLRETURN core_sqlsrv_execute( _Inout_ sqlsrv_stmt* stmt, _In_reads_bytes_(sql_l // if the statement executed but failed in a subsequent operation before returning, // we need to cancel the statement and deref the output and stream parameters if ( stmt->send_streams_at_exec ) { - finalize_output_parameters( stmt ); + finalize_output_parameters( stmt, true ); zend_hash_clean( Z_ARRVAL( stmt->param_streams )); } if( stmt->executed ) { @@ -2364,10 +2364,14 @@ void format_decimal_numbers(_In_ SQLSMALLINT decimals_places, _In_ SQLSMALLINT f // parameters passed to SQLBindParameter. It also converts output strings from UTF-16 to UTF-8 if necessary. // For integer or float parameters, it sets those to NULL if a NULL was returned by SQL Server -void finalize_output_parameters( _Inout_ sqlsrv_stmt* stmt ) +void finalize_output_parameters( _Inout_ sqlsrv_stmt* stmt, _In_opt_ bool exception_thrown /*= false*/ ) { if (Z_ISUNDEF(stmt->output_params)) return; + if (exception_thrown) { + zend_hash_clean(Z_ARRVAL(stmt->output_params)); + return; + } HashTable* params_ht = Z_ARRVAL(stmt->output_params); zend_ulong index = -1; diff --git a/source/shared/core_util.cpp b/source/shared/core_util.cpp index 2d0c9c589..99b1113a2 100644 --- a/source/shared/core_util.cpp +++ b/source/shared/core_util.cpp @@ -257,7 +257,7 @@ void convert_datetime_string_to_zval(_Inout_ sqlsrv_stmt* stmt, _In_opt_ char* i // 3/message) driver specific error message // The fetch type determines if the indices are numeric, associative, or both. -bool core_sqlsrv_get_odbc_error( _Inout_ sqlsrv_context& ctx, _In_ int record_number, _Inout_ sqlsrv_error_auto_ptr& error, _In_ logging_severity severity, _In_ bool check_warning /* = false */) +bool core_sqlsrv_get_odbc_error( _Inout_ sqlsrv_context& ctx, _In_ int record_number, _Inout_ sqlsrv_error_auto_ptr& error, _In_ logging_severity severity, _In_opt_ bool check_warning /* = false */) { SQLHANDLE h = ctx.handle(); SQLSMALLINT h_type = ctx.handle_type(); diff --git a/test/functional/pdo_sqlsrv/pdo_ae_output_param_errors.phpt b/test/functional/pdo_sqlsrv/pdo_ae_output_param_errors.phpt new file mode 100644 index 000000000..f6654665a --- /dev/null +++ b/test/functional/pdo_sqlsrv/pdo_ae_output_param_errors.phpt @@ -0,0 +1,138 @@ +--TEST-- +Test to incorrectly bind input parameters as output parameters of various types +--DESCRIPTION-- +Test to incorrectly bind input parameters as output parameters of various types. +The key is to enable ColumnEncryption and check for memory leaks. +--SKIPIF-- + +--FILE-- +getAttribute(PDO::ATTR_CLIENT_VERSION)["DriverVer"]; + $vers = explode(".", $msodbcsql_ver); + + unset($conn); + if ($vers[0] >= 17 && $vers[1] > 0){ + return true; + } else { + return false; + } +} + +require_once("MsCommon_mid-refactor.inc"); + +try { + // Check if the ODBC driver supports connecting with ColumnEncryption + // If not simply return + if (!checkODBCVersion()) { + echo "Done\n"; + return; + } + + $conn = connect("ColumnEncryption=Enabled;"); + + // Create a dummy table with various data types + $tbname = 'pdo_output_param_errors'; + $colMetaArr = array("c1_int" => "int", + "c2_smallint" => "smallint", + "c3_tinyint" => "tinyint", + "c4_bit" => "bit", + "c5_bigint" => "bigint", + "c6_decimal" => "decimal(18,5)", + "c7_numeric" => "numeric(10,5)", + "c8_float" => "float", + "c9_real" => "real", + "c10_date" => "date", + "c11_datetime" => "datetime", + "c12_datetime2" => "datetime2", + "c13_datetimeoffset" => "datetimeoffset", + "c14_time" => "time", + "c15_char" => "char(5)", + "c16_varchar" => "varchar(max)", + "c17_nchar" => "nchar(5)", + "c18_nvarchar" => "nvarchar(max)"); + createTable($conn, $tbname, $colMetaArr); + + // Create a dummy select statement + $tsql = "SELECT * FROM $tbname WHERE c1_int = ? OR c2_smallint = ? OR c3_tinyint = ? "; + $tsql .= "OR c4_bit = ? OR c5_bigint = ? OR c6_decimal = ? OR c7_numeric = ? OR c8_float = ? "; + $tsql .= "OR c9_real = ? OR c10_date = ? OR c11_datetime = ? OR c12_datetime2 = ? "; + $tsql .= "OR c13_datetimeoffset = ? OR c14_time = ? OR c15_char = ? "; + $tsql .= "OR c16_varchar = ? OR c17_nchar = ? OR c18_nvarchar = ?"; + + // Initialize all inputs, set bigint, decimal and numeric as empty strings + $intOut = 0; + $smallintOut = 0; + $tinyintOut = 0; + $bitOut = 0; + $bigintOut = ''; + $decimalOut = ''; + $numericOut = ''; + $floatOut = 0.0; + $realOut = 0.0; + $dateOut = '0001-01-01'; + $datetimeOut = '1753-01-01 00:00:00'; + $datetime2Out = '0001-01-01 00:00:00'; + $datetimeoffsetOut = '1900-01-01 00:00:00 +01:00'; + $timeOut = '00:00:00'; + $charOut = ''; + $varcharOut = ''; + $ncharOut = ''; + $nvarcharOut = ''; + + $usage1 = 0; + $rounds = 30; + for ($i = 0; $i < $rounds; $i++) { + $stmt = $conn->prepare($tsql); + + $stmt->bindParam(1, $intOut, PDO::PARAM_INT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE); + $stmt->bindParam(2, $smallintOut, PDO::PARAM_INT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE); + $stmt->bindParam(3, $tinyintOut, PDO::PARAM_INT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE); + $stmt->bindParam(4, $bitOut, PDO::PARAM_INT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE); + $stmt->bindParam(5, $bigintOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(6, $decimalOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(7, $numericOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(8, $floatOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(9, $realOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(10, $dateOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(11, $datetimeOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(12, $datetime2Out, PDO::PARAM_STR, 2048); + $stmt->bindParam(13, $datetimeoffsetOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(14, $timeOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(15, $charOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(16, $varcharOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(17, $ncharOut, PDO::PARAM_STR, 2048); + $stmt->bindParam(18, $nvarcharOut, PDO::PARAM_STR, 2048, PDO::SQLSRV_ENCODING_UTF8); + + // Expect the following to fail so just ignore the exception caught + try { + $stmt->execute(); + } catch (PDOException $e) { + ; + } + unset($stmt); + + // Compare the current memory usage to the previous usage + if ($i == 0) { + $usage1 = memory_get_usage(); + } else { + $usage2 = memory_get_usage(); + if ($usage2 > $usage1) { + echo "Memory leaks ($i)! Expected $usage1 but now $usage2\n"; + } + } + } + + dropTable($conn, $tbname); + unset($conn); +} catch (PDOException $e) { + var_dump($e); +} + +echo "Done\n"; +?> +--EXPECT-- +Done \ No newline at end of file diff --git a/test/functional/sqlsrv/sqlsrv_ae_output_param_errors.phpt b/test/functional/sqlsrv/sqlsrv_ae_output_param_errors.phpt new file mode 100644 index 000000000..4933aacaa --- /dev/null +++ b/test/functional/sqlsrv/sqlsrv_ae_output_param_errors.phpt @@ -0,0 +1,143 @@ +--TEST-- +Test to incorrectly bind input parameters as output parameters of various types +--DESCRIPTION-- +Similar to pdo_ae_output_param_errors.phpt - test to incorrectly bind input parameters +as output parameters of various types. The key is to enable ColumnEncryption and +check for memory leaks. +--SKIPIF-- + +--FILE-- += 17 && $vers[1] > 0){ + return true; + } else { + return false; + } +} + +// Check if the ODBC driver supports connecting with ColumnEncryption +// If not simply return +require_once('MsHelper.inc'); +require_once('MsSetup.inc'); + +$conn = sqlsrv_connect($server, $connectionOptions); +if ($conn === false) { + fatalError("Failed to connect to $server."); +} +if (!checkODBCVersion($conn)) { + echo "Done\n"; + sqlsrv_close($conn); + return; +} + +// Create a dummy table with various data types +$tbname = 'srv_output_param_errors'; +$colMetaArr = array( new AE\ColumnMeta("int", "c1_int"), + new AE\ColumnMeta("smallint", "c2_smallint"), + new AE\ColumnMeta("tinyint", "c3_tinyint"), + new AE\ColumnMeta("bit", "c4_bit"), + new AE\ColumnMeta("bigint", "c5_bigint"), + new AE\ColumnMeta("decimal(18,5)", "c6_decimal"), + new AE\ColumnMeta("numeric(10,5)", "c7_numeric"), + new AE\ColumnMeta("float", "c8_float"), + new AE\ColumnMeta("real", "c9_real"), + new AE\ColumnMeta("date", "c10_date"), + new AE\ColumnMeta("datetime", "c11_datetime"), + new AE\ColumnMeta("datetime2", "c12_datetime2"), + new AE\ColumnMeta("datetimeoffset", "c13_datetimeoffset"), + new AE\ColumnMeta("time", "c14_time"), + new AE\ColumnMeta("char(5)", "c15_char"), + new AE\ColumnMeta("varchar(max)", "c16_varchar"), + new AE\ColumnMeta("nchar(5)", "c17_nchar"), + new AE\ColumnMeta("nvarchar(max)", "c18_nvarchar")); +AE\createTable($conn, $tbname, $colMetaArr); + +// Create a dummy select statement +$sql = "SELECT * FROM $tbname WHERE c1_int = ? OR c2_smallint = ? OR c3_tinyint = ? "; +$sql .= "OR c4_bit = ? OR c5_bigint = ? OR c6_decimal = ? OR c7_numeric = ? OR c8_float = ? "; +$sql .= "OR c9_real = ? OR c10_date = ? OR c11_datetime = ? OR c12_datetime2 = ? "; +$sql .= "OR c13_datetimeoffset = ? OR c14_time = ? OR c15_char = ? "; +$sql .= "OR c16_varchar = ? OR c17_nchar = ? OR c18_nvarchar = ?"; + +$options = array_merge($connectionOptions, + array('ColumnEncryption' => 'Enabled', + 'CharacterSet' => 'UTF-8')); + +// Initialize all inputs, set bigint, decimal and numeric as empty strings +$intOut = 0; +$smallintOut = 0; +$tinyintOut = 0; +$bitOut = 0; +$bigintOut = ''; +$decimalOut = ''; +$numericOut = ''; +$floatOut = 0.0; +$realOut = 0.0; +$dateOut = ''; +$datetimeOut = ''; +$datetime2Out = ''; +$datetimeoffsetOut = ''; +$timeOut = ''; +$charOut = ''; +$varcharOut = ''; +$ncharOut = ''; +$nvarcharOut = ''; + +$usage1 = 0; +$rounds = 30; +for ($i = 0; $i < $rounds; $i++) { + // Connect with ColumnEncryption enabled + $conn2 = sqlsrv_connect($server, $options); + if ($conn2 === false) { + fatalError("Failed to connect to $server."); + } + + $stmt = sqlsrv_prepare($conn2, $sql, array( array( &$intOut, SQLSRV_PARAM_OUT ), + array( &$smallintOut, SQLSRV_PARAM_OUT ), + array( &$tinyintOut, SQLSRV_PARAM_OUT ), + array( &$bitOut, SQLSRV_PARAM_OUT ), + array( &$bigintOut, SQLSRV_PARAM_OUT ), + array( &$decimalOut, SQLSRV_PARAM_OUT ), + array( &$numericOut, SQLSRV_PARAM_OUT ), + array( &$floatOut, SQLSRV_PARAM_OUT ), + array( &$realOut, SQLSRV_PARAM_OUT ), + array( &$dateOut, SQLSRV_PARAM_OUT ), + array( &$datetimeOut, SQLSRV_PARAM_OUT ), + array( &$datetime2Out, SQLSRV_PARAM_OUT ), + array( &$datetimeoffsetOut, SQLSRV_PARAM_OUT ), + array( &$timeOut, SQLSRV_PARAM_OUT ), + array( &$charOut, SQLSRV_PARAM_OUT ), + array( &$varcharOut, SQLSRV_PARAM_OUT ), + array( &$ncharOut, SQLSRV_PARAM_OUT ), + array( &$nvarcharOut, SQLSRV_PARAM_OUT ))); + + // Expect the following to fail so just ignore the errors + sqlsrv_execute($stmt); + + // Compare the current memory usage to the previous usage + if ($i == 0) { + $usage1 = memory_get_usage(); + } else { + $usage2 = memory_get_usage(); + if ($usage2 > $usage1) { + echo "Memory leaks ($i)! Expected $usage1 but now $usage2\n"; + } + } + + // Free the resources to trigger the destruction of any zvals with refcount of 0 + sqlsrv_free_stmt($stmt); + sqlsrv_close($conn2); +} + +sqlsrv_query($conn, "DROP TABLE $tbname"); +sqlsrv_close($conn); + +echo "Done\n"; +?> +--EXPECT-- +Done \ No newline at end of file From e2103712e2f90523c63bdb157fa5711d5a273a10 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Mon, 22 Jun 2020 14:37:00 -0700 Subject: [PATCH 2/2] Added comment to explain the change --- source/shared/core_stmt.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index bbf06bfce..5ddf73876 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -2368,6 +2368,10 @@ void finalize_output_parameters( _Inout_ sqlsrv_stmt* stmt, _In_opt_ bool except { if (Z_ISUNDEF(stmt->output_params)) return; + + // If an error occurs or an exception is thrown during an execution, the values of any output + // parameters or columns are undefined. Therefore, do not depend on them having any specific + // values, because the ODBC driver may or may not have modified them. if (exception_thrown) { zend_hash_clean(Z_ARRVAL(stmt->output_params)); return;