Azure SDK para .NET:historia sobre una búsqueda de error difícil

Cuando decidimos buscar errores en el proyecto Azure SDK for .NET, nos sorprendió gratamente su tamaño. "Tres millones y medio de líneas de código", decíamos, estudiando las estadísticas del proyecto. Puede haber tantos hallazgos. ¡Ay y ay! El proyecto resultó ser astuto. Entonces, ¿cuál fue el entusiasmo del proyecto y cómo se verificó? Lea este artículo.

Sobre el proyecto

Estoy escribiendo este artículo después del anterior, que también trataba sobre un proyecto relacionado con Microsoft Azure:Azure PowerShell:en su mayoría inofensivo. Entonces, esta vez estaba apostando a un número sólido de errores diversos e interesantes. Después de todo, el tamaño del proyecto es un factor muy importante en términos de análisis estático, en particular cuando se verifica un proyecto por primera vez. De hecho, en la práctica, la aplicación de cheques únicos no es el enfoque correcto. Sin embargo, si los desarrolladores lo aceptan, solo se lleva a cabo en la etapa de introducción del analizador. Al mismo tiempo, nadie se esfuerza por clasificar la enorme cantidad de advertencias y simplemente perder el tiempo como deuda técnica utilizando mecanismos de supresión de advertencias masivas y almacenándolas en bases especiales. Hablando de eso, tener una gran cantidad de advertencias está bien cuando se ejecuta el analizador por primera vez. En cuanto a nosotros, buscamos controles únicos con fines de investigación. Por esta razón, los proyectos grandes son siempre más preferibles para el siguiente análisis en comparación con los pequeños.

Sin embargo, el proyecto SDK de Azure para .NET inmediatamente demostró ser un banco de pruebas inviable. Incluso su impresionante tamaño no ayudó, sino que complicó trabajar en él. La razón se da en las siguientes estadísticas del proyecto:

• Archivos fuente .cs (sin incluir las pruebas):16 500
• Soluciones de Visual Studio (.sln):163
• Líneas de código no vacías:3 462 000
• De estos generados automáticamente:unos 3 300 000
• El repositorio del proyecto está disponible en GitHub.

Aproximadamente el 95% del código se genera automáticamente y gran parte de ese código se repite muchas veces. La verificación de dichos proyectos con un analizador estático suele llevar mucho tiempo y es inútil, ya que hay mucho código viable, pero ilógico (al menos a primera vista) y redundante. Esto conduce a una gran cantidad de falsos positivos.

Toda esa cantidad de código esparcido por las 163 soluciones de Visual Studio se convirtió en la "guinda del pastel". Tomó algunos esfuerzos verificar el código restante (no generado automáticamente). Lo que realmente ayudó fue el hecho de que todo el código generado automáticamente se almacenó en subdirectorios de soluciones por la ruta relativa "\src\Generated". Además, cada archivo .cs de este tipo contiene un comentario especial en la etiqueta :

// <auto-generated>
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for
// license information.
//
// Code generated by Microsoft (R) AutoRest Code Generator.
// Changes may cause incorrect behavior and will be lost if the code is
// regenerated.
// </auto-generated>

Para comprobar la pureza del experimento, comprobé por parches unas diez soluciones autogeneradas seleccionadas al azar. Hablaré del resultado más tarde.

Entonces, a pesar de la pequeña cantidad de código "honesto" restante, aún logré encontrar una serie de errores de lo que quedaba. Esta vez no voy a citar advertencias en el orden de los códigos de diagnóstico de PVS-Studio. En su lugar, agruparé los mensajes en las soluciones en las que se han encontrado.

Bueno, veamos qué logré encontrar en Azure SDK para el código .NET.

Microsoft.Azure.Administrador.Asesor

Esta es una de las muchas soluciones que contiene código generado automáticamente. Como dije antes, revisé al azar alrededor de una docena de tales soluciones. En cada caso, las advertencias fueron las mismas y, como era de esperar, inútiles. Aquí hay un par de ejemplos.

V3022 La expresión 'Credentials !=null' siempre es verdadera. AdvisorManagementClient.cs 204

