¡Hola, todos ustedes, coleccionistas de bichos exóticos y simples! Hoy tenemos un espécimen raro en nuestro banco de pruebas PVS-Studio: un juego llamado "osu!", Escrito en C #. Como de costumbre, buscaremos errores, los analizaremos y jugaremos.
El juego
Osu! es un juego de ritmo de código abierto. Según el
sitio web del juego, es bastante popular, con más de 15 millones de cuentas de jugadores. El proyecto presenta jugabilidad gratuita, diseño colorido, personalización de mapas, un avanzado sistema de clasificación de jugadores en línea, modo multijugador y un rico conjunto de piezas musicales. No tiene sentido seguir profundizando en el juego; Puedes leer todo sobre esto en Internet. Comience con
esta página .
Estoy más interesado en el código fuente del proyecto, que está disponible en
GitHub . Una cosa que llama la atención de inmediato es la gran cantidad de confirmaciones de repositorio (más de 24 mil), que es una señal de desarrollo intenso y continuo (el juego se lanzó por primera vez en 2007, pero el trabajo debe haber comenzado incluso antes). Sin embargo, el proyecto no es grande: solo 1813 archivos .cs con un total de 135 mil LOC no vacíos. Este número también incluye pruebas, que generalmente no tengo en cuenta al ejecutar cheques. Las pruebas componen 306 de los archivos .cs con 25 mil LOC. El proyecto es realmente pequeño: por ejemplo, el núcleo C # de PVS-Studio tiene aproximadamente 300 mil LOC.
Dejando de lado los archivos de prueba, revisé 1507 archivos de 110 mil LOC de largo. El cheque reveló algunos errores interesantes, que estoy dispuesto a mostrarle.
Los bichos
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; .... }); }
Este es un buen ejemplo de programación orientada a copiar y pegar, que es un término humorístico utilizado recientemente por mi compañero de trabajo Valeriy Komarov en su artículo "
Los 10 errores principales encontrados en proyectos Java en 2019 ".
De todos modos, se ejecutan dos verificaciones idénticas en una fila. Uno de ellos probablemente estaba destinado a verificar alguna otra constante de la enumeración
HitResult :
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
¿Qué constante debía ser revisada? ¿O tal vez la segunda verificación no debería estar allí en absoluto? Estas son las preguntas que solo los autores pueden responder. De todos modos, este es un error que distorsiona la lógica de ejecución 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; .... }
Copiar y pegar nuevamente. Refactoré el código para que el error se note fácilmente ahora pero originalmente se había escrito en una línea. Al igual que en el ejemplo anterior, no puedo decir con certeza cómo se debe solucionar este. La enumeración
TournamentTypeface contiene solo una constante:
public enum TournamentTypeface { Aquatico }
Quizás el error se trata de verificar la variable
familiar dos veces, pero puedo estar equivocado.
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; }
Este método devuelve
falso cada vez. En casos como este, generalmente verificaría la llamada a la función, porque a menudo puede encontrar que la persona que llama no usa el valor de retorno, lo que significa que no hay problema (aparte del mal estilo). Así es como se ve la llamada en este caso:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Como puede ver, la persona que llama utiliza el valor devuelto por el método
OnPressed . Como ese valor siempre es
falso , la persona que llama siempre devuelve
falso también. Es muy probable que este código contenga un error y debe ser revisado.
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
La variable
val.NewValue se maneja de forma peligrosa en la condición del operador
?: . Lo que hace pensar al analizador es el hecho de que más adelante en la bifurcación, la misma variable se maneja de manera segura utilizando el operador 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 es más ambiguo, pero creo que también es un error. El programador crea un objeto de tipo
ActionableInfo . El campo
Acción se inicializa utilizando una función lambda, que maneja la
API de referencia potencialmente nula de una manera peligrosa. El analizador piensa que este patrón es un error porque la variable
api se maneja de manera segura más tarde, al inicializar el parámetro
Value . Llamé a este caso ambiguo porque el código en la función lambda implica una ejecución retrasada, en el momento en que el desarrollador podría garantizar de alguna manera que la referencia de la
API sería no nula. Pero no estoy seguro de eso porque el cuerpo de la función lambda no parece utilizar ningún manejo de referencia seguro, como verificaciones previas.
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 sospecha que los argumentos del método
Atan2 se pasan en el orden incorrecto. Esta es la declaración del método:
Los valores se pasaron en el orden inverso. No estoy seguro de si esto es un error porque el método
UpdateProgress contiene muchos cálculos no triviales; Solo lo menciono como un posible error.
V3080 [CWE-476] Posible desreferencia nula. Considere inspeccionar 'Beatmap'. WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
El analizador señala una posible desreferencia nula de
Beatmap :
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Bueno, el analizador es correcto.
Para obtener más información sobre cómo PVS-Studio detecta errores como este, y sobre las nuevas características agregadas en C # 8.0 que tienen que ver con el manejo de referencias potencialmente nulas, consulte el artículo "
Tipos de referencias 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); } .... }
Este es un error menor y bastante común. Los suscriptores pueden darse de baja del evento entre la verificación nula y la invocación del evento, lo que resulta en un bloqueo. Esta es una forma de corregir el error:
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(); }
La iteración sobre la colección de
columnas se realiza de forma peligrosa. El desarrollador asumió que la referencia de
columnas podría ser nula, lo que se indica mediante el uso del operador de acceso condicional para acceder a la colección más adelante en el código.
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 dice que es peligroso usar un evento anulado o virtual. Consulte la
descripción del diagnóstico para obtener una explicación. También escribí un artículo sobre este tema: "
Eventos virtuales en C #: algo salió mal ".
Aquí hay otra construcción insegura similar:
- 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, aquí hay un ejemplo sintético que demuestra la lógica original de este código:
x = (c * a) ?? b;
El error se debe al hecho de que la precedencia del operador "*" es mayor que la del "??" operador Así es como debería verse el código fijo (con paréntesis agregados):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Otro error similar:
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; } }
Como en el caso anterior, el programador tenía suposiciones erróneas sobre la precedencia de los operadores. La expresión original pasada al método
Math.Abs se evalúa de la siguiente manera:
(a – b) ?? 0
Así es como se debe solucionar:
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 cree que el código del controlador
OnPressed es inalcanzable a partir de la segunda instrucción
if . Esto se deduce del hecho de que la primera condición siempre es verdadera, es decir, que el método
base.OnPressed siempre devolverá
falso . Echemos un vistazo a la
base. Método
OnPressed :
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Y ahora en el 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 importa aquí porque la lógica del método
UpdateResult implica que la última declaración de
devolución es equivalente a lo siguiente:
return false;
Esto significa que el método
UpdateResult devolverá
false todo el tiempo, lo que provocará un problema de código inalcanzable anteriormente en 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 cree que la llamada al conjunto de reglas.
CreateInstance () no es segura. Antes de esta llamada, a la variable de
conjunto de reglas se le asigna un valor como resultado de llamar al método
GetRuleset :
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Como puede ver, la advertencia es válida ya que la secuencia de llamadas incluye el método
FirstOrDefault , que puede devolver
nulo .
Conclusión
No hay muchos errores en el código de "osu!", Y eso es bueno. Pero aún así recomiendo que los autores verifiquen los problemas informados por el analizador. Espero que esto ayude a mantener la alta calidad y el juego continuará brindando alegría a los jugadores.
Como recordatorio, PVS-Studio es una buena opción si le gusta jugar con el código fuente. El analizador está disponible para
descargar en el sitio web oficial. Otra cosa que me gustaría tener en cuenta es que las comprobaciones únicas como esta no tienen nada que ver con el uso normal del análisis estático en el proceso de desarrollo real. Es más efectivo solo cuando se usa regularmente tanto en el servidor de compilación como en las computadoras de los desarrolladores (esto se llama análisis incremental). Su objetivo final es evitar que los errores entren en el sistema de control de versiones al atraparlos en la etapa de codificación.
¡Buena suerte y mantente creativo!
Referencias
Este es nuestro primer artículo en 2020. Mientras lo hacemos, aquí están los enlaces a los controles de los proyectos de C # realizados durante el año pasado: