Este artículo es el resultado de volver a verificar el proyecto Orchard utilizando el analizador estático PVS-Studio. Orchard es un sistema de gestión de contenido de código abierto que forma parte de Outercurve Foundation, una galería de proyectos ASP.NET sin fines de lucro. La prueba es interesante porque el proyecto y el analizador han crecido significativamente desde el primer análisis. Nuevos positivos y errores interesantes te están esperando.
Sobre Orchard CMSProbamos Orchard con PVS-Studio hace tres años. Desde entonces, el analizador C # se ha desarrollado enormemente: mejoramos el análisis del flujo de datos, desarrollamos el análisis interprocedial, agregamos nuevos diagnósticos y reparamos una serie de falsos positivos. Además, el análisis mostró que todos los comentarios del artículo anterior fueron corregidos. Esto significa que se logra el objetivo: el código ha mejorado.
Quiero creer que los desarrolladores prestarán atención a este artículo y harán los cambios necesarios. Aún mejor, si introduce la práctica del uso regular de PVS-Studio. Permítame recordarle que para
proyectos de código abierto proporcionamos una versión gratuita de la licencia. Por cierto, hay otras
opciones que son adecuadas para proyectos cerrados.
El código del proyecto Orchard se puede descargar
desde aquí , como lo hice yo. Una descripción completa del proyecto se puede encontrar
aquí . Si aún no tiene nuestro analizador PVS-Studio, puede descargar una versión de prueba
desde aquí . Usé la versión de PVS-Studio versión 7.05 Beta. Compartiré los resultados de su trabajo. Espero que esté de acuerdo con la utilidad de usar PVS-Studio. Lo principal es usar el analizador
regularmente .
Resultados de validaciónTe daré algunos números del último artículo para que no tengas que cambiar para comparar.
“Todos los archivos (3739 piezas) con la extensión .cs participaron en la última verificación. En total, contenían 214.564 líneas de código. Según los resultados de la auditoría, se recibieron 137 advertencias. En el primer nivel (alto) de confianza, se recibieron 39 advertencias. En el segundo nivel (promedio), se recibieron 60 advertencias ".
Por el momento, el proyecto tiene 2767 archivos .cs, es decir, el proyecto se ha convertido en menos de mil archivos. A juzgar por esta disminución en el número de archivos y el cambio del nombre del repositorio, se asignó un núcleo del proyecto (
commit 966 ). Hay 108.287 líneas de código en el núcleo y el analizador emitió 153 advertencias, 33 en un nivel alto, 70 en promedio. Por lo general, no consideramos advertencias del tercer nivel, no rompí la tradición.
Advertencia de PVS-Studio :
V3110 Posible recursión infinita dentro del método 'TryValidateModel'. PrefixedModuleUpdater.cs 48
public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); }
Como en el artículo anterior, abrimos la lista de errores con recursión infinita. Es difícil decir qué quería hacer exactamente el desarrollador en este caso. Pero noté que el método
TryValidateModel tiene una sobrecarga de argumento único que se ve así:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Supongo que, como con el método sobrecargado, el desarrollador quería llamar a los métodos a través de
_updateModel. El compilador no vio un error,
_updateModel es del tipo
IUpdateModel y la clase actual también implementa esta interfaz. Dado que no hay una sola condición en el método para proteger contra una
StackOverflowException , es posible que no se haya llamado al método ni una sola vez, pero no contaría con él. Si la suposición es correcta, la versión corregida se verá así:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
Advertencia de PVS-Studio: V3008 A la variable 'contenido' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 197, 190. DynamicCacheTagHelper.cs 197
public override async Task ProcessAsync(....) { .... IHtmlContent content; .... try { content = await output.GetChildContentAsync(); } finally { _cacheScopeManager.ExitScope(); } content = await ProcessContentAsync(output, cacheContext); .... }
El analizador vio dos asignaciones al
contenido de la variable local
. GetChildContentAsync es un método de la biblioteca que no es lo suficientemente común como para que podamos decidirlo y anotarlo. Entonces, desafortunadamente, ni nosotros ni el analizador sabemos nada sobre el objeto devuelto del método y los efectos secundarios. Pero definitivamente podemos decir que asignar un resultado en el
contenido no tiene sentido sin uso. Quizás esto no sea un error en absoluto, solo una operación adicional. No pude llegar a una conclusión inequívoca sobre la forma de solucionarlo. Dejaré esta decisión a los desarrolladores.
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula. Considere inspeccionar 'itemTag'. CoreShapes.cs 92
public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = String.IsNullOrEmpty(itemTagName) ? null : ....; .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }
En este código, el analizador detectó una
etiqueta de elemento de desreferencia peligrosa. Un buen ejemplo de la diferencia entre un analizador estático y una persona que realiza pruebas. El método tiene un parámetro llamado
ItemTag y una variable local llamada
itemTag . Como sabes, ¡la diferencia es enorme para el compilador! Estas son dos variables diferentes, aunque relacionadas. La conexión pasa por la tercera variable,
itemTagName. El escenario para lanzar una excepción es este: el argumento
ItemTag es "-", el
itemTagName no tiene asignado un valor, seguirá siendo una referencia nula, y si es una referencia nula, el
itemTag local también se convertirá en una referencia nula. Creo que es mejor lanzar una excepción aquí después de verificar la cadena.
public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = ....; if(String.IsNullOrEmpty(itemTag)) throw .... .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }
Advertencia de PVS-Studio: V3095 El objeto 'remoteClient' se usó antes de que se verificara como nulo. Verifique las líneas: 49, 51. ImportRemoteInstanceController.cs 49
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); var apiKey = Encoding.UTF8.GetString(....(remoteClient.ProtectedApiKey)); if (remoteClient == null || ....) { .... } .... }
El analizador vio la desreferenciación del
RemoteClient y la comprobación de
null a continuación. Esto es realmente una potencial
NullReferenceException , porque el método
FirstOrDefault puede devolver el valor predeterminado (
nulo para los tipos de referencia). Supongo que, para arreglar el código, simplemente transfiera el cheque anterior:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
Sin embargo, podría valer la pena reemplazar
FirstOrDefault con
First y eliminar la verificación por completo.
Desde PVS-Studio 7.05 Beta:Por el momento, hemos anotado todos los métodos
orDefault de
LINQ . Esta información será utilizada por nuevos diagnósticos, notando la desreferenciación del resultado de llamar a estos métodos sin verificar. Para cada método
orDefault hay un análogo que
genera una excepción si no se encuentra un elemento adecuado. Esta excepción ayudará más a comprender el problema que la excepción
NullReferenceException abstracta.
No puedo dejar de compartir el resultado de los diagnósticos desarrollados en este proyecto. 27 lugares potencialmente peligrosos. Aquí hay algunos de ellos:
ContentTypesAdminNodeNavigationBuilder.cs 71:
var treeBuilder = treeNodeBuilders.Where(....).FirstOrDefault(); await treeBuilder.BuildNavigationAsync(childNode, builder, treeNodeBuilders);
ListPartDisplayDriver.cs 217:
var contentTypePartDefinition = ....Parts.FirstOrDefault(....); return contentTypePartDefinition.Settings....;
ContentTypesAdminNodeNavigationBuilder.cs 113:
var typeEntry = node.ContentTypes.Where(....).FirstOrDefault(); return AddPrefixToClasses(typeEntry.IconClass);
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula del valor de retorno del método. Considere inspeccionar: CreateScope (). SetupService.cs 136
public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... }
Entonces, el analizador observó la desreferenciación del resultado de llamar al método
CreateScope . El método
CreateScope es muy pequeño, lo daré en su totalidad:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Como puede ver, puede devolver
nulo en dos casos. El analizador no puede decir por qué rama pasará el programa, por lo tanto, marca el código como sospechoso. En mi código, inmediatamente agregaría un cheque
nulo .
Tal vez juzgue parcial, pero todos los métodos asincrónicos deben estar asegurados contra
NullReferenceException lo mejor posible. La depuración de tales cosas es un placer muy dudoso.
En este caso, el método
CreateScope tiene cuatro llamadas, y en dos hay una comprobación, pero en las otras dos falta. Además, un par sin verificación es similar a copiar y pegar (una clase, un método, desreferenciado para llamar a UsingAsync). Ya hice la primera llamada de este par y, por supuesto, la segunda llamada también tiene una advertencia de analizador similar:
V3080 Posible desreferencia nula del valor de retorno del método. Considere inspeccionar: CreateScope (). SetupService.cs 192
Advertencia de PVS-Studio: V3127 Se
encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'AccessTokenSecret' en lugar de 'ConsumerSecret' TwitterClientMessageHandler.cs 52
public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }
Clásico copiar y pegar.
ConsumerSecret se verificó dos veces y
AccessTokenSecret , no una. Obviamente, necesitas arreglar:
public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.AccessTokenSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }
Advertencia de PVS-Studio: V3139 Dos o más ramificaciones de casos realizan las mismas acciones. SerialDocumentExecuter.cs 23
Otro error de copiar y pegar. Para que quede más claro, daré toda la clase, ya que es pequeña.
public class SerialDocumentExecuter : DocumentExecuter { private static IExecutionStrategy ParallelExecutionStrategy = new ParallelExecutionStrategy(); private static IExecutionStrategy SerialExecutionStrategy = new SerialExecutionStrategy(); private static IExecutionStrategy SubscriptionExecutionStrategy = new SubscriptionExecutionStrategy(); protected override IExecutionStrategy SelectExecutionStrategy(....) { switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } } }
El analizador consideró dos ramas de
caso idénticas sospechosas. De hecho, hay tres entidades en la clase, dos regresan cuando circulan, una no. Si esto está planeado y se abandona la tercera opción, puede eliminarla combinando las dos ramas de la siguiente manera:
switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Si se trata de un error de copiar y pegar, debe corregir el campo devuelto de esta manera:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
O viceversa. No estoy familiarizado con el proyecto y no puedo correlacionar los nombres de los tipos de operaciones y estrategias.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula. Considere inspeccionar 'solicitud'. GraphQLMiddleware.cs 148
private async Task ExecuteAsync(HttpContext context....) { .... GraphQLRequest request = null; .... if (HttpMethods.IsPost(context.Request.Method)) { .... } else if (HttpMethods.IsGet(context.Request.Method)) { .... request = new GraphQLRequest(); .... } var queryToExecute = request.Query; .... }
En el primer bloque
if , la variable de
solicitud obtiene un valor distinto de
nulo en varios lugares, pero en todas partes con condiciones anidadas. No cité todas estas condiciones, ya que el ejemplo habría sido demasiado engorroso. Las primeras condiciones que comprueban el tipo http del método
IsGet o
IsPost son suficientes . La clase
Microsoft.AspNetCore.Http.HttpMethods tiene nueve métodos estáticos para verificar el tipo de solicitud. Esto significa que si, por ejemplo, una solicitud
Eliminar o
Establecer ingresa al método
ExecuteAsync , se
generará una
NullReferenceException . Incluso si en este momento estos métodos no son compatibles, es mejor hacer una verificación con una excepción. Después de todo, los requisitos para el sistema pueden cambiar. Ejemplo de verificación a continuación:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula del valor de retorno del método. Considere inspeccionar: Obtenga <ContentPart> (...). ContentPartHandlerCoordinator.cs 190
La
mayoría de los
desencadenadores de diagnóstico
V3080 son más fáciles de ver en el entorno de desarrollo. Necesita navegación de método, resaltado de tipo, una atmósfera IDE alentadora. Trato de mantener los ejemplos lo más cortos posible para que se sienta más cómodo leyendo. Pero si no me funciona, o si desea verificar su nivel de programación y resolverlo por su cuenta, le aconsejo que observe cómo funciona este diagnóstico en cualquier proyecto abierto o propio.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
El analizador jura en esta línea. Veamos el método
Get: public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
Causa su sobrecarga. Miremosla:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }
Resulta que si al recibir un elemento de
Datos por el indexador de
nombres obtenemos una entidad de un tipo incompatible con
JObject , entonces el método
Get devolverá
nulo . No me aventuraré a juzgar la probabilidad de esto, ya que estos tipos son de la biblioteca
Newtonsoft.Json , y tengo un poco de experiencia con eso. Pero el desarrollador sospechó que el artículo deseado podría no serlo. Por lo tanto, no se olvide de esta posibilidad cuando se refiera a los resultados de lectura. Agregaría un lanzamiento de excepción al primer
Get si pensamos que el nodo debería ser, o verifique antes de desreferenciar, si la ausencia del nodo no cambia la lógica general (se toma el valor predeterminado, por ejemplo).
Opcion uno:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Opción dos:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula. Considere inspeccionar los "resultados". ContentQueryOrchardRazorHelperExtensions.cs 19
public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... }
Un ejemplo bastante simple en relación con el anterior. El analizador considera que el resultado de llamar a
QueryAsync puede ser una referencia nula. Aquí está el método:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Aquí
GetQueryAsync es un método de interfaz, no puede estar seguro de cada implementación. Además, en el mismo proyecto existe tal opción:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
El análisis del método
GetDocumentAsync se complica por la abundancia de llamadas a funciones externas y accesos a caché. Detengámonos en el hecho de que vale la pena agregar la verificación. Además, el método es asíncrono.
public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... }
Conclusión¡No puedo dejar de notar la alta calidad del código! Sí, hubo otras deficiencias que no mencioné en este artículo, pero se consideraron las más graves. Por supuesto, esto no significa que un cheque cada tres años sea suficiente. El beneficio máximo se logra con el uso
regular de un analizador estático, ya que es este enfoque el que le permite detectar y solucionar problemas en las primeras etapas, cuando el costo y la complejidad de la edición son mínimos.
Aunque las comprobaciones únicas no son tan efectivas como sea posible, le insto a que descargue y pruebe
PVS-Studio en su proyecto. ¿Qué sucede si puede encontrar algo interesante?
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Alexander Senichkin.
Escaneando el código de Orchard CMS en busca de errores .