From 84b6c6f99425395d6a3d54842fa47534a65a8ee0 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Mon, 10 Dec 2018 16:13:12 -0800 Subject: [PATCH 1/4] Initialize the allocated extra space for output buffer to nulls --- source/shared/core_stmt.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index 3b263fe22..9d8fd1588 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -2770,10 +2770,11 @@ void resize_output_buffer_if_necessary( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* { SQLSRV_ASSERT( column_size != SQLSRV_UNKNOWN_SIZE, "column size should be set to a known value." ); buffer_len = Z_STRLEN_P( param_z ); + SQLLEN original_len = buffer_len; SQLLEN expected_len; SQLLEN buffer_null_extra; SQLLEN elem_size; - SQLLEN without_null_len; + // SQLLEN without_null_len; // calculate the size of each 'element' represented by column_size. WCHAR is of course 2, // as is a n(var)char/ntext field being returned as a binary field. @@ -2802,7 +2803,7 @@ void resize_output_buffer_if_necessary( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* buffer_null_extra = (c_type == SQL_C_BINARY) ? elem_size : 0; // this is the size of the string for Zend and for the StrLen parameter to SQLBindParameter - without_null_len = field_size * elem_size; + // without_null_len = field_size * elem_size; // increment to include the null terminator since the Zend length doesn't include the null terminator buffer_len += elem_size; @@ -2821,8 +2822,10 @@ void resize_output_buffer_if_necessary( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* // A zval string len doesn't include the null. This calculates the length it should be // regardless of whether the ODBC type contains the NULL or not. - // null terminate the string to avoid a warning in debug PHP builds - ZSTR_VAL(param_z_string)[without_null_len] = '\0'; + // set the newly allocated space to nulls + char *p = ZSTR_VAL(param_z_string); + p = p + original_len; + memset(p, '\0', expected_len - original_len); ZVAL_NEW_STR(param_z, param_z_string); // buffer_len is the length passed to SQLBindParameter. It must contain the space for NULL in the From b33cae451bb124c6f49b61aad8597cc8d70dec26 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Tue, 11 Dec 2018 11:17:53 -0800 Subject: [PATCH 2/4] Added tests to verify that output param buffers are initialized --- source/shared/core_stmt.cpp | 6 +- .../pdo_900_output_param_memory_data.phpt | 84 +++++++++++++++++++ .../sqlsrv_900_output_param_memory_data.phpt | 81 ++++++++++++++++++ 3 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt create mode 100644 test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index 9d8fd1588..ffeb3def6 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -2774,7 +2774,6 @@ void resize_output_buffer_if_necessary( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* SQLLEN expected_len; SQLLEN buffer_null_extra; SQLLEN elem_size; - // SQLLEN without_null_len; // calculate the size of each 'element' represented by column_size. WCHAR is of course 2, // as is a n(var)char/ntext field being returned as a binary field. @@ -2802,9 +2801,6 @@ void resize_output_buffer_if_necessary( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* // binary fields aren't null terminated, so we need to account for that in our buffer length calcuations buffer_null_extra = (c_type == SQL_C_BINARY) ? elem_size : 0; - // this is the size of the string for Zend and for the StrLen parameter to SQLBindParameter - // without_null_len = field_size * elem_size; - // increment to include the null terminator since the Zend length doesn't include the null terminator buffer_len += elem_size; @@ -2822,7 +2818,7 @@ void resize_output_buffer_if_necessary( _Inout_ sqlsrv_stmt* stmt, _Inout_ zval* // A zval string len doesn't include the null. This calculates the length it should be // regardless of whether the ODBC type contains the NULL or not. - // set the newly allocated space to nulls + // initialize the newly allocated space char *p = ZSTR_VAL(param_z_string); p = p + original_len; memset(p, '\0', expected_len - original_len); diff --git a/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt b/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt new file mode 100644 index 000000000..67eb7fc29 --- /dev/null +++ b/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt @@ -0,0 +1,84 @@ +--TEST-- +GitHub issue 900 - output parameter displays data from memory when not finalized +--DESCRIPTION-- +This test verifies that when there is an active resultset and output parameter not finalized, it should not show any data from client memory. This test does not work with AlwaysEncrypted because +the output param is not assigned in the stored procedure. +--ENV-- +PHPT_EXEC=true +--SKIPIF-- + +--FILE-- +prepare("$storedProcName @OUTPUT = :output"); + // $stmt->bindParam('output', $output, PDO::PARAM_STR, $size); + if ($inout) { + $paramType = PDO::PARAM_STR | PDO::PARAM_INPUT_OUTPUT; + } else { + $paramType = PDO::PARAM_STR; + } + $stmt->bindParam('output', $output, $paramType, $size); + + $stmt->execute(); + + // the output param should be doubled in size for wide characters + // however, it should not contain any data so after trimming it + // should be merely an empty string + $len = strlen($output); + $result = trim($output); + + if ($len != ($size * 2) || $result !== "" ) { + echo "Unexpected output param for $dataType: "; + var_dump($output); + } + + $stmt->closeCursor(); + if (!is_null($output)) { + echo "Output param should be null when finalized!"; + } + unset($stmt); + } catch (PdoException $e) { + echo $e->getMessage() . PHP_EOL; + } +} + +try { + // This helper method sets PDO::ATTR_ERRMODE to PDO::ERRMODE_EXCEPTION + // $conn = connect(); + $conn = new PDO( "sqlsrv:server=$server; Database = $databaseName", $uid, $pwd); + $conn->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION ); + + $dataTypes = array("VARCHAR(512)", "VARCHAR(max)", "NVARCHAR(512)", "NVARCHAR(max)"); + for ($i = 0, $p = 3; $i < count($dataTypes); $i++, $p++) { + // Create the stored procedure first + $storedProcName = "spNullOutputParam" . $i; + $procArgs = "@OUTPUT $dataTypes[$i] OUTPUT"; + $procCode = "SELECT 1, 2, 3"; + + createProc($conn, $storedProcName, $procArgs, $procCode); + getOutputParam($conn, $storedProcName, $dataTypes[$i], false); + getOutputParam($conn, $storedProcName, $dataTypes[$i], true); + + // Drop the stored procedure + dropProc($conn, $storedProcName); + } + + echo "Done\n"; + + unset($conn); +} catch (PdoException $e) { + echo $e->getMessage() . PHP_EOL; +} +?> +--EXPECT-- +Done diff --git a/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt b/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt new file mode 100644 index 000000000..273ade1d1 --- /dev/null +++ b/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt @@ -0,0 +1,81 @@ +--TEST-- +GitHub issue 900 - output parameter displays data from memory when not finalized +--DESCRIPTION-- +This test verifies that when there is an active resultset and output parameter not finalized, it should not show any data from client memory. This test does not work with AlwaysEncrypted because +the output param is not assigned in the stored procedure. +--ENV-- +PHPT_EXEC=true +--SKIPIF-- + +--FILE-- + "UTF-8")); +if (!$conn) { + fatalError("Could not connect.\n"); +} + +$dataTypes = array("VARCHAR(512)", "VARCHAR(max)", "NVARCHAR(512)", "NVARCHAR(max)"); +for ($i = 0, $p = 3; $i < count($dataTypes); $i++, $p++) { + // Create the stored procedure first + $storedProcName = "spNullOutputParam" . $i; + $procArgs = "@OUTPUT $dataTypes[$i] OUTPUT"; + $procCode = "SELECT 1, 2, 3"; + + createProc($conn, $storedProcName, $procArgs, $procCode); + getOutputParam($conn, $storedProcName, false, ($i < 2), ($i % 2 != 0)); + getOutputParam($conn, $storedProcName, true, ($i < 2), ($i % 2 != 0)); + + // Drop the stored procedure + dropProc($conn, $storedProcName); +} + +echo "Done\n"; + +sqlsrv_close($conn); + +?> +--EXPECT-- +Done From 3ed709e9ba7968dae3a6762fa9657bfca81c0c36 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Tue, 11 Dec 2018 11:56:27 -0800 Subject: [PATCH 3/4] Added include file --- test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt | 1 + 1 file changed, 1 insertion(+) diff --git a/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt b/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt index 67eb7fc29..47f3512fc 100644 --- a/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt +++ b/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt @@ -10,6 +10,7 @@ PHPT_EXEC=true --FILE-- Date: Fri, 14 Dec 2018 09:03:46 -0800 Subject: [PATCH 4/4] Redesigned the output param tests --- .../pdo_900_output_param_memory_data.phpt | 17 +++++-------- .../sqlsrv_900_output_param_memory_data.phpt | 24 ++++++++----------- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt b/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt index 47f3512fc..b1683ec91 100644 --- a/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt +++ b/test/functional/pdo_sqlsrv/pdo_900_output_param_memory_data.phpt @@ -1,8 +1,7 @@ --TEST-- GitHub issue 900 - output parameter displays data from memory when not finalized --DESCRIPTION-- -This test verifies that when there is an active resultset and output parameter not finalized, it should not show any data from client memory. This test does not work with AlwaysEncrypted because -the output param is not assigned in the stored procedure. +This test verifies that when there is an active resultset and output parameter not finalized, it should not show any data from client memory. This test does not work with AlwaysEncrypted because the output param is not assigned in the stored procedure. --ENV-- PHPT_EXEC=true --SKIPIF-- @@ -12,17 +11,13 @@ PHPT_EXEC=true require_once("MsSetup.inc"); require_once("MsCommon_mid-refactor.inc"); -$size = 30; - function getOutputParam($conn, $storedProcName, $dataType, $inout) { - global $size; + $size = rand(1000, 4000); // The maximum anticipated size is 8000 for wide chars try { $output = null; - $stmt = $conn->prepare("$storedProcName @OUTPUT = :output"); - // $stmt->bindParam('output', $output, PDO::PARAM_STR, $size); if ($inout) { $paramType = PDO::PARAM_STR | PDO::PARAM_INPUT_OUTPUT; } else { @@ -32,9 +27,9 @@ function getOutputParam($conn, $storedProcName, $dataType, $inout) $stmt->execute(); - // the output param should be doubled in size for wide characters - // however, it should not contain any data so after trimming it - // should be merely an empty string + // The output param should be doubled in size for wide characters. + // However, it should not contain any data so after trimming it + // should be merely an empty string because it was originally set to null $len = strlen($output); $result = trim($output); @@ -59,7 +54,7 @@ try { $conn = new PDO( "sqlsrv:server=$server; Database = $databaseName", $uid, $pwd); $conn->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION ); - $dataTypes = array("VARCHAR(512)", "VARCHAR(max)", "NVARCHAR(512)", "NVARCHAR(max)"); + $dataTypes = array("VARCHAR(256)", "VARCHAR(512)", "VARCHAR(max)", "NVARCHAR(256)", "NVARCHAR(512)", "NVARCHAR(max)"); for ($i = 0, $p = 3; $i < count($dataTypes); $i++, $p++) { // Create the stored procedure first $storedProcName = "spNullOutputParam" . $i; diff --git a/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt b/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt index 273ade1d1..ee195951f 100644 --- a/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt +++ b/test/functional/sqlsrv/sqlsrv_900_output_param_memory_data.phpt @@ -1,8 +1,7 @@ --TEST-- GitHub issue 900 - output parameter displays data from memory when not finalized --DESCRIPTION-- -This test verifies that when there is an active resultset and output parameter not finalized, it should not show any data from client memory. This test does not work with AlwaysEncrypted because -the output param is not assigned in the stored procedure. +This test verifies that when there is an active resultset and output parameter not finalized, it should not show any data from client memory. This test does not work with AlwaysEncrypted because the output param is not assigned in the stored procedure. --ENV-- PHPT_EXEC=true --SKIPIF-- @@ -11,16 +10,14 @@ PHPT_EXEC=true