Welcome to all fans of trashing someone else's code. :) Today in our laboratory, we have a new material for a research - the source code of the AWS SDK for .NET project. At the time, we wrote an article about checking AWS SDK for C++. Then there was not anything particularly interesting. Let's see what .NET of the AWS SDK version is worth. Once again, it is a great opportunity to demonstrate the abilities of the PVS-Studio analyzer and make the world a bit better.
Amazon Web Services (AWS) SDK for .NET is a set of developer's tools, meant for creating applications based on .NET in the AWS infrastructure. This set enables to significantly simplify the process of code writing. SDK includes sets API .NET for various AWS services, such as Amazon S3, Amazon EC2, DynamoDB and others. SDK source code is available on GitHub.
As I mentioned, at the time we have already written the article about checking AWS SDK for C++. The article turned out to be small - only a couple of errors found per 512 thousands of lines of code. This time we are dealing with a much larger size of the code, which includes about 34 thousand of cs-files, and the total number of lines of code (excluding blank ones) is impressive 5 million. A small part of code (200 thousand lines in 664-cs files) accrues to tests, I haven't considered them.
If the quality of the .NET code of the SDK version is approximately the same as the one of C++ (two errors per 512 KLOC), then we should get about 10 times greater number of errors. Of course, this is a very inaccurate calculation methodology, which doesn't take into account the linguistic peculiarities and many other factors, but I don't think the reader now wants to go into boring reasoning. Instead, I suggest moving on to the results.
The check was performed using PVS-Studio 6.27. It is just incredible, but still the fact is that in the AWS SDK for .NET the analyzer managed to detect 40 errors, which would be worth to talk about. It demonstrates not only a high quality of the SDK code (about 4 errors per 512 KLOC), but also comparable quality of the C# PVS-Studio analyzer in comparison with C++. A great result!
Authors of AWS SDK for .NET, you're real champs! With each project, you demonstrate tremendous quality of the code. It can be a great example for other teams. However, of course, I would not be a developer of a static analyzer, if I didn't give my 2 cents. :) We are already working with a Lumberyard team from Amazon on the use of PVS-Studio. Since it is a very large company with a bunch of units around the world, it is very likely that the AWS SDK team for .NET has never heard of PVS-Studio. Anyway, I have not found any signs of using our analyzer in the SDK code, although it doesn't say anything. However, at least, the team uses the analyzer built into Visual Studio. It is great, but code reviews can always be enhanced :).
As a result, I did manage to find a few bugs in the SDK code and, finally, it's time to share them.
Error in logic
PVS-Studio warning: V3008 [CWE-563] The 'this.linker.s3.region' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 116, 114. AWSSDK.DynamoDBv2.Net45 S3Link.cs 116
public string Region
{
get
{
....
}
set
{
if (String.IsNullOrEmpty(value))
{
this.linker.s3.region = "us-east-1";
}
this.linker.s3.region = value;
}
}The analyzer warns about repeated value assignment to the same variable. From the code it becomes clear that this is due to the error that violates the logic of the program work: the value of the variable this.linker.s3.region will always be equal to the value of the variable value, regardless of the condition if (String.IsNullOrEmpty(value)). return statement was missed in the body of if block. The code needs to be fixed as follows:
public string Region
{
get
{
....
}
set
{
if (String.IsNullOrEmpty(value))
{
this.linker.s3.region = "us-east-1";
return;
}
this.linker.s3.region = value;
}
}Infinite recursion
PVS-Studio warning: V3110 [CWE-674] Possible infinite recursion inside 'OnFailure' property. AWSSDK.ElasticMapReduce.Net45 ResizeJobFlowStep.cs 171
OnFailure? onFailure = null;
public OnFailure? OnFailure
{
get { return this.OnFailure; } // <=
set { this.onFailure = value; }
}A classic example of a typo, which leads to an infinite recursion in the get accessor of the OnFailure property. Instead of returning the value of a private field onFailure, the access to property OnFailure takes place. Correct variant:
public OnFailure? OnFailure
{
get { return this.onFailure; }
set { this.onFailure = value; }
}You may ask: "How did it work?" So far - no how. The property is not used anywhere else, but this is temporary. At one point, someone will start using it and will certainly receive an unexpected result. To prevent such typos it is recommended not to use identifiers that differ only in case of the first letter.
Another comment to this construction is using of the identifier, which completely matches the name of the OnFailure type. From the point of view of the compiler, it is quite acceptable, but this complicates the perception of code for a person.
Another similar error:
PVS-Studio warning: V3110 [CWE-674] Possible infinite recursion inside 'SSES3' property. AWSSDK.S3.Net45 InventoryEncryption.cs 37
private SSES3 sSES3;
public SSES3 SSES3
{
get { return this.SSES3; }
set { this.SSES3 = value; }
}The situation is identical to the described above. However, here infinite recursion will occur when accessing the property SSES3 both for reading and assigning. Correct variant:
public SSES3 SSES3
{
get { return this.sSES3; }
set { this.sSES3 = value; }
}Test on consideration
Now I'd like to cite a task from a developer, taken with using Copy-Paste method. Take a look at the way code looks like in the Visual Studio editor, and try to find an error (click on image to enlarge).
PVS-Studio warning: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 91, 95. AWSSDK.AppSync.Net45 CreateApiKeyResponseUnmarshaller.cs 91
I reduced the body of the method UnmarshallException, having removed everything that is not needed. Now you can see that identical checks follow each other:
public override AmazonServiceException UnmarshallException(....)
{
....
if (errorResponse.Code != null &&
errorResponse.Code.Equals("LimitExceededException"))
{
return new LimitExceededException(errorResponse.Message,
innerException, errorResponse.Type, errorResponse.Code,
errorResponse.RequestId, statusCode);
}
if (errorResponse.Code != null &&
errorResponse.Code.Equals("LimitExceededException"))
{
return new LimitExceededException(errorResponse.Message,
innerException, errorResponse.Type, errorResponse.Code,
errorResponse.RequestId, statusCode);
}
....
}It might seem that the bug is not rough - just an extra checking. Nevertheless, often such a pattern may indicate more serious problems in the code, when a needed check will not be performed.
In the code, there are several similar errors.
PVS-Studio warnings:
What are you?
PVS-Studio warning: V3062 An object 'attributeName' is used as an argument to its own method. Consider checking the first actual argument of the 'Contains' method. AWSSDK.MobileAnalytics.Net45 CustomEvent.cs 261
/// <summary>
/// Dictionary that stores attribute for this event only.
/// </summary>
private Dictionary<string,string> _attributes =
new Dictionary<string,string>();
/// <summary>
/// Gets the attribute.
/// </summary>
/// <param name="attributeName">Attribute name.</param>
/// <returns>The attribute. Return null of attribute doesn't
/// exist.</returns>
public string GetAttribute(string attributeName)
{
if(string.IsNullOrEmpty(attributeName))
{
throw new ArgumentNullException("attributeName");
}
string ret = null;
lock(_lock)
{
if(attributeName.Contains(attributeName)) // <=
ret = _attributes[attributeName];
}
return ret;
}The analyzer has detected an error in the GetAttribute method: a string is checked whether it contains itself. From the description of the method it follows that if the attribute name (attributeName key) is found (in the dictionary _attributes), the attribute value should be returned, otherwise - null. In fact, as the condition attributeName.Contains(attributeName) is always true, an attempt is made to return the value by a key which might not be found in a dictionary. Then, instead of returning null, an exception KeyNotFoundException will be thrown.
Let's try to fix this code. To understand better how to do this, you should look at another method:
/// <summary>
/// Determines whether this instance has attribute the specified
/// attributeName.
/// </summary>
/// <param name="attributeName">Attribute name.</param>
/// <returns>Return true if the event has the attribute, else
/// false.</returns>
public bool HasAttribute(string attributeName)
{
if(string.IsNullOrEmpty(attributeName))
{
throw new ArgumentNullException("attributeName");
}
bool ret = false;
lock(_lock)
{
ret = _attributes.ContainsKey(attributeName);
}
return ret;
}This method checks whether the attribute name (attributeName key) exists in the dictionary _attributes. Let's get back to the GetAttribute method again and fix the error:
public string GetAttribute(string attributeName)
{
if(string.IsNullOrEmpty(attributeName))
{
throw new ArgumentNullException("attributeName");
}
string ret = null;
lock(_lock)
{
if(_attributes.ContainsKey(attributeName))
ret = _attributes[attributeName];
}
return ret;
}Now the method does exactly what is stated in the description.
One more small comment to this fragment of code. I noticed that the authors use lock when working with the _attributes dictionary. It is clear that this is necessary when having a multithreaded access, but the lock construction is rather slow and cumbersome. Instead of a Dictionary, in this case, perhaps, it would be more convenient to use thread-safe version of the dictionary - ConcurrentDictionary. This way, there will be no need in lock. Although, maybe I don't know about the specifics of the project.
Suspicious behavior
PVS-Studio warning: V3063 [CWE-571] A part of conditional expression is always true if it is evaluated: string.IsNullOrEmpty(inferredIndexName). AWSSDK.DynamoDBv2.PCL ContextInternal.cs 802
private static string GetQueryIndexName(....)
{
....
string inferredIndexName = null;
if (string.IsNullOrEmpty(specifiedIndexName) &&
indexNames.Count == 1)
{
inferredIndexName = indexNames[0];
}
else if (indexNames.Contains(specifiedIndexName,
StringComparer.Ordinal))
{
inferredIndexName = specifiedIndexName;
}
else if (string.IsNullOrEmpty(inferredIndexName) && // <=
indexNames.Count > 0)
throw new InvalidOperationException("Local Secondary Index range
key conditions are used but no index could be inferred from
model. Specified index name = " + specifiedIndexName);
....
}The analyzer was concerned about the check string.IsNullOrEmpty(inferredIndexName). Indeed, the string inferredIndexName is assigned null, then the value of this variable isn't changed anywhere, then for some reason it is checked for null or an empty string. Looks suspicious. Let's take a close look at the above code fragment. I deliberately did not reduce it to understand better the situation. So, in the first if statement (and also in the next one) the variable specifiedIndexName is somehow checked. Depending on the results of the checks, the variable inferredIndexName is getting a new value. Now let's look at the third if statement. The body of this statement (throwing of the exception) will be performed in case if indexNames.Count > 0, as the first part of the whole condition, which is string.IsNullOrEmpty(inferredIndexName) is always true. Perhaps, variables specifiedIndexName and inferredIndexName are mixed up or the third check has to be without else, representing a standalone if statement:
if (string.IsNullOrEmpty(specifiedIndexName) &&
indexNames.Count == 1)
{
inferredIndexName = indexNames[0];
}
else if (indexNames.Contains(specifiedIndexName,
StringComparer.Ordinal))
{
inferredIndexName = specifiedIndexName;
}
if (string.IsNullOrEmpty(inferredIndexName) &&
indexNames.Count > 0)
throw new InvalidOperationException(....);In this case, it is difficult to give a definite answer on options to fix this code. Anyway, the author needs to check it out.
NullReferenceException
PVS-Studio warning: V3095 [CWE-476] The 'conditionValues' object was used before it was verified against null. Check lines: 228, 238. AWSSDK.Core.Net45 JsonPolicyWriter.cs 228
private static void writeConditions(....)
{
....
foreach (....)
{
IList<string> conditionValues = keyEntry.Value;
if (conditionValues.Count == 0) // <=
continue;
....
if (conditionValues != null && conditionValues.Count != 0)
{
....
}
....
}
}It's a classic. The variable conditionValues is used without a preliminary check for null. While later in the code this check is performed. The code needs to be corrected as follows:
private static void writeConditions(....)
{
....
foreach (....)
{
IList<string> conditionValues = keyEntry.Value;
if (conditionValues != null && conditionValues.Count == 0)
continue;
....
if (conditionValues != null && conditionValues.Count != 0)
{
....
}
....
}
}I found several similar errors in code.
PVS-Studio warnings:
The following warning is very similar in meaning, but the case is opposite to the one discussed above.
PVS-Studio warning: V3125 [CWE-476] The 'state' object was used after it was verified against null. Check lines: 139, 127. AWSSDK.Core.Net45 RefreshingAWSCredentials.cs 139
private void UpdateToGeneratedCredentials(
CredentialsRefreshState state)
{
string errorMessage;
if (ShouldUpdate)
{
....
if (state == null)
errorMessage = "Unable to generate temporary credentials";
else
....
throw new AmazonClientException(errorMessage);
}
state.Expiration -= PreemptExpiryTime; // <=
....
}One of the code fragments includes checking the value of the state variable for null. In the code below, the variable is used to unsubscribe from the PreemptExpiryTime event, however, a check for null is no longer performed and throwing of the exception NullReferenceException becomes possible. A more secure version of the code:
private void UpdateToGeneratedCredentials(
CredentialsRefreshState state)
{
string errorMessage;
if (ShouldUpdate)
{
....
if (state == null)
errorMessage = "Unable to generate temporary credentials";
else
....
throw new AmazonClientException(errorMessage);
}
if (state != null)
state.Expiration -= PreemptExpiryTime;
....
}In the code, there are other similar errors:
PVS-Studio warnings:
Non-alternate reality
PVS-Studio warning: V3009 [CWE-393] It's odd that this method always returns one and the same value of 'true'. AWSSDK.Core.Net45 Lexer.cs 651
private static bool State19 (....)
{
while (....) {
switch (....) {
case '"':
....
return true;
case '\\':
....
return true;
default:
....
continue;
}
}
return true;
}The method always returns true. Let's see how critical it is for the calling code. I checked out the cases of using the State19 method. It is involved in filling the array of handlers fsm_handler_table equally with other similar methods (there are 28 of them with the names, respectively, starting from State1 to State28). Here it is important to note that, in addition to State19, for some other handlers the warnings V3009 [CWE-393] were issued as well. These are handlers: State23, State26, State27, State28. The warnings, issued by the analyzer for them:
Here is the way the declaration and the array initialization of handlers look like:
private static StateHandler[] fsm_handler_table;
....
private static void PopulateFsmTables ()
{
fsm_handler_table = new StateHandler[28] {
State1,
State2,
....
State19,
....
State23,
....
State26,
State27,
State28
};To complete the picture, let's see code of one of the handlers to which the analyzer haven't had any claims, for example, State2:
private static bool State2 (....)
{
....
if (....) {
return true;
}
switch (....) {
....
default:
return false;
}
}Here's the way how the call of handlers occurs:
public bool NextToken ()
{
....
while (true) {
handler = fsm_handler_table[state - 1];
if (! handler (fsm_context)) // <=
throw new JsonException (input_char);
....
}
....
}As we can see, an exception will be thrown in case of returning false. In our case, for the handlers State19, State23, State26 State27 and State28 this will never happen. Looks suspicious. On the other hand, five handlers have similar behavior (will always return true), so maybe it was so contrived and is not the result of a typo.
Why am I going so deep in all this? This situation is very significant in the sense that the static analyzer often can only indicate a suspicious construction. And even a person (not a machine), who does not have sufficient knowledge about the project, is still not able to give a full answer on the presence of the error, even having spent time learning code. A developer should review this code.
Meaningless checks
PVS-Studio warning: V3022 [CWE-571] Expression 'doLog' is always true. AWSSDK.Core.Net45 StoredProfileAWSCredentials.cs 235
private static bool ValidCredentialsExistInSharedFile(....)
{
....
var doLog = false;
try
{
if (....)
{
return true;
}
else
{
doLog = true;
}
}
catch (InvalidDataException)
{
doLog = true;
}
if (doLog) // <=
{
....
}
....
}Pay attention to the doLog variable. After initialization with the false value, this variable will get the true value in all cases further along the code. Therefore, the check if (doLog) is always true. Perhaps, earlier in the method there was a branch, in which the doLog variable wasn't assigned any value. At the moment of checking it could contain the false value, received when initializing. But now there is no such a branch.
Another similar error:
PVS-Studio warning: V3022 Expression '!result' is always false. AWSSDK.CognitoSync.PCL SQLiteLocalStorage.cs 353
public void PutValue(....)
{
....
bool result = PutValueHelper(....);
if (!result) <=
{
_logger.DebugFormat("{0}",
@"Cognito Sync - SQLiteStorage - Put Value Failed");
}
else
{
UpdateLastModifiedTimestamp(....);
}
....
}The analyzer claims that the value of the result variable is always true. It is possible only in case if the method PutValueHelper will always return true. Take a look at this method:
private bool PutValueHelper(....)
{
....
if (....))
{
return true;
}
if (record == null)
{
....
return true;
}
else
{
....
return true;
}
}Indeed, the method will return true under all conditions. Moreover, the analyzer has issued a warning for this method. PVS-Studio warning: V3009 [CWE-393] It's odd that this method always returns one and the same value of 'true'. SQLiteLocalStorage.cs 1016
I deliberately did not cite this warning earlier when I was inquiring into other bugs V3009 and saved it up for this case. Thus, the tool was right to point out the error V3022 in the calling code.
Copy-Paste. Again
PVS-Studio warning: V3001 There are identical sub-expressions 'this.token == JsonToken.String' to the left and to the right of the '||' operator. AWSSDK.Core.Net45 JsonReader.cs 343
public bool Read()
{
....
if (
(this.token == JsonToken.ObjectEnd ||
this.token == JsonToken.ArrayEnd ||
this.token == JsonToken.String || // <=
this.token == JsonToken.Boolean ||
this.token == JsonToken.Double ||
this.token == JsonToken.Int ||
this.token == JsonToken.UInt ||
this.token == JsonToken.Long ||
this.token == JsonToken.ULong ||
this.token == JsonToken.Null ||
this.token == JsonToken.String // <=
))
{
....
}
....
}The field this.token is compared twice with the value JsonToken.String of the enumeration JsonToken. Probably, one of the comparisons should contain another enumeration value. If so, a serious mistake has been made here.
Refactoring + inattention?
PVS-Studio warning: V3025 [CWE-685] Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: AWSConfigs.AWSRegionKey. AWSSDK.Core.Net45 AWSRegion.cs 116
public InstanceProfileAWSRegion()
{
....
if (region == null)
{
throw new InvalidOperationException(
string.Format(CultureInfo.InvariantCulture,
"EC2 instance metadata was not available or did not contain
region information.",
AWSConfigs.AWSRegionKey));
}
....
}Perhaps, the format string for the string.Format method previously contained the format item {0}, for which the argument AWSConfigs.AWSRegionKey was set. Then the string was changed, the format item was gone, but a developer forgot to remove the argument. The given code example works without errors (the exception was thrown in the opposite case - the format item without the argument), but it looks not nice. The code should be corrected as follows:
if (region == null)
{
throw new InvalidOperationException(
"EC2 instance metadata was not available or did not contain
region information.");
}Unsafe
PVS-Studio warning: V3083 [CWE-367] Unsafe invocation of event 'mOnSyncSuccess', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. AWSSDK.CognitoSync.PCL Dataset.cs 827
protected void FireSyncSuccessEvent(List<Record> records)
{
if (mOnSyncSuccess != null)
{
mOnSyncSuccess(this, new SyncSuccessEventArgs(records));
}
}A common situation of an unsafe call of the event handler. A user is able to unsubscribe between the checking of the variable mOnSyncSuccess for null and calling of a handler, so its value will become null. The likelihood of such a scenario is small, but it is still better to make code more secure:
protected void FireSyncSuccessEvent(List<Record> records)
{
mOnSyncSuccess?.Invoke(this, new SyncSuccessEventArgs(records));
}In the code, there are other similar errors:
PVS-Studio warnings:
Crude class
PVS-Studio warning: V3126 Type 'JsonData' implementing IEquatable<T> interface does not override 'GetHashCode' method. AWSSDK.Core.Net45 JsonData.cs 26
public class JsonData : IJsonWrapper, IEquatable<JsonData>
{
....
}The JsonData class contains quite a lot of code, so I didn't give it in whole, citing just its declaration. This class really does not contain the overridden method GetHashCode, which is unsafe, as it can lead to erroneous behavior when using the JsonData type for working, for example, with collections. Probably, there is no problem at the moment, but in future this type of strategy might change. This error is described in the documentation in more detail.
Conclusion
These are all interesting bugs that I was able to detect in the code of AWS SDK for .NET using the PVS-Studio static analyzer. I'd like to highlight once again the quality of the project. I found a very small number of errors for 5 million lines of code. Although probably more thorough analysis of issued warnings would let me add a few more errors to this list. Nevertheless, it is also quite likely that I added on some of the warnings to errors for nothing. Unambiguous conclusions in this case are always made only by a developer who is in the context of the checked code.