Geekly articles weekly

Buscando errores en el SDK de Amazon Web Services para el código fuente de .NET

Imagen 1


Saludos a todos los fanáticos de criticar el código de otra persona. :) Hoy, en nuestro laboratorio, el nuevo material de investigación es el código fuente del proyecto AWS SDK para .NET. En un momento, escribimos un artículo sobre cómo verificar AWS SDK para C ++. Entonces no había nada particularmente interesante. Veamos cómo nos complacerá la versión .NET del AWS SDK. Una buena oportunidad para demostrar una vez más las capacidades del analizador PVS-Studio, así como hacer que el mundo sea un poco más perfecto.

El SDK .NET de Amazon Web Services (AWS) es un kit de herramientas para desarrolladores diseñado para crear aplicaciones basadas en .NET en la infraestructura de AWS y simplificar enormemente el proceso de escritura de código. El SDK incluye suites API .NET para varios servicios de AWS como Amazon S3, Amazon EC2, DynamoDB y otros. El código fuente del SDK está alojado en GitHub .

Como dije, en un momento escribimos un artículo sobre cómo verificar AWS SDK para C ++. El artículo resultó ser pequeño: solo hubo un par de errores en 512 mil líneas de código. Esta vez estamos lidiando con una cantidad de código mucho mayor, que incluye aproximadamente 34 mil archivos cs, y el número total de líneas de código (excluyendo los vacíos) es de 5 millones impresionantes. Una pequeña parte del código (200 mil líneas en 664 archivos cs) cae en las pruebas, no las consideré.

Si la calidad del código .NET de la versión del SDK es aproximadamente la misma que la de C ++ (dos errores por 512 KLOC), entonces deberíamos obtener aproximadamente 10 veces más errores. Este, por supuesto, es un método de cálculo muy inexacto que no tiene en cuenta las características del lenguaje y muchos otros factores, pero no creo que el lector ahora quiera profundizar en debates aburridos. En cambio, propongo ir directamente a los resultados.

La verificación se realizó con PVS-Studio versión 6.27. Increíblemente, el analizador pudo detectar alrededor de 40 errores en el SDK de AWS para el código .NET que vale la pena mencionar. Esto indica no solo la alta calidad del código SDK (aproximadamente 4 errores por 512 KLOC), sino también la calidad comparable del trabajo C # del analizador PVS-Studio en comparación con C ++. Gran resultado!

Los autores del AWS SDK para .NET son geniales. De proyecto a proyecto, demuestran una increíble calidad de código. Esto puede servir como un buen ejemplo para otros equipos. Pero, por supuesto, no sería el desarrollador de un analizador estático si no hubiera insertado mis 5 kopecks. :) Ya estamos trabajando con el equipo de Amazon Lumberyard en el uso de PVS-Studio. Pero como se trata de una empresa muy grande con un montón de divisiones en todo el mundo, es muy probable que el SDK de AWS para el equipo .NET nunca haya oído hablar de PVS-Studio. En cualquier caso, en el código SDK no encontré ningún signo de uso de nuestro analizador, aunque esto no significa nada. Sin embargo, el equipo, como mínimo, utiliza el analizador integrado en Visual Studio. Esto es bueno, pero las comprobaciones de código se pueden mejorar :).

Como resultado, logré encontrar algunos errores en el código SDK y, finalmente, es hora de compartirlos.

Error en la logica

Advertencia de PVS-Studio: V3008 [CWE-563] La variable 'this.linker.s3.region' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 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; } }

El analizador advierte de reasignar el valor de la misma variable. A partir del código, queda claro que esto se debe a un error que viola la lógica de operación: el valor de la variable this.linker.s3.region siempre será igual al valor , independientemente de la condición if (String.IsNullOrEmpty (valor)) . En el cuerpo del bloque if , se omitió el retorno . El código debe repararse de la siguiente manera:

 public string Region { get { .... } set { if (String.IsNullOrEmpty(value)) { this.linker.s3.region = "us-east-1"; return; } this.linker.s3.region = value; } }

Recursión infinita

Advertencia de PVS-Studio: V3110 [CWE-674] Posible recursión infinita dentro de la propiedad 'OnFailure'. AWSSDK.ElasticMapReduce.Net45 ResizeJobFlowStep.cs 171

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