// Code generated by Microsoft (R) AutoRest Code Generator.
....
public ServiceClientCredentials Credentials { get; private set; }
....
public AdvisorManagementClient(ServiceClientCredentials credentials,
  params DelegatingHandler[] handlers) : this(handlers)
{
  if (credentials == null)
  {
    throw new System.ArgumentNullException("credentials");
  }
  Credentials = credentials;
  if (Credentials != null)    // <=
  {
    Credentials.InitializeServiceClient(this);
  }
}

Obviamente, este código es redundante y las Credentials !=null comprobar no tiene sentido. Sin embargo, el código funciona. Y se genera automáticamente. Por esta razón, no hay quejas aquí.

V3022 La expresión '_queryParameters.Count> 0' siempre es falsa. ConfiguracionesOperaciones.cs 871

// Code generated by Microsoft (R) AutoRest Code Generator.
....
public async Task<AzureOperationResponse<IPage<ConfigData>>>
  ListBySubscriptionNextWithHttpMessagesAsync(....)
{
  ....
  List<string> _queryParameters = new List<string>();
  if (_queryParameters.Count > 0)
  {
    ....
  }
  ....
}

De nuevo, parece una construcción ilógica. Por alguna razón, los autores del código comprueban el tamaño del vacío recién creado. lista. De hecho, todo es correcto. En este punto, la verificación no tiene sentido, pero en caso de que los desarrolladores agreguen la generación de listas, por ejemplo, en función de otra colección, la verificación definitivamente valdrá la pena. Nuevamente, no hay reclamos sobre el código, por supuesto, con respecto a su origen.

Se han emitido cientos de advertencias similares para cada solución generada automáticamente. Dada su inutilidad, creo que no tiene sentido seguir discutiendo tales casos. A continuación, solo se considerarán los errores reales en el código "normal".

Azure.Núcleo

V3001 Hay subexpresiones idénticas 'buffer.Length' a la izquierda ya la derecha del operador '<'. AzureBaseBuffersExtensions.cs 30

public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....)
{
  byte[]? array = null;
  ....
  if (array == null || buffer.Length < buffer.Length)  // <=
  {
    if (array != null)
      ArrayPool<byte>.Shared.Return(array);
    array = ArrayPool<byte>.Shared.Rent(buffer.Length);
  }
  if (!buffer.TryCopyTo(array))
    throw new Exception("could not rent large enough buffer.");
  ....
}

El error en la condición probablemente fue el resultado de copiar y pegar. Según el hecho de que buffer se copia en matriz , el cheque debería verse así:

if (array == null || array.Length < buffer.Length)

De todos modos, como siempre digo, el autor del código debería ocuparse de corregir dichos errores.

V3083 La invocación no segura del evento '_onChange', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. ClientOptionsMonitor.cs 44

private event Action<TOptions, string> _onChange;
....
private void InvokeChanged(....)
{
  ....
  if (_onChange != null)
  {
    _onChange.Invoke(options, name);
  }
}

No es crítico, pero hay un error aquí. El consumidor puede darse de baja del evento entre verificar el evento para null y su invocación. Entonces el _onChange la variable será null y se lanzará una excepción. Este código tiene que ser reescrito de una manera más segura. Por ejemplo, de la siguiente manera:

private void InvokeChanged(....)
{
  ....
  _onChange?.Invoke(options, name);
}

Azure.Messaging.EventHubs

V3080 Posible falta de referencia nula. Considere inspeccionar 'eventPropertyValue'. AmqpMessageConverter.cs 650

private static bool TryCreateEventPropertyForAmqpProperty(
  object amqpPropertyValue,
  out object eventPropertyValue)
{
  eventPropertyValue = null;
  ....
  switch (GetTypeIdentifier(amqpPropertyValue))
  {
    case AmqpProperty.Type.Byte:
    ....
    case AmqpProperty.Type.String:
      eventPropertyValue = amqpPropertyValue;
      return true;
    ....
  }
  ....
  switch (amqpPropertyValue)
  {
    case AmqpSymbol symbol:
      eventPropertyValue = ....;
      break;

    case byte[] array:
      eventPropertyValue = ....;
      break;

    case ArraySegment<byte> segment when segment.Count == segment.Array.Length:
      eventPropertyValue = ....;
      break;

    case ArraySegment<byte> segment:
      ....
      eventPropertyValue = ....;
      break;

    case DescribedType described when (described.Descriptor is AmqpSymbol):
      eventPropertyValue = ....;
      break;

    default:
      var exception = new SerializationException(
        string.Format(...., eventPropertyValue.GetType().FullName));  // <=
      ....
  }

  return (eventPropertyValue != null);
}

