B√ļsqueda de errores en el c√≥digo fuente del SDK de Amazon Web Services para .NET

Imagen 1


Bienvenido a todos los fanáticos de destrozar el código de otra persona. :) Hoy en nuestro laboratorio, tenemos un nuevo material para una investigación: el código fuente del AWS SDK para el proyecto .NET. En ese momento, escribimos un artículo sobre la comprobación de AWS SDK para C ++. Entonces no había nada particularmente interesante. Veamos qué vale .NET de la versión AWS SDK. Una vez más, es una gran oportunidad para demostrar las habilidades del analizador PVS-Studio y hacer que el mundo sea un poco mejor.

El SDK de Amazon Web Services (AWS) para .NET es un conjunto de herramientas de desarrollador destinadas a crear aplicaciones basadas en .NET en la infraestructura de AWS. Este conjunto permite simplificar significativamente el proceso de escritura de código. El SDK incluye conjuntos API .NET para varios servicios de AWS, como Amazon S3, Amazon EC2, DynamoDB y otros. El código fuente del SDK está disponible en GitHub .

Como mencion√©, en ese momento ya hemos escrito el art√≠culo sobre la comprobaci√≥n de AWS SDK para C ++. El art√≠culo result√≥ ser peque√Īo: solo se encontraron un par de errores por cada 512 mil l√≠neas de c√≥digo. Esta vez estamos tratando con un tama√Īo mucho m√°s grande del c√≥digo, que incluye aproximadamente 34 mil archivos cs, y el n√ļmero total de l√≠neas de c√≥digo (excluyendo las en blanco) es impresionante de 5 millones. Una peque√Īa parte del c√≥digo (200 mil l√≠neas en archivos 664-cs) se acumula en las pruebas, no las he considerado.

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 un n√ļmero de errores aproximadamente 10 veces mayor. Por supuesto, esta es una metodolog√≠a de c√°lculo muy inexacta, que no tiene en cuenta las peculiaridades ling√ľ√≠sticas y muchos otros factores, pero no creo que el lector ahora quiera entrar en un razonamiento aburrido. En cambio, sugiero pasar a los resultados.

La verificación se realizó con PVS-Studio 6.27. Es simplemente increíble, pero el hecho es que en el SDK de AWS para .NET el analizador logró detectar 40 errores, de los que valdría la pena hablar. Demuestra no solo una alta calidad del código SDK (aproximadamente 4 errores por 512 KLOC), sino también una calidad comparable del analizador C # PVS-Studio en comparación con C ++. ¡Un gran resultado!

Autores de AWS SDK para .NET, ¬°son verdaderos campeones! Con cada proyecto, demuestra una calidad tremenda del c√≥digo. Puede ser un gran ejemplo para otros equipos. Sin embargo, por supuesto, no ser√≠a un desarrollador de un analizador est√°tico si no diera mis 2 centavos. :) Ya estamos trabajando con un equipo de Lumberyard de Amazon en el uso de PVS-Studio. Dado que es una empresa muy grande con un mont√≥n de unidades en todo el mundo, es muy probable que el equipo de AWS SDK para .NET nunca haya o√≠do hablar de PVS-Studio. De todos modos, no he encontrado ning√ļn signo de uso de nuestro analizador en el c√≥digo SDK, aunque no dice nada. Sin embargo, al menos, el equipo usa el analizador integrado en Visual Studio. Es genial, pero las revisiones de c√≥digo siempre 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 sobre la asignación de valores repetidos a la misma variable. Del código queda claro que esto se debe al error que viola la lógica del trabajo del programa: el valor de la variable this.linker.s3.region siempre será igual al valor del valor de la variable, independientemente de la condición if (String.IsNullOrEmpty (valor)) . se perdió la declaración de retorno en el cuerpo del bloque if . 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 un error tipográfico, que conduce a una recursión infinita en el get getor de la propiedad OnFailure . En lugar de devolver el valor de un campo privado en Failure, se realiza el acceso a la propiedad OnFailure . Variante correcta:

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

