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 鈥嬧媠e 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 鈥嬧媔nvocaci贸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/


All Articles