📜 ⬆️ ⬇️

We are looking for errors in the source code of the Amazon Web Services SDK for .NET

Picture 1


Greetings to all fans to criticize someone else's code. :) Today in our laboratory a new study material is the source code of the AWS SDK project for .NET. At one time we wrote an article about checking the AWS SDK for C ++. Then there was nothing particularly interesting. Let's see how we enjoy the .NET version of the AWS SDK. A good opportunity to once again demonstrate the capabilities of the PVS-Studio analyzer, as well as to make the world a little bit more perfect.

The Amazon Web Services (AWS) SDK for .NET is a developer toolkit for building .NET-based applications in the AWS infrastructure and making it much easier to write code. The SDK includes .NET API sets for various AWS services, such as Amazon S3, Amazon EC2, DynamoDB, and others. The source code for the SDK is hosted on GitHub .

As I said, we wrote an article about testing the AWS SDK for C ++. The article turned out to be small - only a couple of errors were found for 512 thousand lines of code. This time we are dealing with a much larger amount of code, which includes about 34 thousand cs-files, and the total number of lines of code (excluding empty ones) is an impressive 5 million. A small part of the code (200 thousand lines in 664 cs-files) falls on tests, I did not consider them.

If the quality of the .NET SDK version is approximately the same as that of C ++ (two errors per 512 KLOC), then we should get about 10 times more errors. This, of course, is a very inaccurate method of calculation, which does not take into account language features and many other factors, but I do not think that the reader now wants to delve into boring arguments. Instead, I suggest going straight to the results.

The check was performed using PVS-Studio version 6.27. Incredibly, the analyzer was able to detect about 40 errors in the AWS SDK code for .NET, which would be worth telling. This indicates not only the high quality of the SDK code (approximately 4 errors per 512 KLOC), but also the comparable quality of the C # performance of the PVS-Studio analyzer compared to C ++. Excellent result!

The authors of the AWS SDK for .NET are great fellows. From project to project, they demonstrate amazing code quality. This can serve as a good example for other teams. But, of course, I would not be a developer of a static analyzer if I had not inserted my 5 kopecks. :) We are already interacting with the Lumberyard team from Amazon on the use of PVS-Studio. But since this is a very large company with a lot of divisions around the world, it is very likely that the AWS SDK team for .NET has never heard of PVS-Studio at all. In any case, in the SDK code I did not find any signs of using our analyzer, although this does not mean anything. However, the command, at a minimum, uses the analyzer built into Visual Studio. This is good, but the code checks can be strengthened :).

As a result, I still managed to find a few errors in the SDK code and, finally, it was 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 the reassignment of the value of the same variable. From the code it becomes clear that this is due to an error that violates the logic of operation: the value of the variable this.linker.s3.region will always be equal to value , regardless of the condition if (String.IsNullOrEmpty (value)) . In the body of the if block, return was omitted The code needs to be corrected 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; } }

The classic example of a typo that causes an infinite recursion in the get access method of the OnFailure property. Instead of returning the value of the private onFailure field , access the OnFailure property. Corrected version:

 public OnFailure? OnFailure { get { return this.onFailure; } set { this.onFailure = value; } }

You ask: “How did it work?” So far - no way. The property is not used anywhere, but this is a temporary phenomenon. At one point, someone will start using it and will definitely get an unexpected result. To prevent such typos, it is recommended not to use identifiers that differ only in the case of the first letter.

Another note to this construct is the use of an identifier that fully matches the name of the OnFailure type. From the point of view of the compiler, this is quite acceptable, but makes it difficult for a person to read the code.

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 that described above. Only here infinite recursion will occur when accessing the SSES3 property for both reading and writing. Corrected version:

 public SSES3 SSES3 { get { return this.sSES3; } set { this.sSES3 = value; } }

Attention Test

The following is a task from a programmer who is passionate about using the Copy-Paste technique. Look at how the code looks in the Visual Studio editor, and try to find the error.

Picture 3