Puede preguntar: "¬ŅC√≥mo funcion√≥?" Hasta ahora, no c√≥mo. La propiedad no se usa en ning√ļn otro lugar, pero esto es temporal. En un momento, alguien comenzar√° a usarlo y ciertamente recibir√° un resultado inesperado. Para evitar estos errores tipogr√°ficos, se recomienda no utilizar identificadores que difieran solo en el caso de la primera letra.

Otro comentario a esta construcción es el uso del identificador, que coincide completamente con el nombre del tipo OnFailure . Desde el punto de vista del compilador, es bastante aceptable, pero esto complica la percepción del código para una persona.

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. Sin embargo, aquí ocurrirá una recursión infinita al acceder a la propiedad SSES3 tanto para leer como para asignar. Variante correcta:

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

Prueba en consideración

Ahora me gustaría citar una tarea de un desarrollador, tomada con el método Copy-Paste. Eche un vistazo a la apariencia del código en el editor de Visual Studio e intente encontrar un 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 , habiendo eliminado todo lo que no es necesario. Ahora puede ver que verificaciones idénticas se suceden:

 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 áspero, solo una comprobación adicional. Sin embargo, a menudo dicho patrón puede indicar problemas más serios en el código, cuando no se realizará una verificación necesaria.

En el código, hay varios errores similares.

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 es

Advertencia de PVS-Studio: V3062 Se utiliza un objeto 'attributeName' como argumento para 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 ha detectado un error en el método GetAttribute : se verifica si una cadena se contiene a sí misma. De la descripción del método se deduce que si se encuentra el nombre del atributo (clave del nombre del atributo ) (en el diccionario _atributos ), el valor del atributo debe ser devuelto, de lo contrario - nulo . De hecho, como la condición attributeName.Contains (attributeName) siempre es verdadera, se intenta devolver el valor mediante una clave que podría no encontrarse en un diccionario. Luego, en lugar de devolver nulo, se generará una excepción KeyNotFoundException .

Intentemos arreglar este código. Para entender mejor cómo hacer esto, debe buscar 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 (key attribute key) existe en el diccionario _attributes . Volvamos al método GetAttribute nuevamente 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.

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 . Est√° claro que esto es necesario cuando se tiene un acceso multiproceso, pero la construcci√≥n de la cerradura es bastante lenta y engorrosa. En lugar de un diccionario , en este caso, tal vez, ser√≠a m√°s conveniente utilizar la versi√≥n segura del subproceso del diccionario: ConcurrentDictionary . De esta manera, no habr√° necesidad de bloqueo. Aunque, tal vez no s√© sobre los detalles 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 estaba preocupado por la cadena de verificaci√≥n. IsNullOrEmpty (inferredIndexName) . De hecho, la cadena inferredIndexName se asigna como nula , luego el valor de esta variable no se cambia en ning√ļn lado, luego, por alguna raz√≥n, se verifica para una cadena nula o vac√≠a. Parece sospechoso Echemos un vistazo de cerca al fragmento de c√≥digo anterior. Deliberadamente no lo reduje para entender mejor la situaci√≥n. Entonces, en la primera instrucci√≥n if (y tambi√©n en la siguiente), la variable especificada IndexIndexName se verifica de alguna manera. Dependiendo de los resultados de las comprobaciones, la variable inferredIndexName est√° obteniendo un nuevo valor. Ahora veamos la tercera declaraci√≥n if . El cuerpo de esta declaraci√≥n (lanzamiento de la excepci√≥n) se realizar√° en caso de que indexNames.Count> 0, como la primera parte de la condici√≥n completa, que es string.IsNullOrEmpty (inferredIndexName) siempre es verdadero. Quiz√°s, las variables especificadas como IndexIndex y inferredIndexName se mezclan o la tercera verificaci√≥n tiene que ser sin otra cosa , lo que representa 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 definitiva sobre las opciones para corregir este código. De todos modos, el autor necesita verificarlo.

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

Es un clasico. La variable conditionValues se usa sin una comprobación preliminar de nulo . Mientras más tarde en el código se realiza esta verificación. El código debe corregirse 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) { .... } .... } } 

Encontré varios errores 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 el caso es opuesto al discutido 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 incluye verificar el valor de la variable de estado para nulo . En el siguiente código, la variable se utiliza para cancelar la suscripción del evento PreemptExpiryTime , sin embargo, ya no se realiza una comprobación de nulo y es posible lanzar la excepción NullReferenceException . Una versión más segura del código:

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

