From c08813d4c7ab356a4db818e2da929803ab9707d4 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Fri, 3 May 2019 16:03:08 -0700 Subject: [PATCH 1/6] Fixed memory issues with data classification --- source/shared/core_sqlsrv.h | 2 +- source/shared/core_stmt.cpp | 6 +++--- source/shared/core_util.cpp | 23 ++++++++++++++++------- source/sqlsrv/stmt.cpp | 2 +- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/source/shared/core_sqlsrv.h b/source/shared/core_sqlsrv.h index e573f953d..b7a8c3921 100644 --- a/source/shared/core_sqlsrv.h +++ b/source/shared/core_sqlsrv.h @@ -1436,7 +1436,7 @@ namespace data_classification { struct sensitivity_metadata; void name_id_pair_free(name_id_pair * pair); - void parse_sensitivity_name_id_pairs(_Inout_ sqlsrv_stmt* stmt, _Inout_ USHORT& numpairs, _Inout_ std::vector>& pairs, _Inout_ unsigned char **pptr TSRMLS_CC); + void parse_sensitivity_name_id_pairs(_Inout_ sqlsrv_stmt* stmt, _Inout_ USHORT& numpairs, _Inout_ std::vector>* pairs, _Inout_ unsigned char **pptr TSRMLS_CC); void parse_column_sensitivity_props(_Inout_ sensitivity_metadata* meta, _Inout_ unsigned char **pptr); USHORT fill_column_sensitivity_array(_Inout_ sqlsrv_stmt* stmt, _In_ SQLSMALLINT colno, _Inout_ zval *column_data TSRMLS_CC); diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index f3d3e6fb5..89f65341d 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -968,7 +968,7 @@ field_meta_data* core_sqlsrv_field_metadata( _Inout_ sqlsrv_stmt* stmt, _In_ SQL } } - // Set the field name lenth + // Set the field name length meta_data->field_name_len = static_cast( field_name_len ); field_meta_data* result_field_meta_data = meta_data; @@ -1052,8 +1052,8 @@ void core_sqlsrv_sensitivity_metadata( _Inout_ sqlsrv_stmt* stmt TSRMLS_DC ) sensitivity_meta = new (sqlsrv_malloc(sizeof(sensitivity_metadata))) sensitivity_metadata(); // Parse the name id pairs for labels first then info types - parse_sensitivity_name_id_pairs(stmt, sensitivity_meta->num_labels, sensitivity_meta->labels, &dcptr); - parse_sensitivity_name_id_pairs(stmt, sensitivity_meta->num_infotypes, sensitivity_meta->infotypes, &dcptr); + parse_sensitivity_name_id_pairs(stmt, sensitivity_meta->num_labels, &sensitivity_meta->labels, &dcptr); + parse_sensitivity_name_id_pairs(stmt, sensitivity_meta->num_infotypes, &sensitivity_meta->infotypes, &dcptr); // Next parse the sensitivity properties parse_column_sensitivity_props(sensitivity_meta, &dcptr); diff --git a/source/shared/core_util.cpp b/source/shared/core_util.cpp index 5db20c3fc..234184b0b 100644 --- a/source/shared/core_util.cpp +++ b/source/shared/core_util.cpp @@ -426,12 +426,18 @@ namespace data_classification { void convert_sensivity_field(_Inout_ sqlsrv_stmt* stmt, _In_ SQLSRV_ENCODING encoding, _In_ unsigned char *ptr, _In_ int len, _Inout_updates_bytes_(cchOutLen) char** field_name) { sqlsrv_malloc_auto_ptr temp_field_name; - int temp_field_len = len * 2; + int temp_field_len = len * sizeof(SQLWCHAR); SQLLEN field_name_len = 0; + if (len == 0) { + *field_name = reinterpret_cast(sqlsrv_malloc(1)); + *field_name[0] = '\0'; + return; + } + temp_field_name = static_cast(sqlsrv_malloc((len + 1) * sizeof(SQLWCHAR))); + memset(temp_field_name, L'\0', len + 1); memcpy_s(temp_field_name, temp_field_len, ptr, temp_field_len); - temp_field_name[temp_field_len] = '\0'; bool converted = convert_string_from_utf16(encoding, temp_field_name, len, field_name, field_name_len); @@ -450,14 +456,16 @@ namespace data_classification { } sqlsrv_free(pair); } - - void parse_sensitivity_name_id_pairs(_Inout_ sqlsrv_stmt* stmt, _Inout_ USHORT& numpairs, _Inout_ std::vector>& pairs, _Inout_ unsigned char **pptr) + + void parse_sensitivity_name_id_pairs(_Inout_ sqlsrv_stmt* stmt, _Inout_ USHORT& numpairs, _Inout_ std::vector>* pairs, _Inout_ unsigned char **pptr) { unsigned char *ptr = *pptr; unsigned short npairs; - numpairs = npairs = *(unsigned short*)ptr; + numpairs = npairs = *(reinterpret_cast(ptr)); SQLSRV_ENCODING encoding = ((stmt->encoding() == SQLSRV_ENCODING_DEFAULT ) ? stmt->conn->encoding() : stmt->encoding()); + pairs->reserve(numpairs); + ptr += sizeof(unsigned short); while (npairs--) { int namelen, idlen; @@ -485,8 +493,9 @@ namespace data_classification { convert_sensivity_field(stmt, encoding, idptr, idlen, (char**)&id); pair->id = id; - pairs.push_back(pair.get()); + name_id_pair* a_pair = pair; pair.transferred(); + pairs->push_back(a_pair); } *pptr = ptr; } @@ -537,7 +546,7 @@ namespace data_classification { if (meta == NULL) { return 0; } - + SQLSRV_ASSERT(colno >= 0 && colno < meta->num_columns, "fill_column_sensitivity_array: column number out of bounds"); zval data_classification; diff --git a/source/sqlsrv/stmt.cpp b/source/sqlsrv/stmt.cpp index b6f56b6e3..da2d3c945 100644 --- a/source/sqlsrv/stmt.cpp +++ b/source/sqlsrv/stmt.cpp @@ -538,7 +538,7 @@ PHP_FUNCTION( sqlsrv_field_metadata ) TSRMLS_CC ); if (stmt->data_classification) { - data_classification::fill_column_sensitivity_array(stmt, f, &field_array); + data_classification::fill_column_sensitivity_array(stmt, f, &field_array TSRMLS_CC); } // add this field's meta data to the result set meta data From ced1e0c11c325042864d0a563e37f479b18c4660 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Mon, 6 May 2019 08:37:33 -0700 Subject: [PATCH 2/6] Applied review --- source/shared/core_util.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/shared/core_util.cpp b/source/shared/core_util.cpp index 234184b0b..7b12e0716 100644 --- a/source/shared/core_util.cpp +++ b/source/shared/core_util.cpp @@ -493,9 +493,8 @@ namespace data_classification { convert_sensivity_field(stmt, encoding, idptr, idlen, (char**)&id); pair->id = id; - name_id_pair* a_pair = pair; + pairs.push_back(pair.get()); pair.transferred(); - pairs->push_back(a_pair); } *pptr = ptr; } From 13183850589eef10ef8b62d59cb8bac1d7e1b47e Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Mon, 6 May 2019 09:00:37 -0700 Subject: [PATCH 3/6] Fixed typo --- source/shared/core_util.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/shared/core_util.cpp b/source/shared/core_util.cpp index 7b12e0716..8fea03358 100644 --- a/source/shared/core_util.cpp +++ b/source/shared/core_util.cpp @@ -493,7 +493,7 @@ namespace data_classification { convert_sensivity_field(stmt, encoding, idptr, idlen, (char**)&id); pair->id = id; - pairs.push_back(pair.get()); + pairs->push_back(pair.get()); pair.transferred(); } *pptr = ptr; From 719786126c585a65108d863cdc5b7cf15ffaf265 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Tue, 7 May 2019 14:35:42 -0700 Subject: [PATCH 4/6] Used reset to free the resources properly --- source/shared/core_stmt.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/shared/core_stmt.cpp b/source/shared/core_stmt.cpp index 89f65341d..a2fd1501c 100644 --- a/source/shared/core_stmt.cpp +++ b/source/shared/core_stmt.cpp @@ -262,8 +262,7 @@ void sqlsrv_stmt::clean_up_sensitivity_metadata() { if (current_sensitivity_metadata) { current_sensitivity_metadata->~sensitivity_metadata(); - sqlsrv_free(current_sensitivity_metadata); - current_sensitivity_metadata = NULL; + current_sensitivity_metadata.reset(); } } From 901c45d3f56b611bc856c95b0ccb09f62d738502 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Tue, 7 May 2019 20:38:25 -0700 Subject: [PATCH 5/6] Added tests with batch queries and used json --- buildscripts/buildtools.py | 2 +- .../pdo_sqlsrv/pdo_data_classification.phpt | 49 +++++++++++++++++- .../sqlsrv/sqlsrv_data_classification.phpt | 50 +++++++++++++++++-- 3 files changed, 95 insertions(+), 6 deletions(-) diff --git a/buildscripts/buildtools.py b/buildscripts/buildtools.py index 59f8b8f55..f7b827ee3 100644 --- a/buildscripts/buildtools.py +++ b/buildscripts/buildtools.py @@ -270,7 +270,7 @@ def update_driver_source(self, source_dir, driver): def generate_build_options(self): """Return the generated build configuration and arguments""" - cmd_line = '' + cmd_line = '--enable-json' if self.debug_enabled: cmd_line = ' --enable-debug ' diff --git a/test/functional/pdo_sqlsrv/pdo_data_classification.phpt b/test/functional/pdo_sqlsrv/pdo_data_classification.phpt index 1b29c6049..fd37e07f5 100644 --- a/test/functional/pdo_sqlsrv/pdo_data_classification.phpt +++ b/test/functional/pdo_sqlsrv/pdo_data_classification.phpt @@ -11,6 +11,8 @@ PHPT_EXEC=true require_once('MsSetup.inc'); require_once('MsCommon_mid-refactor.inc'); +$dataClassKey = 'Data Classification'; + function testConnAttrCases() { // Attribute PDO::SQLSRV_ATTR_DATA_CLASSIFICATION is limited to statement level only @@ -157,8 +159,10 @@ function verifyClassInfo($input, $actual) function compareDataClassification($stmt1, $stmt2, $classData) { + global $dataClassKey; + $numCol = $stmt1->columnCount(); - $noClassInfo = array('Data Classification' => array()); + $noClassInfo = array($dataClassKey => array()); for ($i = 0; $i < $numCol; $i++) { $metadata1 = $stmt1->getColumnMeta($i); @@ -176,7 +180,7 @@ function compareDataClassification($stmt1, $stmt2, $classData) } } else { // Verify the classification metadata - if (!verifyClassInfo($classData[$i], $value['Data Classification'])) { + if (!verifyClassInfo($classData[$i], $value[$dataClassKey])) { var_dump($value); } } @@ -190,6 +194,45 @@ function compareDataClassification($stmt1, $stmt2, $classData) } } +function runBatchQuery($conn, $tableName) +{ + global $dataClassKey; + + $options = array(PDO::SQLSRV_ATTR_DATA_CLASSIFICATION => true); + $tsql = "SELECT SSN, BirthDate FROM $tableName"; + + // Run a batch query + $batchQuery = $tsql . ';' . $tsql; + $stmt = $conn->prepare($batchQuery, $options); + $stmt->execute(); + + $numCol = $stmt->columnCount(); + + // The metadata returned should be the same + $c = rand(0, $numCol - 1); + $metadata1 = $stmt->getColumnMeta($c); + $stmt->nextRowset(); + $metadata2 = $stmt->getColumnMeta($c); + + // Check the returned flags + $data1 = $metadata1['flags']; + $data2 = $metadata2['flags']; + + if (!array_key_exists($dataClassKey, $data1) || !array_key_exists($dataClassKey, $data2)) { + echo "Metadata returned with no classification data\n"; + var_dump($data1); + var_dump($data2); + } else { + $jstr1 = json_encode($data1[$dataClassKey]); + $jstr2 = json_encode($data2[$dataClassKey]); + if ($jstr1 !== $jstr2) { + echo "The JSON encoded strings should be identical\n"; + var_dump($jstr1); + var_dump($jstr2); + } + } +} + /////////////////////////////////////////////////////////////////////////////////////// try { testConnAttrCases(); @@ -254,6 +297,8 @@ try { unset($stmt1); unset($stmt2); + + runBatchQuery($conn, $tableName); } dropTable($conn, $tableName); diff --git a/test/functional/sqlsrv/sqlsrv_data_classification.phpt b/test/functional/sqlsrv/sqlsrv_data_classification.phpt index 4ea83cbb3..2d6cce420 100644 --- a/test/functional/sqlsrv/sqlsrv_data_classification.phpt +++ b/test/functional/sqlsrv/sqlsrv_data_classification.phpt @@ -7,7 +7,9 @@ PHPT_EXEC=true --SKIPIF-- --FILE-- - array()); + $noClassInfo = array($dataClassKey => array()); for ($i = 0; $i < $numCol; $i++) { $diff = array_diff_assoc($metadata2[$i], $metadata1[$i]); @@ -164,13 +168,51 @@ function compareDataClassification($stmt1, $stmt2, $classData) } } else { // Verify the classification metadata - if (!verifyClassInfo($classData[$i], $diff['Data Classification'])) { + if (!verifyClassInfo($classData[$i], $diff[$dataClassKey])) { var_dump($diff); } } } } +function runBatchQuery($conn, $tableName) +{ + global $dataClassKey; + + $options = array('DataClassification' => true); + $tsql = "SELECT SSN, BirthDate FROM $tableName"; + $batchQuery = $tsql . ';' . $tsql; + + $stmt = sqlsrv_query($conn, $batchQuery, array(), $options); + if (!$stmt) { + fatalError("Error when calling sqlsrv_query '$tsql'.\n"); + } + + $numCol = sqlsrv_num_fields($stmt); + $c = rand(0, $numCol - 1); + + $metadata1 = sqlsrv_field_metadata($stmt); + if (!$metadata1 || !array_key_exists($dataClassKey, $metadata1[$c])) { + fatalError("runBatchQuery(1): failed to get metadata"); + } + $result = sqlsrv_next_result($stmt); + if (is_null($result) || !$result) { + fatalError("runBatchQuery: failed to get next result"); + } + $metadata2 = sqlsrv_field_metadata($stmt); + if (!$metadata2 || !array_key_exists($dataClassKey, $metadata2[$c])) { + fatalError("runBatchQuery(2): failed to get metadata"); + } + + $jstr1 = json_encode($metadata1[$c][$dataClassKey]); + $jstr2 = json_encode($metadata2[$c][$dataClassKey]); + if ($jstr1 !== $jstr2) { + echo "The JSON encoded strings should be identical\n"; + var_dump($jstr1); + var_dump($jstr2); + } +} + /////////////////////////////////////////////////////////////////////////////////////// require_once('MsCommon.inc'); @@ -248,6 +290,8 @@ if ($isSupported) { compareDataClassification($stmt, $stmt2, $classData); sqlsrv_free_stmt($stmt2); + + runBatchQuery($conn, $tableName); } sqlsrv_free_stmt($stmt); From 80d90b764212da2f0c609eccd81a6fc0625ce045 Mon Sep 17 00:00:00 2001 From: Jenny Tam Date: Wed, 8 May 2019 09:49:38 -0700 Subject: [PATCH 6/6] Moved the order --- buildscripts/buildtools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/buildscripts/buildtools.py b/buildscripts/buildtools.py index f7b827ee3..c1f9df4cd 100644 --- a/buildscripts/buildtools.py +++ b/buildscripts/buildtools.py @@ -270,7 +270,7 @@ def update_driver_source(self, source_dir, driver): def generate_build_options(self): """Return the generated build configuration and arguments""" - cmd_line = '--enable-json' + cmd_line = '' if self.debug_enabled: cmd_line = ' --enable-debug ' @@ -282,7 +282,7 @@ def generate_build_options(self): else: # pdo_sqlsrv cmd_line = ' --enable-pdo --with-pdo-sqlsrv=shared ' + cmd_line - cmd_line = 'cscript configure.js --disable-all --enable-cli --enable-cgi --enable-embed' + cmd_line + cmd_line = 'cscript configure.js --disable-all --enable-cli --enable-cgi --enable-json --enable-embed' + cmd_line if self.thread == 'nts': cmd_line = cmd_line + ' --disable-zts' return cmd_line