Este artículo revisa los resultados de una segunda verificación del proyecto Orchard con el analizador estático PVS-Studio. Orchard es un sistema de gestión de contenido de código abierto que se entrega como parte de la Galería de código abierto ASP.NET bajo la Fundación sin fines de lucro Outercurve. La verificación de hoy es especialmente interesante porque tanto el proyecto como el analizador han recorrido un largo camino desde la primera verificación, y esta vez veremos nuevos mensajes de diagnóstico y algunos errores agradables.
Sobre Orchard CMSVerificamos Orchard hace tres años. El analizador C # de PVS-Studio ha evolucionado enormemente desde entonces: hemos mejorado el análisis del flujo de datos, agregado análisis interprocedimiento y nuevos diagnósticos, y corregido una serie de falsos positivos. Más que eso, la segunda verificación reveló que los desarrolladores de Orchard habían reparado todos los errores reportados en el primer artículo, lo que significa que habíamos logrado nuestro objetivo, es decir, les habíamos mejorado su código.
Espero que también presten atención a este artículo y hagan las correcciones necesarias o, mejor aún, adopten PVS-Studio para su uso regular. Como recordatorio, ofrecemos a
los desarrolladores de
código abierto una licencia gratuita. Por cierto, hay otras
opciones que los proyectos propietarios también pueden disfrutar.
El código fuente de Orchard está disponible para descargar
aquí . La descripción completa del proyecto se encuentra
aquí . Si aún no tiene una copia de PVS-Studio, puede
descargar la versión de prueba. Utilicé PVS-Studio 7.05 Beta e incluiré algunas de sus advertencias en este artículo. Espero que esta revisión te convenza de que PVS-Studio es una herramienta útil. Solo tenga en cuenta que está destinado a ser utilizado
regularmente .
Resultados de analisisEstas son algunas de las cifras de la primera verificación de Orchard para que no tenga que cambiar entre los dos artículos para comparar.
Durante la verificación anterior, "hicimos el análisis de todos los archivos de código fuente (3739 elementos) con la extensión .cs. En total, había 214.564 líneas de código. El resultado del control fue 137 advertencias. Para ser más precisos, hubo 39 advertencias de primer nivel (alto). También hubo 60 advertencias de segundo nivel (medio) ".
La versión actual de Orchard está compuesta por 2.767 archivos .cs, es decir, es aproximadamente mil archivos más pequeños. La reducción y cambio de nombre del repositorio sugiere que los desarrolladores han aislado el núcleo del proyecto (
commit 966 ), que tiene 108,287 LOC de largo. El analizador emitió 153 advertencias: 33 de primer nivel y 70 de segundo nivel. Por lo general, no incluimos advertencias de tercer nivel, y voy a seguir con la tradición.
Mensaje de diagnóstico 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)); }
Comencemos con un error de recursión infinita, como hicimos en el primer artículo. Esta vez, las intenciones exactas del desarrollador no están claras, pero noté que el método
TryValidateModel tenía una versión sobrecargada con un parámetro:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Creo que, al igual que en el caso de la versión sobrecargada, el desarrollador pretendía llamar al método a través de
_updateModel. El compilador no notó el error;
_updateModel es de tipo
IUpdateModel , y la clase actual también implementa esta interfaz. Dado que el método no incluye ninguna verificación contra
StackOverflowException , probablemente nunca se llamó, aunque no contaría con eso. Si mi suposición es correcta, la versión fija debería verse así:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
Mensaje de diagnóstico 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 detectó dos asignaciones al
contenido de la variable local
. GetChildContentAsync es un método de biblioteca que se usa muy raramente para que nos tomemos la molestia de examinarlo y anotarlo. Entonces, me temo que ni nosotros ni el analizador sabemos nada sobre el objeto de retorno del método y los efectos secundarios. Pero sabemos con certeza que asignar el valor de retorno al
contenido no tiene sentido si no se usa más en el código. Quizás es solo una operación redundante en lugar de un error. No puedo decir cómo se debe solucionar esto, así que se lo dejo a los desarrolladores.
Mensaje de diagnóstico de PVS-Studio: V3080 Posible
anulación de referencia. 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; }
El analizador detectó una desreferencia insegura de
itemTag . Este fragmento es un buen ejemplo de cómo una herramienta de análisis estático es diferente de un desarrollador humano que realiza una revisión de código. El método tiene un parámetro llamado
ItemTag y una variable local llamada
itemTag . ¡No es necesario decirte que hace una gran diferencia para el compilador! Estas son dos variables diferentes, aunque relacionadas. La forma en que se relacionan es a través de una tercera variable,
itemTagName. Aquí está la secuencia de pasos que conducen a la posible excepción: si el argumento
ItemTag es igual a "-", no se asignará ningún valor a
itemTagName , por lo que seguirá siendo una referencia nula, y si es una referencia nula, entonces la variable local
itemTag se convertirá en una referencia nula también. En mi opinión, es mejor lanzar una excepción después de la verificación de 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; }
Mensaje de diagnóstico 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 detectó una desreferencia de
remoteClient seguido de una verificación nula un par de líneas más tarde. De hecho, esta es una potencial
NullReferenceException ya que el método
FirstOrDefault puede devolver un valor predeterminado (que es
nulo para los tipos de referencia). Supongo que este fragmento se puede solucionar simplemente moviendo el cheque hacia arriba para que sea anterior a la operación de desreferenciación:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
O tal vez debería solucionarse reemplazando
FirstOrDefault con
First y eliminando la comprobación por completo.
Advertencias de PVS-Studio 7.05 Beta:Por ahora, hemos anotado todos los métodos de
LINQ o
predeterminados . Esta información será utilizada por el nuevo diagnóstico en el que estamos trabajando: detecta casos en los que los valores devueltos por estos métodos se desreferencian sin una verificación previa. Cada método
orDefault tiene una contraparte que
genera una excepción si no se ha encontrado ningún elemento coincidente. Esta excepción será más útil para rastrear el problema que la excepción
NullReferenceException abstracta.
No puedo dejar de compartir los resultados que obtuve de este diagnóstico en el proyecto Orchard. Hay 27 puntos 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);
Mensaje de diagnóstico 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(....); } .... }
El analizador mencionó una desreferencia del valor devuelto por el método
CreateScope .
CreateScope es un método pequeño, así que aquí está su implementación completa:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Como puede ver, hay dos casos en los que puede devolver
nulo . El analizador no sabe a qué rama seguirá el flujo de ejecución, por lo que es seguro e informa que el código es sospechoso. Si tuviera que escribir un código como ese, escribiría un cheque nulo de inmediato.
Quizás mi opinión es parcial, pero creo que todos los métodos asincrónicos deberían estar protegidos de
NullReferenceException tanto como sea posible porque la depuración de este tipo de cosas está lejos de ser agradable.
En este caso particular, el método
CreateScope se llama cuatro veces: dos de esas llamadas van acompañadas de controles y las otras dos no. Las últimas dos llamadas (sin controles) parecen ser clones de copiar y pegar (misma clase, mismo método, la misma forma de desreferenciar el resultado para llamar a UsingAsync). La primera de esas dos llamadas se mostró arriba, y puede estar seguro de que la segunda activó la misma advertencia:
V3080 Posible desreferencia nula del valor de retorno del método. Considere inspeccionar: CreateScope (). SetupService.cs 192
Mensaje de diagnóstico 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); .... }
Ese es un error clásico de copiar y pegar.
ConsumerSecret se verificó dos veces, mientras que
AccessTokenSecret no se verificó en absoluto. Obviamente, esto se arregla de la siguiente manera:
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); .... }
Mensaje de diagnóstico 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 mayor claridad, aquí está la implementación completa de la clase (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 ....; } } }
Al analizador no le gustaron las dos ramas de
caso idénticas. De hecho, la clase tiene tres entidades, mientras que la instrucción switch solo devuelve dos de ellas. Si se pretende este comportamiento y la tercera entidad no está destinada a ser utilizada, el código puede mejorarse eliminando la tercera rama que los fusionó 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, el primero de los campos de retorno duplicados debe repararse de la siguiente manera:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
O debería ser la segunda rama del caso. No conozco los detalles del proyecto y, por lo tanto, no puedo determinar la correlación entre los nombres de los tipos y estrategias de operación.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Mensaje de diagnóstico de PVS-Studio: V3080 Posible
anulación de referencia. 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; .... }
A la variable de
solicitud se le asigna un valor diferente de
nulo varias veces en el primer bloque
if , pero cada vez con condiciones anidadas. La inclusión de todas esas condiciones haría que el ejemplo fuera demasiado largo, por lo que solo veremos las primeras, que verifican el tipo de método http
IsGet o
IsPost . La clase
Microsoft.AspNetCore.Http.HttpMethods tiene nueve métodos estáticos para verificar el tipo de consulta. Por lo tanto, pasar, por ejemplo, una consulta
Eliminar o
Establecer al método
ExecuteAsync conduciría a generar una
NullReferenceException . Incluso si dichos métodos no son compatibles actualmente, sería aconsejable agregar un cheque de excepción. Después de todo, los requisitos del sistema pueden cambiar. Aquí hay un ejemplo de tal verificación:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
Mensaje de diagnóstico 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 las advertencias del
V3080 son más convenientes para ver dentro del entorno de desarrollo porque necesita la navegación del método, el resaltado de tipo y la atmósfera amigable del IDE. Estoy tratando de reducir el texto de los ejemplos tanto como sea posible para mantenerlos legibles. Pero si no lo estoy haciendo bien o si quieres probar tu habilidad de programación y resolverlo por ti mismo, te recomiendo ver el resultado de este diagnóstico en cualquier proyecto de código abierto o simplemente en tu propio código.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
El analizador informa esta línea. Echemos un vistazo al método
Get :
public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
Llama a su versión sobrecargada. Vamos a verlo también:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }
Resulta que si obtenemos una entidad de un tipo incompatible con
JObject de
datos usando el indexador de
nombres , el método
Get devolverá
nulo . No estoy seguro de qué tan probable es eso porque estos tipos son de la biblioteca
Newtonsoft.Json , con la que no he trabajado mucho. Pero el autor del código sospechaba que el elemento buscado podría no existir, por lo que también debemos tenerlo en cuenta al acceder al resultado de la operación de lectura. Personalmente, tendría una excepción lanzada en el primer
Get si creemos que el nodo debe estar presente, o agregar una marca antes de la desreferencia si la inexistencia del nodo no cambia la lógica general (por ejemplo, obtenemos un valor predeterminado).
Solución 1:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Solución 2:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
Mensaje de diagnóstico de PVS-Studio: V3080 Posible
anulación de referencia. 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) { .... } .... }
Este es un ejemplo bastante simple en comparación con el anterior. El analizador sospecha que el método
QueryAsync podría devolver una referencia nula. Aquí está la implementación del método:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Dado que
GetQueryAsync es un método de interfaz, no puede estar seguro de cada implementación, especialmente si consideramos que el proyecto también incluye la siguiente versión:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
Las múltiples llamadas a funciones externas y accesos a la memoria caché dificultan el análisis de
GetDocumentAsync , así que digamos que la verificación es necesaria, sobre todo porque el método es asincrónico.
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 mencionar la alta calidad del código de Orchard! Es cierto que hubo otros defectos, que no discutí aquí, pero le mostré todos los errores más graves. Por supuesto, esto no quiere decir que verificar su código fuente una vez cada tres años sea suficiente. Obtendrá el máximo rendimiento del análisis estático si lo usa con
regularidad porque esta es la forma en que se garantiza que detectará y corregirá errores en las primeras etapas de desarrollo, donde la corrección de errores es más barata y fácil.
Aunque las comprobaciones únicas no ayudan mucho, todavía le recomiendo que descargue
PVS-Studio y lo pruebe en su proyecto: quién sabe, tal vez también encuentre algunos errores interesantes.