Damos la bienvenida a todos los amantes de exóticos y no muy errores en el código. Hoy en el banco de pruebas PVS-Studio es un invitado bastante raro: un juego en C #. A saber, osu! Como de costumbre: busca errores, piensa, juega.
El juego
Osu! - Un juego de ritmo musical de código abierto. A juzgar por la información
del sitio web del juego , es bastante popular, ya que se reclaman más de 15 millones de jugadores registrados. El proyecto se caracteriza por una jugabilidad gratuita, un diseño colorido con la capacidad de personalizar tarjetas, características avanzadas para compilar un ranking en línea de jugadores, la presencia de multijugador, un gran conjunto de composiciones musicales. No describiré el juego en detalle, los interesados encontrarán fácilmente toda la información en la red. Por ejemplo,
aquí .
Estoy más interesado en el código fuente del proyecto, que está disponible para descargar desde
GitHub . Un número significativo de confirmaciones (más de 24 mil) en el repositorio llama inmediatamente la atención, lo que indica el desarrollo activo del proyecto, que continúa hasta el día de hoy (el juego se lanzó en 2007, pero el trabajo probablemente comenzó antes). Al mismo tiempo, no hay mucho código fuente: 1813 archivos .cs que contienen 135 mil líneas de código, excluyendo las vacías. Este código contiene pruebas que generalmente no tomo en cuenta en los cheques. El código de prueba está contenido en 306 archivos .cs y, en consecuencia, 25 mil líneas de código, excluyendo las vacías. Este es un proyecto pequeño: a modo de comparación, el núcleo C # del analizador PVS-Studio contiene alrededor de 300 mil líneas de código.
En total, para verificar el juego en busca de errores, utilicé proyectos que no son de prueba que contienen 1507 archivos de código fuente y 110 mil líneas. Sin embargo, el resultado me complació parcialmente, ya que hubo varios errores interesantes de los que me apresuré a contarte.
Errores
V3001 Hay
subexpresiones idénticas 'result == HitResult.Perfect' a la izquierda y a la derecha de '||' operador DrawableHoldNote.cs 266
protected override void CheckForResult(....) { .... ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .... }); }
Un buen ejemplo de programación orientada a copiar y pegar. Un término cómico que mi colega Valery Komarov usó recientemente (presentó) en su artículo "
Los 10 errores principales en proyectos Java para 2019 ".
Entonces, dos controles idénticos siguen uno tras otro. Una de las comprobaciones probablemente debería contener otra
constante de enumeración
HitResult :
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
¿Qué constante en particular necesitaba ser utilizada, o la segunda verificación no es necesaria? Preguntas que solo el desarrollador puede responder. En cualquier caso, se ha cometido un error que distorsiona la lógica del programa.
V3001 Hay
subexpresiones idénticas 'family! = GetFamilyString (TournamentTypeface.Aquatico)' a la izquierda y a la derecha del operador '&&'. TournamentFont.cs 64
public static string GetWeightString(string family, FontWeight weight) { .... if (weight == FontWeight.Regular && family != GetFamilyString(TournamentTypeface.Aquatico) && family != GetFamilyString(TournamentTypeface.Aquatico)) weightString = string.Empty; .... }
Y de nuevo copiar y pegar. Formateé el código, por lo que el error se nota fácilmente. En la versión original, toda la condición se escribió en una línea. También es difícil decir cómo se puede arreglar el código. La enumeración
TournamentTypeface contiene una sola constante:
public enum TournamentTypeface { Aquatico }
La condición puede haber usado la variable
familiar dos veces por error, pero esto no es exacto.
V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'falso'. KeyCounterAction.cs 19
public bool OnPressed(T action, bool forwards) { if (!EqualityComparer<T>.Default.Equals(action, Action)) return false; IsLit = true; if (forwards) Increment(); return false; }
El método siempre devolverá
falso . Para tales errores, generalmente verifico el código de llamada, ya que a menudo simplemente no usan el valor de retorno en ningún lado, entonces no hay error (excepto el estilo de programación feo). En este caso, me encontré con el siguiente código:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Como puede ver, se utiliza el resultado devuelto por el método
OnPressed . Y como siempre es
falso , el resultado de llamar a
OnPressed también será siempre
falso . Creo que debería volver a verificar este código nuevamente, ya que hay una alta probabilidad de error.
Otro error similar:
- V3009 [CWE-393] Es extraño que este método siempre devuelva el mismo valor de 'falso'. KeyCounterAction.cs 30
V3042 [CWE-476] Posible NullReferenceException. El '?.' y '.' Los operadores se utilizan para acceder a los miembros del objeto 'val.NewValue' TournamentTeam.cs 41
public TournamentTeam() { Acronym.ValueChanged += val => { if (....) FlagName.Value = val.NewValue.Length >= 2
En la condición del operador
?: , La variable
val.NewValue no es segura. El analizador llegó a esa conclusión, ya que en la rama de entonces, se utiliza una opción segura para acceder a la variable a través de la
declaración de acceso condicional
val.NewValue? .Substring (....) .
Otro error similar:
- V3042 [CWE-476] Posible NullReferenceException. El '?.' y '.' Los operadores se utilizan para acceder a los miembros del objeto 'val.NewValue' TournamentTeam.cs 48
V3042 [CWE-476] Posible NullReferenceException. El '?.' y '.' Los operadores se utilizan para acceder a los miembros del objeto 'api' SetupScreen.cs 77
private void reload() { .... new ActionableInfo { Label = "Current User", ButtonText = "Change Login", Action = () => { api.Logout();
Este código es menos claro, pero creo que el error sigue ahí. Cree un objeto de tipo
ActionableInfo . El campo
Acción se inicializa con una lambda, en cuyo cuerpo no es seguro trabajar con una
API de referencia potencialmente nula. El analizador consideró que este patrón era un error, ya que, al inicializar el parámetro
Value , la variable
api funciona de manera segura. Llamé ambiguo al error, porque el código lambda supone una ejecución retrasada y luego, tal vez, el desarrollador de alguna manera garantiza un valor distinto de cero del enlace
api . Pero esto es solo una suposición, ya que el cuerpo lambda no contiene ningún signo de trabajo seguro con el enlace (verificaciones preliminares, por ejemplo).
V3066 [CWE-683] Posible orden incorrecto de argumentos pasados al método 'Atan2': 'diff.X' y 'diff.Y'. SliderBall.cs 182
public void UpdateProgress(double completionProgress) { .... Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI); .... }
El analizador sospechaba que cuando trabajaba con el método
Atan2 de la clase
Math , el desarrollador confundía el orden de los argumentos.
Anuncio de Atan2 :
Como puede ver, los valores se transfirieron en orden inverso. No puedo juzgar si esto es un error, ya que el método
UpdateProgress contiene muchos cálculos no triviales. Solo tenga en cuenta el hecho de un posible problema en el código.
V3080 [CWE-476] Posible desreferencia nula. Considere inspeccionar 'Beatmap'. WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
El analizador señaló el peligro de acceso a través del enlace nulo
Beatmap . Esto es lo que es:
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Bueno, el analizador tiene razón.
Para obtener más información acerca de cómo PVS-Studio encuentra dichos errores, así como sobre las innovaciones de C # 8.0 relacionadas con temas similares (trabajar con referencias potencialmente nulas), consulte el artículo "
Tipos de referencia anulables en C # 8.0 y análisis estático ".
V3083 [CWE-367] Invocación insegura del evento 'ObjectConverted', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. BeatmapConverter.cs 82
private List<T> convertHitObjects(....) { .... if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .... }
Error no crítico y bastante común. Entre la comprobación del evento para
nulo y su invocación, pueden darse de baja del evento, lo que provocará el bloqueo del programa.
Una de las soluciones:
private List<T> convertHitObjects(....) { .... converted = converted.ToList(); ObjectConverted?.Invoke(obj, converted); .... }
V3095 [CWE-476] El objeto 'columnas' se usó antes de que se verificara como nulo. Líneas de verificación: 141, 142. SquareGraph.cs 141
private void redrawProgress() { for (int i = 0; i < ColumnCount; i++) columns[i].State = i <= progress ? ColumnState.Lit : ColumnState.Dimmed; columns?.ForceRedraw(); }
No es seguro omitir la colección de
columnas en un bucle. Al mismo tiempo, el desarrollador asumió que el enlace de las
columnas podría ser nulo, ya que más adelante en el código, se utiliza un operador de acceso condicional para acceder a la colección.
V3119 Llamar al evento anulado 'OnNewResult' puede conducir a un comportamiento impredecible. Considere implementar accesores de eventos explícitamente o use una palabra clave 'sellada'. DrawableRuleset.cs 256
private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult;
El analizador advierte sobre los peligros de usar un evento anulado o virtual. ¿Cuál es exactamente el peligro? Sugiero leer la
descripción del diagnóstico. Además, en un momento escribí un artículo sobre este tema, "
Eventos virtuales en C #: Algo salió mal ".
Otra construcción insegura similar en el código:
- V3119 Llamar a un evento anulado puede conducir a un comportamiento impredecible. Considere implementar accesores de eventos explícitamente o use una palabra clave 'sellada' DrawableRuleset.cs 257
V3123 [CWE-783] Quizás el '??' El operador funciona de una manera diferente a la esperada. Su prioridad es inferior a la prioridad de otros operadores en su parte izquierda. OsuScreenStack.cs 45
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; }
Para una mejor comprensión del problema, daré un ejemplo sintético de cómo funciona el código ahora:
x = (c * a) ?? b;
El error se cometió debido al hecho de que el operador * tiene mayor prioridad que el operador ??. Versión corregida del código (corchetes agregados):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Otro error similar en el código:
V3123 [CWE-783] Quizás el '??' El operador funciona de una manera diferente a la esperada. Su prioridad es inferior a la prioridad de otros operadores en su parte izquierda. FramedReplayInputHandler.cs 103
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan; } }
Aquí, como en el fragmento de código anterior, no se tuvo en cuenta la prioridad de los operadores. Ahora la expresión pasada al método
Math.Abs se evalúa de la siguiente manera:
(a – b) ?? 0
Código corregido:
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= AllowedImportantTimeSpan; } }
V3142 [CWE-561] Código inalcanzable detectado. Es posible que haya un error presente. DrawableHoldNote.cs 214
public override bool OnPressed(ManiaAction action) { if (!base.OnPressed(action)) return false; if (Result.Type == HitResult.Miss)
El analizador afirma que el
código del controlador
OnPressed , que comienza con la segunda
instrucción if , es inalcanzable. Esto se deduce de la suposición de que la primera condición siempre es verdadera, es decir, la
base. El método
OnPressed siempre devolverá
falso .
Eche un vistazo a la
base. Método
presionado :
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Procedemos al método
UpdateResult :
protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; }
Tenga en cuenta que la implementación de la propiedad
Judged no es importante aquí, ya que la lógica del método
UpdateResult implica que la última
declaración de retorno es equivalente a esto:
return false;
Por lo tanto, el método
UpdateResult siempre devolverá
falso , lo que conducirá a un error con código inalcanzable en el código sobre la pila.
V3146 [CWE-476] Posible desreferencia nula del 'conjunto de reglas'. 'FirstOrDefault' puede devolver un valor nulo predeterminado. APILegacyScoreInfo.cs 24
public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance()
El analizador considera que la llamada al conjunto de
reglas.CreateInstance () no es segura. La variable de
conjunto de reglas obtiene previamente el valor como resultado de llamar a
GetRuleset :
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Como puede ver, la advertencia del analizador está justificada, ya que la cadena de llamadas contiene
FirstOrDefault , que puede devolver
nulo .
Conclusión
En general, el proyecto del juego "osu!" Satisfecho con un pequeño número de errores. Sin embargo, recomiendo a los desarrolladores que presten atención a los problemas detectados. Y deja que el juego siga complaciendo a sus fanáticos.
Y para aquellos a quienes les gusta profundizar en el código, les recuerdo que el analizador PVS-Studio, que es fácil de
descargar desde el sitio oficial, será de gran ayuda. También noto que las comprobaciones de proyectos únicas, como las descritas anteriormente, no tienen nada que ver con el uso de un analizador estático en el trabajo real. La máxima eficiencia en la lucha contra los errores solo se puede lograr con el uso regular de la herramienta tanto en el servidor de compilación como directamente en la computadora del desarrollador (análisis incremental). El objetivo máximo es evitar que los errores ingresen al sistema de control de versiones, reparando defectos que ya están en la etapa de escritura del código.
¡Buena suerte y éxito!
Referencias
Esta es la primera publicación en 2020. Aprovechando esta oportunidad, proporcionaré enlaces a artículos sobre la verificación de proyectos de C # el año pasado:
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Sergey Khrenov.
Juega "osu!", Pero ten cuidado con los errores .