Fixed memory issues with data classification (#985)

microsoft · May 8, 2019 · ffd9849 · ffd9849
1 parent e02db62
commit ffd9849
Show file tree

Hide file tree

Showing 7 changed files with 116 additions and 20 deletions.
diff --git a/buildscripts/buildtools.py b/buildscripts/buildtools.py
@@ -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

diff --git 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<name_id_pair*, sqlsrv_allocator<name_id_pair*>>& pairs, _Inout_ unsigned char **pptr TSRMLS_CC);
+    void parse_sensitivity_name_id_pairs(_Inout_ sqlsrv_stmt* stmt, _Inout_ USHORT& numpairs, _Inout_ std::vector<name_id_pair*, sqlsrv_allocator<name_id_pair*>>* 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
@@ -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();
     }
 }
 
@@ -968,7 +967,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<SQLSMALLINT>( field_name_len );
 
     field_meta_data* result_field_meta_data = meta_data;
@@ -1052,8 +1051,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
@@ -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<SQLWCHAR> 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<char*>(sqlsrv_malloc(1));
+            *field_name[0] = '\0';
+            return;
+        }
+
         temp_field_name = static_cast<SQLWCHAR*>(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<name_id_pair*, sqlsrv_allocator<name_id_pair*>>& pairs, _Inout_ unsigned char **pptr)
+    
+    void parse_sensitivity_name_id_pairs(_Inout_ sqlsrv_stmt* stmt, _Inout_ USHORT& numpairs, _Inout_ std::vector<name_id_pair*, sqlsrv_allocator<name_id_pair*>>* pairs, _Inout_ unsigned char **pptr)
     {
         unsigned char *ptr = *pptr;
         unsigned short npairs;
-        numpairs = npairs = *(unsigned short*)ptr;
+        numpairs = npairs = *(reinterpret_cast<unsigned short*>(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,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;
@@ -537,7 +545,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
@@ -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

diff --git 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
@@ -7,7 +7,9 @@ PHPT_EXEC=true
 --SKIPIF--
 <?php require('skipif_versions_old.inc'); ?>
 --FILE--
-<?php
+<?php
+$dataClassKey = 'Data Classification';
+
 function testErrorCases($conn, $tableName, $isSupported)
 {
     // This function will check two error cases:
@@ -136,6 +138,8 @@ function verifyClassInfo($input, $actual)
 
 function compareDataClassification($stmt1, $stmt2, $classData)
 {
+    global $dataClassKey;
+
     $numCol = sqlsrv_num_fields($stmt1);
 
     $metadata1 = sqlsrv_field_metadata($stmt1);
@@ -152,7 +156,7 @@ function compareDataClassification($stmt1, $stmt2, $classData)
     // empty array. Otherwise, it should contain an array with one set of
     // Label (name, id) and Information Type (name, id)
 
-    $noClassInfo = array('Data Classification' => 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);