Un ejemplo clásico de error tipográfico que causa una recursión infinita en el get getor de la propiedad OnFailure . En lugar de devolver el valor del campo privado, onFailure se refiere a la propiedad OnFailure . Opción corregida:

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

Usted pregunta: "¿Cómo funcionó?" Hasta ahora, de ninguna manera. La propiedad no se usa en ningún lado, pero es temporal. En un momento, alguien comenzará a usarlo y definitivamente obtendrá un resultado inesperado. Para evitar estos errores tipográficos, se recomienda no utilizar identificadores que difieran solo en el caso de la primera letra.

Otra nota de este diseño es el uso de un identificador que coincide completamente con el nombre del tipo OnFailure . Desde el punto de vista del compilador, esto es bastante aceptable, pero dificulta la percepción humana del código.

Otro error similar:

Advertencia de PVS-Studio: V3110 [CWE-674] Posible recursión infinita dentro de la propiedad 'SSES3'. AWSSDK.S3.Net45 InventoryEncryption.cs 37

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

La situación es idéntica a la descrita anteriormente. Solo aquí, se producirá una recursión infinita al acceder a la propiedad SSES3 tanto para leer como para escribir. Opción corregida:

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

Prueba de atención plena

La siguiente es una tarea de un programador que está interesado en usar la técnica Copiar-Pegar. Mire cómo se ve el código en el editor de Visual Studio e intente encontrar el error.

Cuadro 3


Advertencia de PVS-Studio: V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 91, 95. AWSSDK.AppSync.Net45 CreateApiKeyResponseUnmarshaller.cs 91

Reduje el cuerpo del método UnmarshallException , eliminando todo lo innecesario. Ahora puede ver que verificaciones idénticas se suceden una tras otra:

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

Puede parecer que el error no es grave, solo un cheque adicional. Pero a menudo, este patrón puede indicar problemas más serios en el código cuando no se realiza alguna verificación necesaria.

Hay algunos errores más similares en el código.

Advertencias de PVS-Studio:

  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Verificación de líneas: 75, 79. AWSSDK.CloudDirectory.Net45 CreateSchemaResponseUnmarshaller.cs 75
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 105, 109. AWSSDK.CloudDirectory.Net45 GetSchemaAsJsonResponseUnmarshaller.cs 105
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 201, 205. AWSSDK.CodeCommit.Net45 PostCommentForPullRequestResponseUnmarshaller.cs 201
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Verificación de líneas: 101, 105. AWSSDK.CognitoIdentityProvider.Net45 VerifySoftwareTokenResponseUnmarshaller.cs 101
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 72, 76. AWSSDK.Glue.Net45 UpdateConnectionResponseUnmarshaller.cs 72
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 123, 127. AWSSDK.Neptune.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 123
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 167, 171. AWSSDK.Neptune.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 167
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 127, 131. AWSSDK.RDS.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 127
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 171, 175. AWSSDK.RDS.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 171
  • V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Verifique las líneas: 99, 103. AWSSDK.Rekognition.Net45 RecognizeCelebritiesResponseUnmarshaller.cs 99

Que eres

Advertencia de PVS-Studio: V3062 Se usa un objeto 'attributeName' como argumento de su propio método. Considere verificar el primer argumento real del método 'Contiene'. 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; }

El analizador detectó un error en el método GetAttribute : se verifica que la cadena se contenga. De la descripción del método, se deduce que si se encuentra el nombre del atributo (clave attributeName ) (en el diccionario _attributes ), el valor del atributo debe devolverse, de lo contrario, será nulo . En realidad, dado que la condición attributeName.Contains (attributeName) siempre es verdadera, se intenta devolver un valor mediante una clave que puede no encontrarse en el diccionario. Luego, en lugar de devolver nulo, se lanzará una KeyNotFoundException .

Intentemos arreglar este código. Para comprender mejor cómo hacer esto, eche un vistazo a otro método:

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

Este método verifica si el nombre del atributo (clave attributeName ) existe en el diccionario _attributes . Volvamos al método GetAttribute y arreglemos el 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; }

Ahora el método hace exactamente lo que se indica en la descripción.