PVS-Studio warning : V3029 The terms expressions of the "if" are expressed alongside each other are identical. Check lines: 91, 95. AWSSDK.AppSync.Net45 CreateApiKeyResponseUnmarshaller.cs 91

I reduced the body of the UnmarshallException method, removing all unnecessary. Now it is clear that identical checks follow one after the 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 may seem that the error is not gross - just an extra check. But often such a pattern may indicate more serious problems in the code when some necessary verification is not performed.

There are some more similar errors in the code.

PVS-Studio warnings:


What are you?

PVS-Studio Warning: V3062 An object 'attributeName' is used. 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 detected an error in the GetAttribute method: the string is checked that it contains itself. From the description of the method it follows that if the attribute name ( attributeName key) is found (in the _attributes dictionary), then the attribute value should be returned, otherwise null . In fact, since the condition attributeName.Contains (attributeName) is always true, an attempt is made to return a value using a key that may not be found in the dictionary. Then, instead of returning null, KeyNotFoundException will be thrown.

Let's try to fix this code. To better understand 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 _attributes dictionary. Let's go back to the GetAttribute method and correct 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.

And one more small note to this code snippet. I noticed that authors use lock when working with the _attributes dictionary. It is clear that this is necessary for multi-threaded access, but the lock design is rather slow and cumbersome. Instead of a Dictionary in this case, it might be more convenient to use a thread-safe version of the dictionary - ConcurrentDictionary . Then the need for lock disappears. Although, maybe I do not know about some features of the project.

Suspicious behavior

PVS-Studio warning : V3063 [CWE-571] A part of the 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 alerted the verification of string.IsNullOrEmpty (inferredIndexName) . Indeed, the string inferredIndexName is assigned null , then the value of this variable does not change anywhere, and then for some reason it is checked for null or an empty string. It looks suspicious. Let's take a closer look at the above code snippet. I deliberately did not reduce it for a better understanding of the situation. So, in the first if statement (and also in the next), the specifiedIndexName variable is checked in some way. Depending on the result of the checks, the variable inferredIndexName gets a new value. Now pay attention to the third if statement . The body of this operator (throwing an exception) will be executed in the case of indexNames.Count> 0 , since the first part of the condition, string.IsNullOrEmpty (inferredIndexName), is always true. Perhaps the variables specifiedIndexName and inferredIndexName are simply confused. Or the third check should be without else , representing a stand-alone 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 an unequivocal answer about the options for correcting this code. But it is definitely necessary for the author to inspect it.

NullReferenceException

PVS-Studio Warning: V3095 [CWE-476] The 'conditionValues' object was used against it. 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) { .... } .... } }

Classics of the genre. The conditionValues variable is used without first checking for null equality. In this case, further in the code, such verification is performed, but it is too late. :)

The code can 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) { .... } .... } }

A few more similar errors were found in the code.

PVS-Studio warnings:


The following warning is very similar in meaning, but the situation is inverse of the one discussed above.

PVS-Studio warning : V3125 [CWE-476] 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 snippets contains a check for the value of the state variable for null equality. Further, the state variable is used to unsubscribe from the PreemptExpiryTime event, the test for null is no longer performed, and a NullReferenceException exception may be thrown . More secure code option:

 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; .... }

There are other similar errors in the code.

PVS-Studio warnings:


No alternative reality

PVS-Studio Warning: V3009 [CWE-393] This is the odd one. 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 this is for the calling code. I tracked the use of the State19 method. It participates in filling the array of fsm_handler_table handlers along with other similar methods (there are 28 of them with names, respectively, from State1 to State28 ). Here it is important to note that in addition to State19 , warnings V3009 [CWE-393] were also issued for some other processors. These are handlers: State23, State26, State27, State28 . Warnings issued by the analyzer for them:


Here is the declaration and initialization of the array of handlers:

 private static StateHandler[] fsm_handler_table; .... private static void PopulateFsmTables () { fsm_handler_table = new StateHandler[28] { State1, State2, .... State19, .... State23, .... State26, State27, State28 };

For completeness, let's look at the code of one of the handlers for which the analyzer had no complaints, for example, State2 :

 private static bool State2 (....) { .... if (....) { return true; } switch (....) { .... default: return false; } }

And this is how callback handlers occur:

 public bool NextToken () { .... while (true) { handler = fsm_handler_table[state - 1]; if (! handler (fsm_context)) // <= throw new JsonException (input_char); .... } .... }

As you can see, the exception will be thrown if false is returned. In our case, for State19, State23, State26, State27, and State28 handlers, this will never happen. It looks suspicious. On the other hand, as many as five handlers have a similar behavior (they always return true ), so perhaps it was so intended and not the result of a typo.

Why did I stop at all this in such detail? This situation is very indicative in the sense that a static analyzer is often only able to indicate a suspicious design. And even a person (not a machine) who does not have sufficient knowledge of the project, having spent time studying the code, is still not able to give an exhaustive answer about the presence of an error. The code must be studied by the developer.

Senseless 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) // <= { .... } .... }

Notice the doLog variable. After initialization to false , then in the code this variable will be set to true in all cases. Thus, the if (doLog) check is always true. Perhaps, earlier in this method there was a branch in which no value was assigned to the doLog variable, then at the time of the test it could contain the value false obtained during initialization. But now there is no such 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 states that the value of the result variable is always true . This is only possible if the PutValueHelper method always returns true . Let's look at this method:

 private bool PutValueHelper(....) { .... if (....)) { return true; } if (record == null) { .... return true; } else { .... return true; } }

Indeed, the method returns true under any condition. Moreover, the analyzer issued a warning for this method. PVS-Studio Warning: V3009 [CWE-393] This is the odd one. SQLiteLocalStorage.cs 1016

I intentionally did not give this warning earlier, when I considered other V3009 errors, and saved it for this case. Thus, the analyzer 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' for the left | 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 // <= )) { .... } .... }

Twice compare the field this.token with the value of the JsonToken.String JsonToken enumeration. Probably one of the comparisons should contain a different enumeration value. If so, then a serious mistake was made.

Refactoring + inattention?

PVS-Studio warning : V3025 [CWE-685] Incorrect format. A different number of formatted 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)); } .... }

Probably, the format string for the string.Format method previously contained the output specifier {0} , for which the AWSConfigs.AWSRegionKey argument was specified. Then the string was changed, the specifier disappeared, but the argument was forgotten to be deleted. The above code snippet works without errors (an exception would be thrown otherwise - a specifier without an argument), but it looks ugly. 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 an 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 fairly common situation is an unsafe invocation of an event handler. Between checking the mOnSyncSuccess variable for equality to null and calling the handler, you can unsubscribe from the event, and its value becomes zero. The probability of such a scenario is small, but it is still better to make the code more secure:

 protected void FireSyncSuccessEvent(List<Record> records) { mOnSyncSuccess?.Invoke(this, new SyncSuccessEventArgs(records)); }

There are other similar errors in the code.

PVS-Studio warnings:


Incomplete class

PVS-Studio warning : V3126 Type 'JsonData' implementing the interface does not override the '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 cite it as a whole, limiting myself only to the declaration. This class does not really contain the overridden method GetHashCode , which is not safe, as it can lead to erroneous behavior when using the JsonData type to work, for example, with collections. Probably, at present there is no problem, but in the future the strategy of using this type may change. This error is described in more detail in the documentation .

Conclusion

These are all the interesting errors that I managed to find in the AWS SDK code for .NET using the PVS-Studio static analyzer. Once again I will emphasize the quality of the project. I found a very small number of errors for 5 million lines of code. Although it is likely that a more thorough analysis of the warnings issued would allow me to add a few more errors to this list. But it is also very likely that I have in vain ranked some of the described warnings as errors. In this case, only the developer who is in the context of the code being verified always makes unambiguous conclusions.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Amazon Web Services SDK source code for .NET

Source: https://habr.com/ru/post/437516/

More articles: