A medida que continuamos desarrollando el analizador de código estático PVS-Studio, a menudo tenemos que revisar grandes proyectos de código abierto de desarrolladores de renombre. El hecho de que incluso estos proyectos contengan una cierta cantidad de errores añade aún más sentido y peso a nuestro trabajo. Desafortunadamente, todo el mundo comete errores. No importa cuán cuidadosamente controle la calidad de su código, simplemente no hay forma de evitar el "error humano". Mientras el software sea desarrollado por humanos, las herramientas de análisis como PVS-Studio seguirán siendo relevantes y necesarias. Hoy, vamos a discutir los errores encontrados en el código fuente de MSBuild, que, lamentablemente, tampoco es perfecto.
Introducción
Microsoft Build Engine (MSBuild) es una plataforma de Microsoft para crear aplicaciones. Suele utilizarse junto con Microsoft Visual Studio pero no depende de él. MSBuild proporciona un esquema XML para archivos de proyecto (*.csproj, *.vbproj, *.vcxproj) que controla cómo la plataforma de compilación procesa y compila software. La herramienta se envía como parte de la plataforma .NET y está escrita en C#.
Microsoft proporciona archivos fuente de MSBuild para carga gratuita en GitHub. Teniendo en cuenta los altos estándares de desarrollo aplicados por Microsoft, encontrar errores en MSBuild puede ser una tarea difícil incluso para los analizadores estáticos de alta calidad. Pero el éxito viene con la tenacidad. Con la ayuda de PVS-Studio, versión 6.07, revisamos el código fuente del proyecto MSBuild y esto es lo que encontramos.
Datos de análisis y estadísticas
MSBuild consta de 14 proyectos, que incluyen un total de 1256 archivos fuente en C#. Eso hace aproximadamente 440 000 LOC.
PVS-Studio emitió 262 advertencias para este proyecto. Las estadísticas generales de análisis con diferenciación de advertencias según los niveles de gravedad se muestran en el siguiente gráfico:
Como puede ver en el gráfico, la herramienta emitió 73 advertencias de alto nivel, 107 advertencias de nivel medio y 82 advertencias de bajo nivel. Nos centraremos en los niveles Alto y Medio, ya que contienen construcciones potencialmente peligrosas y errores genuinos, mientras que las advertencias de bajo nivel, aunque también se ocupan de los errores, a menudo resultan ser falsos positivos, por lo que generalmente no las discutimos en nuestros artículos.
El análisis de MSBuild ha revelado que aproximadamente el 45 % de las advertencias de nivel alto y medio apuntan a un código incorrecto (81 errores), mientras que el resto de las advertencias simplemente se refieren a construcciones que PVS-Studio encuentra sospechosas y falsos positivos en lugar de errores reales. . Algunas de las advertencias fueron activadas por pruebas unitarias o código con comentarios sobre construcciones potencialmente peligrosas utilizadas para verificar excepciones. En cualquier caso, las advertencias restantes deben ser examinadas por los desarrolladores, ya que son las únicas personas que tienen pleno conocimiento del código y pueden estimar de forma fiable si las advertencias son correctas o no.
Según estos datos, la proporción de PVS-Studio de errores de nivel medio y alto por 1 KLOC (es decir, densidad de errores) es de solo 0,184 (errores por 1 KLOC). Esta cifra no es algo de lo que sorprenderse en el caso de los proyectos de Microsoft y es una señal de la alta calidad del código de MSBuild.
Ahora analicemos los resultados del análisis en detalle. Tenga en cuenta también que el trabajo de examinar todas las advertencias emitidas para este proyecto está más allá del alcance de nuestro artículo, por lo que solo hablaremos sobre los defectos más interesantes, clasificándolos en grupos.
Resultados del análisis
Comprobación nula incorrecta
Mensaje de diagnóstico de PVS-Studio :V3019 Posiblemente una variable incorrecta se compara con nula después de la conversión de tipo utilizando la palabra clave 'as'. Compruebe las variables 'obj', 'nombre'. Reasignación de ensamblajes.cs 64
Este es probablemente un error clásico:lo vemos en casi todos los proyectos que revisamos. Tiene que ver con convertir una variable a un tipo diferente usando el como operador y probando la misma variable, en lugar de la resultante, para null :
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (obj == null) // <=
{
return false;
}En cambio, es el nombre variable que debe verificarse:
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (name == null)
{
return false;
}
Comprobación nula tardía
Mensaje de diagnóstico de PVS-Studio :V3095 El objeto 'diskRoots' se usó antes de que se verificara contra nulo. Verifique las líneas:2656, 2659. ToolLocationHelper.cs 2656
Tenga en cuenta las raíces de disco parámetro. Es una instancia de la Lista clase y puede tener un valor de null . Sin embargo, la verificación correspondiente se realiza solo en el segundo si bloque, después de diskRoots la variable ya se ha utilizado para insertar valores en una lista:
private static void ExtractSdkDiskRootsFromEnvironment
(List<string> diskRoots, string directoryRoots)
{
if (!String.IsNullOrEmpty(directoryRoots))
{
....
diskRoots.AddRange(splitRoots); // <=
}
if (diskRoots != null) // <=
....
}Hay 8 construcciones potencialmente peligrosas más como esa en MSBuild:
- V3095 El objeto 'propertyValue' se usó antes de que se verificara contra nulo. Verificar líneas:2760, 2799. Expander.cs 2760
- V3095 El objeto 'publicKeyToken' se usó antes de que se verificara contra nulo. Verifique las líneas:232, 236. GenerateBindingRedirects.cs 232
- V3095 El objeto 'searchLocation' se usó antes de que se verificara contra nulo. Verificar líneas:170, 178. Resolver.cs 170
- V3095 El objeto 'assemblyName' se usó antes de que se verificara contra nulo. Verificar líneas:176, 194. Resolver.cs 176
- V3095 El objeto 'searchLocation' se usó antes de que se verificara contra nulo. Verificar líneas:249, 264. Resolver.cs 249
- V3095 El objeto 'ReferenceInfo' se usó antes de que se verificara contra nulo. Verificar líneas:87, 97. AxReference.cs 87
- V3095 El objeto 'packageFileName' se usó antes de que se verificara contra nulo. Verificar líneas:1448, 1457. BootstrapperBuilder.cs 1448
- V3095 El objeto 'metadataNames' se usó antes de que se verificara contra nulo. Verifique las líneas:243, 253. CommandLineBuilderExtension.cs 243
Suposición incorrecta sobre la longitud de la cadena
Mensaje de diagnóstico de PVS-Studio :V3057 La función 'Subcadena' podría recibir el valor '-1' mientras se esperaba un valor no negativo. Inspeccione el segundo argumento. Utilidades.cs 328
Para el si bloque para ejecutar, debe haber una cadena que consta de uno o más caracteres, mientras que dentro de ese bloque, el programador intenta obtener una subcadena de la cadena original. Obviamente, el segundo parámetro de la Subcadena será negativo para una cadena de un carácter, por lo que el método arrojará una excepción ArgumentOutOfRangeException :
if (toolsVersionList.Length > 0)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}Así es como podría verse una versión segura de este código:
if (toolsVersionList.Length > 1)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}Otros errores similares:
- V3057 La función 'Subcadena' podría recibir el valor '-1' mientras se esperaba un valor no negativo. Inspeccione el segundo argumento. SolutionFile.cs 1217
- V3057 La función 'Subcadena' podría recibir el valor '-1' mientras se esperaba un valor no negativo. Inspeccione el segundo argumento. XMake.cs 2929
- V3057 La función 'Eliminar' podría recibir el valor '-1' mientras se esperaba un valor no negativo. Inspeccione el primer argumento. BootstrapperBuilder.cs 767
Conversión de tipo con pérdida de significado
Mensaje de diagnóstico de PVS-Studio :V3041 La expresión se transformó implícitamente del tipo 'largo' al tipo 'flotante'. Considere utilizar una conversión de tipos explícita para evitar la pérdida de una parte fraccionaria. Un ejemplo:doble A =(doble)(X) / Y;. CommunicationsUtilities.cs 593
Las variables ahora y s_lastLoggedTicks son de largo escribe. Participan en algunos cálculos que deberían arrojar un valor de flotante escribe. Sin embargo, dado que la operación de división se realiza sobre valores de tipo long y solo entonces el valor resultante se convierte en tipo float , resultará en la pérdida de precisión:
float millisecondsSinceLastLog =
(float)((now - s_lastLoggedTicks)/10000L);Código fijo:
float millisecondsSinceLastLog =
(float)(now - s_lastLoggedTicks)/10000;Siempre tenga cuidado con los cálculos en los que se usan juntos valores enteros y de punto flotante.
Método que siempre devuelve verdadero
Mensaje de diagnóstico de PVS-Studio :V3009 Es extraño que este método siempre devuelva el mismo valor de 'verdadero'. ComReference.cs 304
GetTypeLibNameForITypeLib método devuelve verdadero no importa cuáles sean las condiciones:
internal static bool GetTypeLibNameForITypeLib(....)
{
....
if (typeLib2 == null)
{
....
return true; // <=
}
....
try
{
if (data == null || ...)
{
....
return true; // <=
}
....
}
catch (COMException ex)
{
....
return true; // <=
}
return true; // <=
}Al mismo tiempo, el valor de tipo bool devuelto por GetTypeLibNameForITypeLib El método se verifica en la persona que llama. Los efectos de tal comportamiento son impredecibles. Este código necesita ser refactorizado y arreglado.
Comparación sin sentido
Mensaje de diagnóstico de PVS-Studio :V3022 La expresión 'itemsAndMetadataFound.Metadata.Values.Count> 0' siempre es verdadera. Evaluador.cs 1752
Después de itemsAndMetadataFound.Metadata.Values.Count> 0 expresión se evalúa en el si externo bloque, la misma comprobación, esta vez inútil, se hace dentro ese bloque:
if (itemsAndMetadataFound.Metadata != null &&
itemsAndMetadataFound.Metadata.Values.Count > 0)
{
....
if (itemsAndMetadataFound.Metadata.Values.Count > 0) // <=
{
needToProcessItemsIndividually = true;
}
....
}Además, MSBuild contiene 7 ediciones más de este tipo:
- V3022 La expresión 'fixedPathInfo !=null' siempre es verdadera. FrameworkLocationHelper.cs 794
- V3022 La expresión '_shutdownException !=null' siempre es falsa. InProcNode.cs 527
- V3022 La expresión 'proj !=null' siempre es verdadera. SolutionFile.cs 817
- V3022 La expresión '_directMetadata ==null' siempre es falsa. ProyectoItem.cs 755
- V3022 La expresión 'Constants.defaultToolsVersion =="2.0"' siempre es verdadera. ToolsetReader.cs 194
- V3022 La expresión '!isQuotedTransform &&functionCapture ==null' siempre es verdadera. ExpressionShredder.cs 281
- V3022 La expresión '!isQuotedTransform &&functionCapture ==null' siempre es verdadera. ExpressionShredder.cs 414
Comparaciones mutuamente excluyentes
Mensaje de diagnóstico de PVS-Studio :V3011 Se encontraron dos condiciones opuestas. La segunda condición es siempre falsa. Verificar líneas:2840, 2838. XMake.cs 2840
Para el si bloque a ejecutar, el registrador la variable debe tener el null valor. Sin embargo, esta variable se vuelve a probar para null dentro de ese bloque, en el VerifyThrow método. Entonces, esa segunda verificación siempre será falsa:
if (logger == null)
{
InitializationException.VerifyThrow(logger != null, // <=
"LoggerNotFoundError", unquotedParameter);
}No estoy seguro de cómo debería verse realmente este código, pero ciertamente no así. Quizás el si operador no es necesario en absoluto.
Argumentos no utilizados en los métodos de formato de cadena
Mensaje de diagnóstico de PVS-Studio :V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'WriteLine'. Argumentos no usados:1ro. Programador.cs 2216
El error está al acecho en la segunda línea. El programador debe haberlo escrito copiando la primera línea (el infame copiar y pegar) y cambiando el primer parámetro en el código copiado, pero se olvidó de eliminar el segundo parámetro, _schedulingData.EventTime.Ticks , que ya no era necesario:
file.WriteLine("Scheduler state at timestamp {0}:",
_schedulingData.EventTime.Ticks);
file.WriteLine("------------------------------------------------",
_schedulingData.EventTime.Ticks); // <=Entonces, el método WriteLine(formato de cadena, objeto arg0) se anula incorrectamente en la segunda línea.
Otros defectos similares:
- V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'Formato'. Argumentos no utilizados:recurso. XmlUtil.cs 75
- V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'Formato'. Argumentos no utilizados:recurso. XmlUtil.cs 82
- V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'Formato'. Argumentos no utilizados:recurso. XmlUtil.cs 91
- V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'Formato'. Argumentos no utilizados:recurso. XmlUtil.cs 112
Parámetro no utilizado
Mensaje de diagnóstico de PVS-Studio :V3061 El parámetro 'numericValue' siempre se reescribe en el cuerpo del método antes de usarse. NodePacketTranslator.cs 320
La lista de parámetros formales del método incluye la variable numericValue cuyo valor nunca se usa ya que se reemplaza inmediatamente con un nuevo valor:
public void TranslateEnum<T>(ref T value, int numericValue)
{
numericValue = _reader.ReadInt32(); // <=
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}Tal vez el programador hizo alguna refactorización, pero no fue posible cambiar la firma del método (a diferencia de su cuerpo). De lo contrario, es mejor arreglar el método:
public void TranslateEnum<T>(ref T value)
{
int numericValue = _reader.ReadInt32();
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}Otra advertencia similar:
- V3061 El parámetro 'defaultToolsVersion' siempre se reescribe en el cuerpo del método antes de usarse. ToolsetProvider.cs 118
Asignación redundante
Mensaje de diagnóstico de PVS-Studio :V3005 La variable '_nextProjectId' se asigna a sí misma. Servicio de registro.cs 325
El analizador detectó una construcción con una asignación adicional al campo _nextProjectId . El resultado del MaxCPUCount + 2 expresión se agrega al valor de _nextProjectId , y luego el valor resultante se asigna al mismo campo usando += operador. Después de eso, este valor se vuelve a asignar a _nextProjectId campo:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId = _nextProjectId += MaxCPUCount + 2; // <=
return _nextProjectId;
}
}
}No hay ningún error en este código, pero parece extraño. La construcción debe simplificarse:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId += MaxCPUCount + 2;
return _nextProjectId;
}
}
}Conclusión
Como conclusión, me gustaría decir que incluso proyectos de alta calidad como MSBuild podrían beneficiarse bastante de las comprobaciones periódicas de su código fuente en busca de errores potenciales y reales por parte de analizadores estáticos como PVS-Studio.
No dude en utilizar la versión de demostración del analizador PVS-Studio para comprobar este proyecto y echar un vistazo a las advertencias que hemos comentado, así como para comprobar sus propios proyectos.