Y un pequeño comentario más a este fragmento de código. Noté que los autores usan el bloqueo cuando trabajan con el diccionario _attributes . Claramente, esto es necesario con acceso multiproceso, pero la construcción de la cerradura es bastante lenta y engorrosa. En lugar de Diccionario en este caso, podría ser más conveniente usar una versión segura del subproceso del diccionario: ConcurrentDictionary . Entonces la necesidad de bloqueo desaparece. Aunque, tal vez no conozco ninguna característica del proyecto.

Comportamiento sospechoso

Advertencia de PVS-Studio: V3063 [CWE-571] Una parte de la expresión condicional siempre es verdadera si se evalúa: 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); .... }

El analizador alertó la verificación string.IsNullOrEmpty (inferredIndexName) . De hecho, la cadena inferredIndexName se asigna como nula , luego el valor de esta variable no se cambia en ninguna parte y, por alguna razón, se verifica si es nula o una cadena vacía. Parece sospechoso Echemos un vistazo de cerca al fragmento de código dado. Deliberadamente no lo reduje para una mejor comprensión de la situación. Por lo tanto, en la primera instrucción if (y también en la siguiente), la variable IndexIndexName especificada se verifica de alguna manera. Dependiendo del resultado de las comprobaciones, la variable inferredIndexName recibe un nuevo valor. Ahora prestemos atención a la tercera declaración if . El cuerpo de esta declaración (lanzando una excepción) se ejecutará si indexNames.Count> 0 , ya que la primera parte de la condición es string.IsNullOrEmpty (inferredIndexName) siempre es verdadero. Quizás las variables especificadas como IndexIndex y inferredIndexName simplemente se confunden. O la tercera verificación debe ser sin otra cosa , representando una declaración if independiente:

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

En este caso, es difícil dar una respuesta inequívoca sobre las opciones para corregir este código. Pero definitivamente es necesario que el autor lo inspeccione.

NullReferenceException

Advertencia de PVS-Studio: V3095 [CWE-476] El objeto 'conditionValues' se usó antes de que se verificara como nulo. Líneas de verificación: 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) { .... } .... } }

Clásicos del género. La variable conditionValues ​​se usa sin verificar primero null . Además, más adelante en el código se realiza dicha verificación, pero es demasiado tarde. :)

El código se puede arreglar de la siguiente manera:

 private static void writeConditions(....) { .... foreach (....) { IList<string> conditionValues = keyEntry.Value; if (conditionValues != null && conditionValues.Count == 0) continue; .... if (conditionValues != null && conditionValues.Count != 0) { .... } .... } }

Se encontraron algunos errores más similares en el código.

Advertencias de PVS-Studio:

  • V3095 [CWE-476] El objeto 'ts.Listeners' se usó antes de que se verificara como nulo. Líneas de verificación: 140, 143. AWSSDK.Core.Net45 Logger.Diagnostic.cs 140
  • V3095 [CWE-476] El objeto 'obj' se usó antes de que se verificara como nulo. Líneas de verificación: 743, 745. AWSSDK.Core.Net45 JsonMapper.cs 743
  • V3095 [CWE-476] El objeto 'multipartUploadMultipartUploadpartsList' se usó antes de que se verificara contra nulo. Verifique las líneas: 65, 67. AWSSDK.S3.Net45 CompleteMultipartUploadRequestMarshaller.cs 65

La siguiente advertencia es muy similar en significado, pero la situación es inversa a la que se discutió anteriormente.

Advertencia de PVS-Studio: V3125 [CWE-476] El objeto 'estado' se usó después de que se verificó contra nulo. Líneas de verificación: 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; // <= .... }

Uno de los fragmentos de código contiene una comprobación de la variable de estado para nulo . Además, según el código, la variable de estado se usa para cancelar la suscripción del evento PreemptExpiryTime , mientras que la comprobación de nulo ya no se realiza y es posible una NullReferenceException . Opción de código más seguro:

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

Hay otros errores similares en el código.

Advertencias de PVS-Studio:

  • V3125 [CWE-476] El objeto 'wrapRequest.Content' se usó después de que se verificó contra nulo. Líneas de verificación: 395, 383. AWSSDK.Core.Net45 HttpHandler.cs 395
  • V3125 [CWE-476] El objeto 'datasetUpdates' se usó después de que se verificó contra nulo. Líneas de verificación: 477, 437. AWSSDK.CognitoSync.Net45 Dataset.cs 477
  • V3125 [CWE-476] El objeto 'cORSConfigurationCORSConfigurationcORSRulesListValue' se usó después de que se verificó contra nulo. Líneas de verificación: 125, 111. AWSSDK.S3.Net45 PutCORSConfigurationRequestMarshaller.cs 125
  • V3125 [CWE-476] El objeto 'lifecycleConfigurationLifecycleConfigurationrulesListValue' se usó después de que se verificó contra nulo. Líneas de verificación: 157, 68. AWSSDK.S3.Net45 PutLifecycleConfigurationRequestMarshaller.cs 157
  • V3125 [CWE-476] El objeto 'this.Key' se usó después de que se verificó contra nulo. Líneas de verificación: 199, 183. AWSSDK.S3.Net45 S3PostUploadRequest.cs 199

Realidad incontestada

Advertencia de PVS-Studio: V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. AWSSDK.Core.Net45 Lexer.cs 651

 private static bool State19 (....) { while (....) { switch (....) { case '"': .... return true; case '\\': .... return true; default: .... continue; } } return true; }

El método siempre devuelve verdadero . Veamos cuán crítico es esto para el código de llamada. Seguí el uso del método State19 . Participa en el llenado de la matriz de controladores fsm_handler_table junto con otros métodos similares (hay 28 de ellos con nombres, respectivamente, de State1 a State28 ). Es importante señalar aquí que, además de State19 , también se emitieron advertencias V3009 [CWE-393] para algunos otros manejadores. Estos son los controladores: Estado23, Estado26, Estado27, Estado28 . Alertas emitidas por el analizador para ellos:

  • V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. AWSSDK.Core.Net45 Lexer.cs 752
  • V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. AWSSDK.Core.Net45 Lexer.cs 810
  • V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. AWSSDK.Core.Net45 Lexer.cs 822
  • V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. AWSSDK.Core.Net45 Lexer.cs 834

Así es como se ve la declaración y la inicialización de la matriz del controlador:

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

Para completar, veamos el código de uno de los manejadores, ante el cual el analizador no tenía quejas, por ejemplo, Estado2 :

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

Y así es como se llaman los controladores:

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

Como puede ver, se lanzará una excepción si se devuelve falso . En nuestro caso, para los manejadores State19, State23, State26, State27 y State28 esto nunca sucederá. Parece sospechoso Por otro lado, hasta cinco controladores tienen un comportamiento similar (siempre devuelve verdadero ), por lo que tal vez esto fue intencionado y no es el resultado de un error tipográfico.

¿Por qué me detengo en todo esto con tanto detalle? Esta situación es muy indicativa en el sentido de que un analizador estático a menudo solo puede indicar un diseño sospechoso. E incluso una persona (no una máquina) que no tiene suficiente conocimiento sobre el proyecto, después de haber estudiado el código, todavía no puede dar una respuesta exhaustiva sobre la presencia de un error. El desarrollador debe estudiar el código.

Verificaciones sin sentido

Advertencia de PVS-Studio: V3022 [CWE-571] La expresión 'doLog' siempre es verdadera. 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) // <= { .... } .... }

Presta atención a la variable doLog . Después de la inicialización con falso , en el código esta variable se volverá verdadera en todos los casos. Entonces la verificación if (doLog) siempre es verdadera. Quizás hubo una rama anterior en este método en la que a la variable doLog no se le asignó ningún valor, luego, en el momento de la verificación, podría contener el valor falso obtenido durante la inicialización. Pero ahora no existe tal rama.

Otro error similar:

Advertencia de PVS-Studio: V3022 La expresión '! Resultado' siempre es falsa. 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(....); } .... }

El analizador afirma que el valor de la variable de resultado siempre es verdadero . Esto solo es posible si el método PutValueHelper siempre devolverá verdadero . Echa un vistazo a este método:

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

De hecho, el método volverá verdadero bajo cualquier condición. Además, el analizador emitió una advertencia para este método. Advertencia de PVS-Studio: V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. SQLiteLocalStorage.cs 1016

Deliberadamente no cité esta advertencia antes cuando estaba considerando otros errores de V3009 , pero la guardé para este caso. Por lo tanto, el analizador tenía razón al indicar el error V3022 en el código de llamada.