En el código, hay otros errores similares:

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 no alternativa

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 para el código de llamada. Revisé los casos de usar el método State19 . Está involucrado en llenar la matriz de manejadores fsm_handler_table por igual con otros métodos similares (hay 28 de ellos con los nombres, respectivamente, comenzando por State1 a State28 ). Aquí es importante tener en cuenta que, además de State19 , para algunos otros manejadores también se emitieron las advertencias V3009 [CWE-393]. Estos son controladores: Estado23, Estado26, Estado27, Estado28 . Las advertencias, 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 los controladores:

 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 la imagen, veamos el c√≥digo de uno de los controladores a los que el analizador no ha tenido ning√ļn reclamo, por ejemplo, State2 :

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

Así es como ocurre la llamada de los manejadores:

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

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

¬ŅPor qu√© estoy yendo tan profundo en todo esto? Esta situaci√≥n es muy significativa en el sentido de que el analizador est√°tico a menudo solo puede indicar una construcci√≥n sospechosa. E incluso una persona (no una m√°quina), que no tiene suficiente conocimiento sobre el proyecto, a√ļn no puede dar una respuesta completa sobre la presencia del error, incluso despu√©s de haber pasado tiempo aprendiendo c√≥digo. Un desarrollador debe revisar este 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 el valor falso , esta variable obtendr√° el valor verdadero en todos los casos a lo largo del c√≥digo. Por lo tanto, la comprobaci√≥n if (doLog) siempre es verdadera. Quiz√°s, anteriormente en el m√©todo hab√≠a una rama, en la cual a la variable doLog no se le asign√≥ ning√ļn valor. En el momento de la comprobaci√≥n, podr√≠a contener el valor falso , recibido al inicializar. 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. Solo es posible en caso de que el método PutValueHelper siempre devuelva 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 en todas las condiciones. Adem√°s, el analizador ha emitido 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 investigando otros errores V3009 y la guard√© para este caso. Por lo tanto, la herramienta ten√≠a raz√≥n al se√Īalar 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 // <= )) { .... } .... } 

El campo this.token se compara dos veces con el valor JsonToken.String de la enumeración JsonToken . Probablemente, una de las comparaciones debe contener otro valor de enumeración. Si es así, se ha cometido un grave error aquí.

¬ŅRefactorizaci√≥n + falta de atenci√≥n?

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

Quizás, la cadena de formato para el método string.Format contenía previamente el elemento de formato {0}, para el cual se estableció el argumento AWSConfigs.AWSRegionKey . Luego se cambió la cadena, el elemento de formato desapareció, pero un desarrollador olvidó eliminar el argumento. El ejemplo de código dado funciona sin errores (la excepción se produjo en el caso contrario, el elemento de formato sin el argumento), pero no se ve bien. El código debe corregirse 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 com√ļn de una llamada insegura del controlador de eventos. Un usuario puede darse de baja entre la comprobaci√≥n de la variable mOnSyncSuccess para nulo y la llamada de un controlador, por lo que su valor se convertir√° en nulo . 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)); } 

En el código, hay otros errores similares:

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 cruda

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 bastante c√≥digo, as√≠ que no lo di en su totalidad, citando solo su declaraci√≥n. Esta clase realmente no contiene el m√©todo anulado GetHashCode, que no es seguro, ya que puede conducir a un comportamiento err√≥neo al usar el tipo JsonData para trabajar, por ejemplo, con colecciones. Probablemente, no hay ning√ļn problema en este momento, pero en el futuro este tipo de estrategia podr√≠a cambiar. Este error se describe en la documentaci√≥n con m√°s detalle.

Conclusión

Todos estos son errores interesantes que pude detectar en el c√≥digo de AWS SDK para .NET usando el analizador est√°tico PVS-Studio. Me gustar√≠a destacar una vez m√°s 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. Sin embargo, tambi√©n es bastante probable que agregue algunas de las advertencias a los errores por nada. Las conclusiones inequ√≠vocas en este caso siempre son hechas solo por un desarrollador que est√© en el contexto del c√≥digo verificado.

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


All Articles