Today we are dissecting AWS SDK for .NET. We will look at suspicious code fragments, figure out what's wrong with them, and try to reproduce some of the errors. Make yourself a cup of coffee and get cozy.
Some analysis details
What project is it?
AWS.SDK for .NET enables .NET developers to work with Amazon Web Services, Amazon S3, Amazon DynamoDB, etc.
I've taken the source code from the GitHub page of the project. If you need the exact version, here's the commit SHA: 93a94821dc8ff7a0073b74def6549728da3b51c7.
What tools were used to check the project?
I checked the code with the PVS-Studio analyzer using the plugin for Visual Studio.
What else is there to say?
Some warnings may look familiar to you, as those code fragments haven't changed since the last check. In this article, I duplicated the warnings that I found interesting.
Enough with the check details, let's take a look at the suspicious code fragments.
Examining suspicious code fragments
Issue #1
public static object GetAttr(object value, string path)
{
if (string.IsNullOrEmpty(path)) throw new ArgumentNullException("path");
var parts = path.Split('.');
var propertyValue = value;
for (int i = 0; i < parts.Length; i++)
{
var part = parts[i];
// indexer is always at the end of path e.g. "Part1.Part2[3]"
if (i == parts.Length - 1)
{
....
// indexer detected
if (indexerStart >= 0)
{
....
if (!(propertyValue is IList))
throw
new ArgumentException("Object addressing by pathing segment '{part}'
with indexer must be IList");
....
}
}
if (!(propertyValue is IPropertyBag))
throw
new ArgumentException("Object addressing by pathing segment '{part}'
must be IPropertyBag");
....
}
....
}
It looks like developers forgot to interpolate the exception messages. So, the {part}
string literal will be used instead of the actual value of the part
variable.
Issue #2
private CredentialsRefreshState Authenticate(ICredentials userCredential)
{
....
ICoreAmazonSTS coreSTSClient = null;
try
{
....
coreSTSClient =
ServiceClientHelpers.CreateServiceFromAssembly<ICoreAmazonSTS>(....);
}
catch (Exception e)
{
....
}
var samlCoreSTSClient
#if NETSTANDARD
= coreSTSClient as ICoreAmazonSTS_SAML;
if (coreSTSClient == null)
{
throw new NotImplementedException(
"The currently loaded version of AWSSDK.SecurityToken
doesn't support SAML authentication.");
}
#else
= coreSTSClient;
#endif
try
{
var credentials = samlCoreSTSClient.CredentialsFromSAMLAuthentication(....);
}
catch (Exception e)
{
var wrappedException =
new AmazonClientException("Credential generation from
SAML authentication failed.",
e);
var logger = Logger.GetLogger(typeof(FederatedAWSCredentials));
logger.Error(wrappedException, wrappedException.Message);
throw wrappedException;
}
....
}
The GitHub link.
We need this large code fragment to understand the context better. The error lurks here:
var samlCoreSTSClient
#if NETSTANDARD
= coreSTSClient as ICoreAmazonSTS_SAML;
if (coreSTSClient == null)
{
throw new NotImplementedException(
"The currently loaded version of AWSSDK.SecurityToken
doesn't support SAML authentication.");
}
In the if
statement condition, the wrong variable is checked for null
. The samlCoreSTSClient
variable should have been checked instead of coreSTSClient
.
Let's take a look at the following elements:
-
samlCoreSTSClient
is the name of the resulting variable; -
ICoreAmazonSTS_SAML
is the type of an interface being cast to; -
"... doesn't support SAML authentication"
is the text of the exception message.
SAML is mentioned everywhere except for the name of the variable being checked (coreSTSClient
). :)
It's interesting how checking different variables changes the logic if the casting fails.
When checking samlCoreSTSClient
:
- -> casting with the
as
operator - -> checking if
samlCoreSTSClient
equalsnull
- -> throwing
NotImplementedException
When checking coreSTSClient
:
- -> casting with the
as
operator - -> checking if
coreSTSClient
equalsnull
- -> attempting to call the
CredentialsFromSAMLAuthentication
method - -> throwing the
NotImplementedException
exception - -> catching an exception in
catch
- -> logging the issue
- -> throwing
AmazonClientException
That is, an exception of a different type and with a different message will be thrown in the external code.
By the way, checking for the wrong variable after using the as
operator is a quite common error in C# projects. Take a look at other examples.
Issue #3
public static class EC2InstanceMetadata
{
[Obsolete("EC2_METADATA_SVC is obsolete, refer to ServiceEndpoint
instead to respect environment and profile overrides.")]
public static readonly string EC2_METADATA_SVC = "http://169.254.169.254";
[Obsolete("EC2_METADATA_ROOT is obsolete, refer to EC2MetadataRoot
instead to respect environment and profile overrides.")]
public static readonly string
EC2_METADATA_ROOT = EC2_METADATA_SVC + LATEST + "/meta-data";
[Obsolete("EC2_USERDATA_ROOT is obsolete, refer to EC2UserDataRoot
instead to respect environment and profile overrides.")]
public static readonly string
EC2_USERDATA_ROOT = EC2_METADATA_SVC + LATEST + "/user-data";
[Obsolete("EC2_DYNAMICDATA_ROOT is obsolete, refer to EC2DynamicDataRoot
instead to respect environment and profile overrides.")]
public static readonly string
EC2_DYNAMICDATA_ROOT = EC2_METADATA_SVC + LATEST + "/dynamic";
[Obsolete("EC2_APITOKEN_URL is obsolete, refer to EC2ApiTokenUrl
instead to respect environment and profile overrides.")]
public static readonly string
EC2_APITOKEN_URL = EC2_METADATA_SVC + LATEST + "/api/token";
public static readonly string
LATEST = "/latest",
AWS_EC2_METADATA_DISABLED = "AWS_EC2_METADATA_DISABLED";
....
}
The GitHub link.
Note the order in which the fields are declared and initialized.
The EC2_APITOKEN_URL
, EC2_DYNAMICDATA_ROOT
, EC2_USERDATA_ROOT
, and EC2_METADATA_ROOT
fields are declared first. Each of them uses the LATEST
field in the initializer. However, the field is not yet initialized when being used, because it is declared further in the code. As a result, when calculating values for the EC2_*
fields, the default(string)
value (null
) will be used instead of the "/latest"
string.
We can easily verify the above by referring to the corresponding API:
var arr = new[]
{
EC2InstanceMetadata.EC2_APITOKEN_URL,
EC2InstanceMetadata.EC2_DYNAMICDATA_ROOT,
EC2InstanceMetadata.EC2_USERDATA_ROOT,
EC2InstanceMetadata.EC2_METADATA_ROOT
};
foreach(var item in arr)
Console.WriteLine(item);
The result of code execution:
As you can see, no line has the "/latest"
literal.
But it's debatable if this is a mistake. The order of field initialization was changed in an individual commit. In the same commit, the fields were decorated with the Obsolete
attribute. Although, if you are not going to use the actual LATEST
value, it's better not to use it at all. This way, the code won't confuse anybody.
Issue #4
public IRequest Marshall(GetObjectTorrentRequest getObjectTorrentRequest)
{
IRequest request = new DefaultRequest(getObjectTorrentRequest, "AmazonS3");
request.HttpMethod = "GET";
if (getObjectTorrentRequest.IsSetRequestPayer())
request.Headers
.Add(S3Constants.AmzHeaderRequestPayer,
S3Transforms.ToStringValue(getObjectTorrentRequest.RequestPayer
.ToString()));
if (getObjectTorrentRequest.IsSetRequestPayer())
request.Headers
.Add(S3Constants.AmzHeaderRequestPayer,
S3Transforms.ToStringValue(getObjectTorrentRequest.RequestPayer
.ToString()));
if (getObjectTorrentRequest.IsSetExpectedBucketOwner())
request.Headers
.Add(S3Constants.AmzHeaderExpectedBucketOwner,
S3Transforms.ToStringValue(
getObjectTorrentRequest.ExpectedBucketOwner));
....
}
The GitHub link.
The first two if
statements completely duplicate each other in both conditions and bodies. Either one of them is redundant and needs to be removed, or one of the statements should have a different condition and perform other actions.
Issue #5
public string Region
{
get
{
if (String.IsNullOrEmpty(this.linker.s3.region))
{
return "us-east-1";
}
return this.linker.s3.region;
}
set
{
if (String.IsNullOrEmpty(value))
{
this.linker.s3.region = "us-east-1";
}
this.linker.s3.region = value;
}
}
The GitHub link.
The code above is quite interesting. On the one hand, there is an error. On the other hand, the error will not impact the app's logic if we work only with the Region
property.
The error itself lurks in the set
accessor. value
will always be written to the this.linker.s3.region
property. So, the String.IsNullOrEmpty(value)
check has no effect. There is also a check in the get
accessor: if linker.s3.region
is null
or an empty string, the property returns the "us-east-1"
value.
So, here's what happens: it makes no difference whether there is an issue or not for a user who just deals with the Region
property. Although, it's better to fix the error anyway.
Issue #6
internal string
GetPreSignedURLInternal(....)
{
....
RegionEndpoint endpoint = RegionEndpoint.GetBySystemName(region);
var s3SignatureVersionOverride
= endpoint.GetEndpointForService("s3",
Config.ToGetEndpointForServiceOptions())
.SignatureVersionOverride;
if (s3SignatureVersionOverride == "4" || s3SignatureVersionOverride == null)
{
signatureVersionToUse = SignatureVersion.SigV4;
}
var fallbackToSigV2 = useSigV2Fallback && !AWSConfigsS3.UseSigV4SetExplicitly;
if ( endpoint?.SystemName == RegionEndpoint.USEast1.SystemName
&& fallbackToSigV2)
{
signatureVersionToUse = SignatureVersion.SigV2;
}
....
}
The GitHub link.
A strange order of handling potential null
values attracts bugs. Sometimes the value is used before it is checked for null
. This is where we encounter puzzles: is it an error and an exception will be thrown? Is the check redundant, and the variable can't be null
? Is it something else...?
Here we have a similar issue. Developers accessed the endpoint
variable unconditionally (endpoint.GetEndpointForService
), but then they used the conditional access operator (endpoint?.SystemName
).
Issue #7
public class GetObjectMetadataResponse : AmazonWebServiceResponse
{
....
private ServerSideEncryptionMethod
serverSideEncryption;
private ServerSideEncryptionCustomerMethod
serverSideEncryptionCustomerMethod;
....
public ServerSideEncryptionCustomerMethod
ServerSideEncryptionCustomerMethod
{
get
{
if (this.serverSideEncryptionCustomerMethod == null)
return ServerSideEncryptionCustomerMethod.None;
return this.serverSideEncryptionCustomerMethod;
}
set { this.serverSideEncryptionCustomerMethod = value; }
}
// Check to see if ServerSideEncryptionCustomerMethod property is set
internal bool IsSetServerSideEncryptionCustomerMethod()
{
return this.serverSideEncryptionCustomerMethod != null;
}
....
public ServerSideEncryptionMethod
ServerSideEncryptionMethod
{
get
{
if (this.serverSideEncryption == null)
return ServerSideEncryptionMethod.None;
return this.serverSideEncryption;
}
set { this.serverSideEncryption = value; }
}
// Check to see if ServerSideEncryptionCustomerMethod property is set
internal bool IsSetServerSideEncryptionMethod()
{
return this.serverSideEncryptionCustomerMethod != null;
}
....
}
Let me warn you: similar names are about to make your eyes dazzled. I guess that's what caused an error.
The ServerSideEncryptionMethod
and the ServerSideEncryptionCustomerMethod
properties are defined in the GetObjectMetadataResponse
type. They use the serverSideEncryption
and serverSideEncryptionCustomerMethod
backing fields:
-
ServerSideEncryptionMethod
->serverSideEncryption
; -
ServerSideEncryptionCustomerMethod
->serverSideEncryptionCustomerMethod
.
There are IsSetServerSideEncryptionMethod
and IsSetServerSideEncryptionCustomerMethod
as well. You may assume, they also use the serverSideEncryption
and serverSideEncryptionCustomerMethod
backing fields, respectively... But no. Because of the error, both methods check the serverSideEncryptionCustomerMethod
field.
// Check to see if ServerSideEncryptionCustomerMethod property is set
internal bool IsSetServerSideEncryptionCustomerMethod()
{
return this.serverSideEncryptionCustomerMethod != null;
}
// Check to see if ServerSideEncryptionCustomerMethod property is set
internal bool IsSetServerSideEncryptionMethod()
{
return this.serverSideEncryptionCustomerMethod != null;
}
The IsSetServerSideEncryptionMethod
method should check the serverSideEncryption
field.
Issue #8
public string GetDecryptedPassword(string rsaPrivateKey)
{
RSAParameters rsaParams;
try
{
rsaParams = new PemReader(
new StringReader(rsaPrivateKey.Trim())
).ReadPrivatekey();
}
catch (Exception e)
{
throw new AmazonEC2Exception("Invalid RSA Private Key", e);
}
RSACryptoServiceProvider rsa = new RSACryptoServiceProvider();
rsa.ImportParameters(rsaParams);
byte[] encryptedBytes = Convert.FromBase64String(this.PasswordData);
var decryptedBytes = rsa.Decrypt(encryptedBytes, false);
string decrypted = Encoding.UTF8.GetString(decryptedBytes);
return decrypted;
}
The GitHub link.
The RSACryptoServiceProvider
type implements the IDisposable
interface. In this code, however, the Dispose
method is called neither explicitly nor implicitly.
I can't say if it's critical in this case. However, it would be better to call Dispose
to clean up data, especially when the code is working with passwords, etc.
Issue #9
public class ResizeJobFlowStep
{
....
public OnFailure? OnFailure
{
get { return this.OnFailure; }
set { this.onFailure = value; }
}
....
}
The GitHub link.
Due to a typo in the get
accessor of the OnFailure
property, the OnFailure
property is used instead of the onFailure
backing field. An attempt to get a property value results in an infinite recursion, which causes StackOverflowException
.
We can easily reproduce this error by using the corresponding API:
ResizeJobFlowStep obj = new ResizeJobFlowStep();
_ = obj.OnFailure;
Compile the code, run it, and get the expected result:
Issue #10
private static void
writeConditions(Statement statement, JsonWriter generator)
{
....
IList<string> conditionValues = keyEntry.Value;
if (conditionValues.Count == 0)
continue;
generator.WritePropertyName(keyEntry.Key);
if (conditionValues.Count > 1)
{
generator.WriteArrayStart();
}
if (conditionValues != null && conditionValues.Count != 0)
{
foreach (string conditionValue in conditionValues)
{
generator.Write(conditionValue);
}
}
....
}
The GitHub link.
The code looks weird: first, the reference from the conditionValues
variable is dereferenced, and then it is checked for null
. However, the value of the variable doesn't change. So, if the reference is null
, NullReferenceException
will occur while executing conditionValues.Count == 0
.
This code may have both an error and a redundant check for null
inequality.
There is one thing I'd like to point out. I got the impression that the project developers like to add null
equality checks just in case. :) Take a look at some examples below.
string[] settings
= value.Split(validSeparators, StringSplitOptions.RemoveEmptyEntries);
if (settings == null || settings.Length == 0)
return LoggingOptions.None;
The GitHub link.
The String.Split
method doesn't return null
. There's a similar check here.
Here is another example of a similar check:
var constructors
= GetConstructors(objectTypeWrapper, validConstructorInputs).ToList();
if (constructors != null && constructors.Count > 0)
The GitHub link.
The Enumerable.ToList
method doesn't return null
, so the value of the constructors
variable can never be null
.
The example below is closer to the original one — developers first dereferenced the reference, then checked its value for null
:
TraceSource ts = new TraceSource(testName, sourceLevels);
ts.Listeners.AddRange(AWSConfigs.TraceListeners(testName));
// no listeners? skip
if (ts.Listeners == null || ts.Listeners.Count == 0)
The GitHub link.
Although, I haven't found any cases where the Listeners
property could be null
. In .NET, the return value of the property is marked with a null-forgiving operator (link to GitHub):
public TraceListenerCollection Listeners
{
get
{
Initialize();
return _listeners!;
}
}
Issue #11
private static string GetXamarinInformation()
{
var xamarinDevice = Type.GetType("Xamarin.Forms.Device, Xamarin.Forms.Core");
if (xamarinDevice == null)
{
return null;
}
var runtime = xamarinDevice.GetProperty("RuntimePlatform")
?.GetValue(null)
?.ToString() ?? "";
var idiom = xamarinDevice.GetProperty("Idiom")
?.GetValue(null)
?.ToString() ?? "";
var platform = runtime + idiom;
if (string.IsNullOrEmpty(platform))
{
platform = UnknownPlatform;
}
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", "Xamarin");
}
The GitHub link.
The last line of the method looks odd. The "Xamarin"
string literal is substituted in the "Xamarin_{0}"
template using String.Format
. The value of the platform
variable, which can store the necessary information, is ignored. That's strange.
I assume that the return
statement should look like this:
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform);
By the way, there is a similar method for getting the Unity game engine information. It is written in a similar pattern, but the return value is produced correctly:
private static string GetUnityInformation()
{
var unityApplication
= Type.GetType("UnityEngine.Application, UnityEngine.CoreModule");
if (unityApplication == null)
{
return null;
}
var platform = unityApplication.GetProperty("platform")
?.GetValue(null)
?.ToString() ?? UnknownPlatform;
return string.Format(CultureInfo.InvariantCulture, "Unity_{0}", platform);
}
Conclusion
Before publishing this article, I've already notified the developers of the issues I found in the project — here is the link to the bug report.
Do you want to know if your project has similar issues? Check your code with the PVS-Studio analyzer.
Top comments (0)