Copiar y pegar. De nuevo

Advertencia de PVS-Studio: V3001 Hay subexpresiones idénticas 'this.token == JsonToken.String' a la izquierda y a la derecha de '||' operador 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 // <= )) { .... } .... }

Compare dos veces el campo this.token con el valor JsonToken.String de la enumeración JsonToken . Probablemente una de las comparaciones debería contener un valor de enumeración diferente. Si es así, entonces se ha cometido un grave error.

Refactorización + descuido?

Advertencia de PVS-Studio: V3025 [CWE-685] Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'Formatear'. Argumentos no utilizados: 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)); } .... }

Probablemente, la cadena de formato para el método string.Format contenía previamente el especificador de salida {0} , para el cual se estableció el argumento AWSConfigs.AWSRegionKey . Luego se cambió la línea, el especificador desapareció, pero olvidaron eliminar el argumento. El fragmento de código anterior funciona sin errores (se generaría una excepción en el caso contrario, un especificador sin argumento), pero se ve feo. El código debe arreglarse de la siguiente manera:

 if (region == null) { throw new InvalidOperationException( "EC2 instance metadata was not available or did not contain region information."); }

Inseguro

Advertencia de PVS-Studio: V3083 [CWE-367] Invocación insegura del evento 'mOnSyncSuccess', es posible NullReferenceException. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.CognitoSync.PCL Dataset.cs 827

 protected void FireSyncSuccessEvent(List<Record> records) { if (mOnSyncSuccess != null) { mOnSyncSuccess(this, new SyncSuccessEventArgs(records)); } }

Una situación bastante común de una llamada insegura al controlador de eventos. Entre verificar la variable mOnSyncSuccess para nulo y llamar al controlador, se puede cancelar la suscripción de un evento y su valor se convertirá en cero. La probabilidad de tal escenario es pequeña, pero aún es mejor hacer que el código sea más seguro:

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

Hay otros errores similares en el código.

Advertencias de PVS-Studio:

  • V3083 [CWE-367] Invocación insegura del evento 'mOnSyncFailure', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.CognitoSync.PCL Dataset.cs 839
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.PCL AmazonServiceClient.cs 332
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.PCL AmazonServiceClient.cs 344
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.PCL AmazonServiceClient.cs 357
  • V3083 [CWE-367] Invocación insegura del evento 'mExceptionEvent', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.PCL AmazonServiceClient.cs 366
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.PCL AmazonWebServiceRequest.cs 78
  • V3083 [CWE-367] La ​​invocación insegura del evento 'OnRead', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.PCL EventStream.cs 97
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.Android NetworkReachability.cs 57
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.Android NetworkReachability.cs 94
  • V3083 [CWE-367] Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. AWSSDK.Core.iOS NetworkReachability.cs 54

Clase inacabada

Advertencia de PVS-Studio: V3126 Tipo 'JsonData' que implementa la interfaz IEquatable <T> no anula el método 'GetHashCode'. AWSSDK.Core.Net45 JsonData.cs 26

 public class JsonData : IJsonWrapper, IEquatable<JsonData> { .... }

La clase JsonData contiene mucho código, por lo que no lo di en su totalidad, limitándome solo a la declaración. Esta clase realmente no contiene un método anulado GetHashCode , que no es seguro ya que puede conducir a un comportamiento erróneo cuando se usa el tipo JsonData para trabajar, por ejemplo, con colecciones. Probablemente no haya ningún problema en este momento, pero la estrategia para usar este tipo puede cambiar en el futuro. Este error se describe con más detalle en la documentación .

Conclusión

Esos son todos los errores interesantes que logré detectar en AWS SDK para código .NET usando el analizador estático PVS-Studio. Una vez más, enfatizo la calidad del proyecto. Encontré un número muy pequeño de errores para 5 millones de líneas de código. Aunque, probablemente, un análisis más exhaustivo de las advertencias emitidas me permitiría agregar algunos errores más a esta lista. Pero también es muy probable que clasifique en vano algunas de las advertencias descritas como errores. En este caso, solo el desarrollador, que está en el contexto del código que se verifica, siempre saca conclusiones inequívocas.



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Sergey Khrenov. Búsqueda de errores en el código fuente del SDK de Amazon Web Services para .NET

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

More articles:


All Articles