Veamos qué sucede con el eventPropertyValue valor variable en el fragmento de código dado. La variable se asigna null al principio del método. Además, en uno de los primeros interruptores condiciones, la variable se inicializa, después de lo cual el método sale. El segundo interruptor bloque contiene muchas condiciones, en cada una de las cuales la variable también recibe un nuevo valor. Mientras que en el predeterminado bloque, el eventPropertyValue la variable se usa sin ninguna verificación, lo cual es un error, ya que la variable es null en este momento.

V3066 Posible orden incorrecto de los argumentos pasados ​​al constructor 'EventHubConsumer':'partitionId' y 'consumerGroup'. TrackOneEventHubClient.cs 394

public override EventHubConsumer CreateConsumer(....)
{
  return new EventHubConsumer
  (
    new TrackOneEventHubConsumer(....),
    TrackOneClient.EventHubName,
    partitionId,                  // <= 3
    consumerGroup,                // <= 4
    eventPosition,
    consumerOptions,
    initialRetryPolicy
  );
}

El analizador sospechó que el orden de los argumentos tercero y cuarto era confuso al llamar al EventHubConsumer constructor de clases. Entonces, revisemos esta declaración del constructor:

internal EventHubConsumer(TransportEventHubConsumer transportConsumer,
                          string eventHubName,
                          string consumerGroup,         // <= 3
                          string partitionId,           // <= 4
                          EventPosition eventPosition,
                          EventHubConsumerOptions consumerOptions,
                          EventHubRetryPolicy retryPolicy)
{
  ....
}

De hecho, los argumentos se mezclan. Me atrevería a sugerir cómo se cometió el error. Quizás, el formato de código incorrecto sea el culpable aquí. Solo eche otro vistazo al EventHubConsumer declaración del constructor. Debido al hecho de que el primer transportConsumer está en la misma línea que el nombre de la clase, puede parecer que el partitionId el parámetro está en el tercer lugar, no en el cuarto (mis comentarios con los números de parámetro no están disponibles en el código original). Eso es solo una suposición, pero cambiaría el formato del código del constructor a lo siguiente:

internal EventHubConsumer
(
  TransportEventHubConsumer transportConsumer,
  string eventHubName,
  string consumerGroup,
  string partitionId,
  EventPosition eventPosition,
  EventHubConsumerOptions consumerOptions,
  EventHubRetryPolicy retryPolicy)
{
  ....
}

Azure.Almacenamiento

V3112 Una anormalidad dentro de comparaciones similares. Es posible que haya un error tipográfico dentro de la expresión 'ContentLanguage ==other.ContentEncoding'. BlobSasBuilder.cs 410

public struct BlobSasBuilder : IEquatable<BlobSasBuilder>
{
  ....
  public bool Equals(BlobSasBuilder other) =>
    BlobName == other.BlobName &&
    CacheControl == other.CacheControl &&
    BlobContainerName == other.BlobContainerName &&
    ContentDisposition == other.ContentDisposition &&
    ContentEncoding == other.ContentEncoding &&         // <=
    ContentLanguage == other.ContentEncoding &&         // <=
    ContentType == other.ContentType &&
    ExpiryTime == other.ExpiryTime &&
    Identifier == other.Identifier &&
    IPRange == other.IPRange &&
    Permissions == other.Permissions &&
    Protocol == other.Protocol &&
    StartTime == other.StartTime &&
    Version == other.Version;
}

Un error cometido por falta de atención. Encontrar tal error con la revisión de código es bastante difícil. Esta es la versión correcta del código:

    ....
    ContentEncoding == other.ContentEncoding &&
    ContentLanguage == other.ContentLanguage &&
    ....

V3112 Una anormalidad dentro de comparaciones similares. Es posible que haya un error tipográfico dentro de la expresión 'ContentLanguage ==other.ContentEncoding'. ArchivoSasBuilder.cs 265

