Fixing bugs found during Code Review.

Author: raulgarciamsft

.gitignore

@@ -13,3 +13,6 @@
 /.vs/ql/v15/Browse.VC.db
 /.vs/ProjectSettings.json
 
+/.vs/ql_ICryptoTransform/v15/Browse.VC.opendb
+/.vs/ql_ICryptoTransform/v15/Browse.VC.db
+/.vs/ql_ICryptoTransform/v15/.suo

csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs

@@ -1,9 +1,6 @@
-internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed
+internal class TokenCacheThreadUnsafeICryptoTransformDemo
 {
-    // We are replacing the static SHA256 field with an instance one
-    //
-    //private static SHA256 _sha = SHA256.Create();
-    private SHA256 _sha = SHA256.Create();
+    private static SHA256 _sha = SHA256.Create();
 
     public string ComputeHash(string data)
     {
@@ -21,8 +18,8 @@ static void Main(string[] args)
 
         Action<object> action = (object obj) =>
         {
-            var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed();
-            if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
+            var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo();
+            if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
             {
                 Console.WriteLine("**** We got incorrect Results!!! ****");
             }

csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs

@@ -1,6 +1,9 @@
-internal class TokenCacheThreadUnsafeICryptoTransformDemo
+internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed
 {
-    private static SHA256 _sha = SHA256.Create();
+    // We are replacing the static SHA256 field with an instance one
+    //
+    //private static SHA256 _sha = SHA256.Create();
+    private SHA256 _sha = SHA256.Create();
 
     public string ComputeHash(string data)
     {
@@ -18,8 +21,8 @@ static void Main(string[] args)
 
         Action<object> action = (object obj) =>
         {
-            var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo();
-            if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
+            var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed();
+            if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
             {
                 Console.WriteLine("**** We got incorrect Results!!! ****");
             }

csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp

@@ -4,23 +4,22 @@
 <qhelp>
 <overview>
 <p>Classes that implement <code>System.Security.Cryptography.ICryptoTransform</code> are not thread safe.</p>
-<p>The root cause of the problem is because of the way these class are implemented using Microsoft CAPI/CNG patterns.</p>
-<p>For example, a hash class implementing this interface, typically there would be an instance-specific hash object (i.e. using <code>BCryptCreateHash</code> function), which can be called multiple times to add data to the hash (i.e. <code>BCryptHashData</code>), and finally calling the function that would finish the hash & get back the data (i.e. <code>BCryptFinishHash</code>).</p>
-<p>The implementation would potentially allow the same hash object to be called with data from multiple threads before calling the finish function, thus leading to potentially incorrect results.</p>
-<p>Because of this pattern, you can expect that, if you have multiple threads hashing <code>"abc"</code> on a static hash object, you may occasionally obtain the results (incorrectly) for hashing <code>"abcabc"</code>, or face other unexpected behavior.</p>
-<p>It is very unlikely somebody outside Microsoft would write a class that implements <code>ICryptoTransform</code>, and even if they do, it is likely that they will follow the same common pattern than the existing classes implementing this interface.</p>
+<p>This problem is caused by the way these classes are implemented using Microsoft CAPI/CNG patterns.</p>
+<p>For example, when a hash class implements this interface, there would typically be an instance-specific hash object created (for example using <code>BCryptCreateHash</code> function). This object can be called multiple times to add data to the hash (for example <code>BCryptHashData</code>). Finally, a function is called that finishes the hash and returns the data (for example <code>BCryptFinishHash</code>).</p>
+<p>Allowing the same hash object to be called with data from multiple threads before calling the finish function could potentially lead to incorrect results.</p>
+<p>For example, if you have multiple threads hashing <code>"abc"</code> on a static hash object, you may occasionally obtain the results (incorrectly) for hashing <code>"abcabc"</code>, or face other unexpected behavior.</p>
+<p>It is very unlikely somebody outside Microsoft would write a class that implements <code>ICryptoTransform</code>, and even if they do, it is likely that they will follow the same common pattern as the existing classes implementing this interface.</p>
 <p>Any object that implements <code>System.Security.Cryptography.ICryptoTransform</code> should not be used in concurrent threads as the instance members of such object are also not thread safe.</p>
-<p>Potential problems may not be evident at first, but can range from explicit errors such as exceptions, to incorrect results when sharing an instance of such object in multiple threads.</p>
+<p>Potential problems may not be evident at first, but can range from explicit errors such as exceptions, to incorrect results when sharing an instance of such an object in multiple threads.</p>
 
 </overview>
 <recommendation>
-<p>Verify that the object is not being shared across threads.</p>
-<p>If it is shared accross instances. Consider changing the code to use a non-static object of type <code>System.Security.Cryptography.ICryptoTransform</code> instead.</p>
+<p>If the object is shared across instances, you should consider changing the code to use a non-static object of type <code>System.Security.Cryptography.ICryptoTransform</code> instead.</p>
 <p>As an alternative, you could also look into using <code>ThreadStatic</code> attribute, but make sure you read the initialization remarks on the documentation.</p>
 
 </recommendation>
 <example>
-<p>This example demonstrates the dangers of using a static <code>System.Security.Cryptography.ICryptoTransform</code> in such a way that the results may be incorrect.</p>
+<p>This example demonstrates the dangers of using a static <code>System.Security.Cryptography.ICryptoTransform</code> in a way that generates incorrect results.</p>
 <sample src="ThreadUnSafeICryptoTransformBad.cs" />
 
 <p>A simple fix is to change the <code>_sha</code> field from being a static member to an instance one by removing the <code>static</code> keyword.</p>