Code flaws found by PVS-Studio

[VasilievSerg]

Describe the bug

I've checked the sources via the PVS-Studio static code analyzer. Below I describe few suspicious code fragments found by the analyzer. Note that not all of the detected code flaws are listed here, but only the most interesting ones

I hope the report will be useful. :)

P.S. I've gathered 11 flaws in this issue so as not to describe each one in a separate task. If a separate issue for each flaw would be better, please let me know.

Issue 1

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

Link to the sources.

It looks like the wrong variable is checked for the null value. Perhaps samlCoreSTSClient should be checked.

Issue 2

public static object GetAttr(object value, string path)
{
  if (string.IsNullOrEmpty(path)) throw new ArgumentNullException("path");

  var parts = path.Split('.');
  var propertyValue = value;
            
  ....
  var part = parts[i]; 
  ....
  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");
  ....
}

Links to the sources: link1, link2.

Perhaps both exception messages should be interpolated to insert the part values into the messages.

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

Link to the sources.

Values of EC2_METADATA_ROOT, EC2_USERDATA_ROOT, EC2_DYNAMICDATA_ROOT, EC2_APITOKEN_URL will not include the "/latest" string since the LATEST variable is initialized after them.

I'm not sure if this an error or not.

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

Link to the sources.

The first and the second if statements are fully equal. It looks suspicious.

Issue 5

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

Link to the sources.

The OnFailure property is used in the get accessor instead of the onFailure field. As a result the StackOverflowException will occur if a developer try to get the OnFailure value.

Here is an example:

ResizeJobFlowStep obj = new ResizeJobFlowStep();
_ = obj.OnFailure; // StackOverflowException

Issue 6

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

Link to the sources.
The this.linker.s3.region field will always be rewritten regardless of the value value. Perhaps the return statement is missing here.

Issue 7

RegionEndpoint endpoint = RegionEndpoint.GetBySystemName(region);
var s3SignatureVersionOverride = endpoint.GetEndpointForService("s3", Config.ToGetEndpointForServiceOptions()).SignatureVersionOverride;
....
if (endpoint?.SystemName == RegionEndpoint.USEast1.SystemName && fallbackToSigV2)
{
  signatureVersionToUse = SignatureVersion.SigV2;
}

Link to the sources.

First, the endpoint reference is dereferenced without the null check (endpoint.GetEndpointForService). Second, the endpoint variable is accessed with null checking (endpoint?.SystemName).

If endpoint can't be null, the null-conditional operator is excessive. Otherwise, the first dereference should be performed after the null checking.

Issue 8

private ServerSideEncryptionMethod serverSideEncryption;
private ServerSideEncryptionCustomerMethod serverSideEncryptionCustomerMethod;

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

Links to the sources: link1, link2.

It looks like the wrong field is checked in the IsSetServerSideEncryptionMethod method (this.serverSideEncryptionCustomerMethod instead of serverSideEncryption).

P.S. Look at the ServerSideEncryptionMethod and ServerSideEncryptionCustomerMethod properties. They use correct backing fields, but the mentioned methods do not.

Issue 9

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

Link to the sources.

The RSACryptoServiceProvider type implements the IDisposable interface, but rsa is not disposed here.

Issue 10

IList<string> conditionValues = keyEntry.Value;
if (conditionValues.Count == 0)
  continue;

generator.WritePropertyName(keyEntry.Key);

if (conditionValues.Count > 1)
{ .... }

if (conditionValues != null && conditionValues.Count != 0)
{ .... }

Link to the sources.

The conditionValues reference is dereferenced before it is checked for null value. I'm not sure if there is an error here. Perhaps null checking is just redundant.

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");
}

Link to the sources.

The value of the platform variable is not used after the assignment. Perhaps it should be used in the last return statement instead of the "Xamarin" string literal.

Expected Behavior

Current Behavior

Reproduction Steps

Check the sources with PVS-Studio 7.24.

Possible Solution

No response

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

Commit SHA: 93a9482

Targeted .NET Platform

Operating System and version

Windows 10