public struct FileSasBuilder : IEquatable<FileSasBuilder>
{
  ....
  public bool Equals(FileSasBuilder other)
    => CacheControl == other.CacheControl
    && ContentDisposition == other.ContentDisposition
    && ContentEncoding == other.ContentEncoding         // <=
    && ContentLanguage == other.ContentEncoding         // <=
    && ContentType == other.ContentType
    && ExpiryTime == other.ExpiryTime
    && FilePath == other.FilePath
    && Identifier == other.Identifier
    && IPRange == other.IPRange
    && Permissions == other.Permissions
    && Protocol == other.Protocol
    && ShareName == other.ShareName
    && StartTime == other.StartTime
    && Version == other.Version
    ;

Hay exactamente el mismo error en un código muy similar. Es posible que el código se haya copiado y modificado parcialmente. Pero el error permaneció.

Microsoft.Azure.Batch

V3053 Una expresión excesiva. Examine las subcadenas 'IList' y 'List'. PropertyData.cs 157

V3053 Una expresión excesiva. Examine las subcadenas 'List' y 'IReadOnlyList'. PropertyData.cs 158

public class PropertyData
{
  ....
  public bool IsTypeCollection => this.Type.Contains("IList") ||
                                  this.Type.Contains("IEnumerable") ||
                                  this.Type.Contains("List") ||        // <=
                                  this.Type.Contains("IReadOnlyList"); // <=
}

El analizador emitió dos advertencias sobre controles sin sentido o erróneos. En el primer caso, buscar la subcadena "Lista" después de buscar "IList" parece redundante. Es cierto, esta condición:

this.Type.Contains("IList") || this.Type.Contains("List")

puede ser bien cambiado por lo siguiente:

this.Type.Contains("List")

En el segundo caso, la búsqueda de la subcadena "IReadOnlyList" no tiene sentido, ya que anteriormente se buscaba una subcadena más corta "Lista".

También existe la posibilidad de que las subcadenas de búsqueda tengan errores y debería haber algo más. De todos modos, solo el autor del código puede sugerir la versión correcta del código teniendo en cuenta ambos comentarios.

V3095 El objeto 'httpRequest.Content.Headers' se usó antes de que se verificara contra nulo. Verifique las líneas:76, 79. BatchSharedKeyCredential.cs 76

public override Task ProcessHttpRequestAsync(
  HttpRequestMessage httpRequest, ....)
{
  ....
  signature.Append(httpRequest.Content != null
    && httpRequest.Content.Headers.Contains("Content-Language") ? .... :  
                                                                  ....;

  long? contentLength = httpRequest.Content?.Headers?.ContentLength;
  ....
}

Los httpRequest.Content.Headers La variable se usa primero sin ninguna verificación, pero luego se aborda usando el operador de acceso condicional.

V3125 El objeto 'omPropertyData' se usó después de que se verificó contra nulo. Verifique las líneas:156, 148. CodeGenerationUtilities.cs 156

private static string GetProtocolCollectionToObjectModelCollectionString(
  ...., PropertyData omPropertyData, ....)
{
  if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....))
  {
    ....
  }

  if (IsTypeComplex(omPropertyData.GenericTypeParameter))
  ....
}

Y aquí hay una situación inversa. Un bloque de código contiene una variante de acceso seguro a omPropertyData referencia potencialmente nula. Más adelante en el código, esta referencia se maneja sin ninguna verificación.

V3146 Posible falta de referencia nula de 'valor'. El 'FirstOrDefault' puede devolver un valor nulo predeterminado. BatchSharedKeyCredential.cs 127

public override Task
  ProcessHttpRequestAsync(HttpRequestMessage httpRequest, ....)
{
  ....
  foreach (string canonicalHeader in customHeaders)
  {
    string value = httpRequest.Headers.
                   GetValues(canonicalHeader).FirstOrDefault();
    value = value.Replace('\n', ' ').Replace('\r', ' ').TrimStart();
    ....
  }
  ....
}

Debido a FirstOrDefault método, si la búsqueda falla, se devolverá el valor predeterminado, que es null para la cadena escribe. El valor se asignará al valor variable, que luego se usa en el código con Reemplazar método sin controles. El código debe hacerse más seguro. Por ejemplo, de la siguiente manera:

foreach (string canonicalHeader in customHeaders)
{
  string value = httpRequest.Headers.
                 GetValues(canonicalHeader).FirstOrDefault();
  value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart();
  ....
}

Microsoft.Azure.ServiceBus

V3121 Se declaró una enumeración 'BlocksUsing' con el atributo 'Flags', pero no establece ningún inicializador para anular los valores predeterminados. Fx.cs 69

static class Fx
{
  ....
  public static class Tag
  {
    ....
    [Flags]
    public enum BlocksUsing
    {
      MonitorEnter,
      MonitorWait,
      ManualResetEvent,
      AutoResetEvent,
      AsyncResult,
      IAsyncResult,
      PInvoke,
      InputQueue,
      ThreadNeutralSemaphore,
      PrivatePrimitive,
      OtherInternalPrimitive,
      OtherFrameworkPrimitive,
      OtherInterop,
      Other,

      NonBlocking,
    }
    ....
  }
  ....
}

La enumeración se declara con las Flags atributo. Al mismo tiempo, los valores constantes se dejan por defecto (MonitorEnter =0 , Espera de monitor =1 , Evento de reinicio manual =2 y así). Esto puede resultar en el siguiente caso:al intentar usar una combinación de banderas, por ejemplo, la segunda y la tercera constantes MonitorWait (=1) | Evento de reinicio manual (=2) , no se recibirá un valor único, sino la constante con el valor 3 por defecto (AutoResetEvent ). Esto puede ser una sorpresa para el código de la persona que llama. Si BlocksUsing la enumeración realmente se debe usar para establecer combinaciones de banderas (campo de bits), las constantes deben recibir valores, iguales al número que son potencias de dos.

[Flags]
public enum BlocksUsing
{
  MonitorEnter = 1,
  MonitorWait = 2,
  ManualResetEvent = 4,
  AutoResetEvent = 8,
  AsyncResult = 16,
  IAsyncResult = 32,
  PInvoke = 64,
  InputQueue = 128,
  ThreadNeutralSemaphore = 256,
  PrivatePrimitive = 512,
  OtherInternalPrimitive = 1024,
  OtherFrameworkPrimitive = 2048,
  OtherInterop = 4096,
  Other = 8192,

  NonBlocking = 16384,
}

V3125 El objeto 'sesión' se usó después de que se verificó contra nulo. Verifique las líneas:69, 68. AmqpLinkCreator.cs 69

public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
{
  ....
  AmqpSession session = null;
  try
  {
    // Create Session
    ....
  }
  catch (Exception exception)
  {
    ....
    session?.Abort();
    throw AmqpExceptionHelper.GetClientException(exception, null,
      session.GetInnerException(), amqpConnection.IsClosing());
  }
  ....
}

Presta atención a la sesión manejo de variables en el catch bloquear. El abortar El operador de acceso condicional llama al método de forma segura. Pero después de la GetInnerException El método se llama de forma insegura. Al hacerlo, NullReferenceException podría lanzarse en lugar de una excepción del tipo esperado. Este código tiene que ser arreglado. La AmqpExceptionHelper.GetClientException El método admite pasar el null valor para la innerException parámetro:

public static Exception GetClientException(
  Exception exception, 
  string referenceId = null, 
  Exception innerException = null, 
  bool connectionError = false)
{
  ....
}

Por lo tanto, solo se puede usar el operador de acceso condicional al llamar a session.GetInnerException() :

public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
{
  ....
  AmqpSession session = null;
  try
  {
    // Create Session
    ....
  }
  catch (Exception exception)
  {
    ....
    session?.Abort();
    throw AmqpExceptionHelper.GetClientException(exception, null,
      session?.GetInnerException(), amqpConnection.IsClosing());
  }
  ....
}

Conclusión

Como puede ver, un tamaño de proyecto grande no siempre garantiza muchos errores. Sin embargo, nos mantenemos alerta ya que siempre podemos encontrar algo. Incluso en un proyecto estructuralmente tan complejo como Azure SDK para .NET. Encontrar algunos defectos cruciales requiere esfuerzos adicionales. Pero cuantas más dificultades, más agradable el resultado. Por otro lado, para evitar esfuerzos indebidos, recomendamos usar el análisis estático directamente en las computadoras de los desarrolladores al escribir código nuevo. Este es el enfoque más eficaz. Descargue y pruebe PVS-Studio en acción. ¡Buena suerte en la lucha contra los insectos!