Приветствуем всех любителей экзотических и не очень ошибок кода.
Сегодня у нас на тестовом стенде PVS-Studio довольно редкий гость — игра на языке C#.
А именно – «осу!» Как обычно: ищем ошибки, думаем, играем.
Игра
Осу! — это музыкальная ритм-игра с открытым исходным кодом.Судя по информации из игровой сайт – довольно популярен, так как зарегистрировано более 15 миллионов игроков.
Проект отличается свободным геймплеем, красочным оформлением с возможностью настройки карт, расширенными возможностями составления онлайн-рейтингов игроков, наличием мультиплеера и большим выбором музыкальных композиций.
Подробно описывать игру не буду; желающие легко найдут всю информацию в Интернете.
Например, здесь .
Меня больше интересует исходный код проекта, который доступен для скачивания с сайта GitHub .
Значительное количество коммитов (более 24 тысяч) в репозиторий сразу обращает на себя внимание, что говорит об активном развитии проекта, которое продолжается и по сей день (игра вышла в 2007 году, но работа, вероятно, началась раньше).
При этом исходного кода немного — 1813 файлов .
cs, содержащих 135 тысяч строк кода, не считая пустых.
В этом коде есть тесты, которые я обычно не рассматриваю в проверках.
Код теста содержится в 306 файлах .
cs и соответственно 25 тысячах строк кода, не считая пустых.
Это небольшой проект: для сравнения, C#-ядро анализатора PVS-Studio содержит около 300 тысяч строк кода.
Всего для проверки игры на ошибки я использовал нетестовые проекты, содержащие 1507 файлов исходного кода и 110 тысяч строк.
Однако результат меня несколько порадовал, так как было несколько интересных ошибок, о которых спешу вам рассказать.
Ошибки
В3001 Слева и справа от '||' имеются идентичные подвыражения 'result == HitResult.Perfect'.оператор.
DrawableHoldNote.cs 266
Хороший пример программирования, ориентированного на копирование и вставку.protected override void CheckForResult(.) { .
ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .
}); }
Юмористический термин, который недавно использовал (ввел) мой коллега Валерий Комаров в своей статье " Топ-10 ошибок в Java-проектах за 2019 год ".
Итак, две одинаковые проверки следуют одна за другой.
Одна из проверок, скорее всего, должна содержать другую константу перечисления.
Результат попадания : public enum HitResult
{
None,
Miss,
Meh,
Ok,
Good,
Great,
Perfect,
}
Какую константу надо было использовать или вторая проверка вообще не нужна? Вопросы, на которые может ответить только разработчик.
В любом случае была допущена ошибка, искажающая логику работы программы.
В3001 Слева и справа от оператора «&&» находятся идентичные подвыражения «family != GetFamilyString(TournamentTypeface.Aquatico)».
Турнирный шрифт.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; .
}
И снова копипаст. Я отформатировал код так, чтобы ошибку было легко увидеть.
В оригинальной версии все условие записывалось в одну строку.
Также сложно сказать, как можно исправить код. Передача ТурнирШрифт содержит одну константу: public enum TournamentTypeface
{
Aquatico
}
В условии дважды по ошибке использовалась переменная.
семья , но это не совсем так.
В3009 [CWE-393] Странно, что этот метод всегда возвращает одно и то же значение false. 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;
}
Метод всегда будет возвращать ЛОЖЬ .
На наличие таких ошибок я обычно проверяю вызывающий код, так как часто просто нигде не используют возвращаемое значение, то и ошибки нет (кроме некрасивого стиля программирования).
В данном случае я наткнулся на следующий код: public bool OnPressed(T action) =>
Target.Children
.
OfType<KeyCounterAction<T>>() .
Any(c => c.OnPressed(action, Clock.Rate >= 0));
Как видите, результат, возвращаемый методом OnPressed , используется.
И поскольку он всегда ЛОЖЬ , то результат вызывающего абонента OnPressed тоже всегда будет ЛОЖЬ .
Думаю, вам стоит еще раз перепроверить этот код, так как велика вероятность ошибки.
Еще одна похожая ошибка:
- V3009 [CWE-393] Странно, что этот метод всегда возвращает одно и то же значение false. KeyCounterAction.cs 30
' и '.
' операторы используются для доступа к членам объекта val.NewValue TournamentTeam.cs 41 public TournamentTeam()
{
Acronym.ValueChanged += val =>
{
if (.
) FlagName.Value = val.NewValue.Length >= 2 // <= ? val.NewValue?.
Substring(0, 2).
ToUpper() : string.Empty; }; .
}
В состоянии оператора ?: с переменной val.NewValue работать небезопасно.
Анализатор сделал такой вывод, потому что далее в then-ветви для доступа к переменной используют безопасный вариант работы через оператор условного доступа.
val.NewValue?.
Подстрока(.
) .
Еще одна похожая ошибка:
- V3042 [CWE-476] Возможное исключение NullReferenceException. '?.
' и '.
' операторы используются для доступа к членам объекта val.NewValue TournamentTeam.cs 48
' и '.
' операторы используются для доступа к членам объекта API SetupScreen.cs 77 private void reload()
{
.
new ActionableInfo { Label = "Current User", ButtonText = "Change Login", Action = () => { api.Logout(); // <= .
}, Value = api?.
LocalUser.Value.Username, .
}, .
} private class ActionableInfo : LabelledDrawable<Drawable> { .
public Action Action; .
}
Этот код менее понятен, но я думаю, что здесь все же есть ошибка.
Создайте объект типа ActionableInfo .
Поле Действие инициализируется лямбдой, в теле которой небезопасно работать с потенциально нулевой ссылкой API .
Анализатор посчитал этот паттерн ошибкой, так как в дальнейшем при инициализации параметра Ценить с переменной API работать безопасно.
Ошибку я назвал неоднозначной, поскольку лямбда-код предполагает отложенное выполнение и тогда, возможно, разработчик каким-то образом гарантирует ненулевое значение ссылки API .
Но это лишь предположение, так как тело лямбды не содержит никаких признаков безопасной работы со ссылкой (предварительные проверки, например).
В3066 [CWE-683] Возможно, неправильный порядок аргументов, передаваемых в метод Atan2: «diff.X» и «diff.Y».
SliderBall.cs 182 public void UpdateProgress(double completionProgress)
{
.
Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI); .
}
Анализатор заподозрил, что при работе с методом Атан2 сорт Математика Разработчик перепутал порядок аргументов.
Объявление Атан2 : // Parameters:
// y:
// The y coordinate of a point.
//
// x:
// The x coordinate of a point.
public static double Atan2(double y, double x);
Как видите, значения были переданы в обратном порядке.
Не берусь судить, ошибка ли это, поскольку метод ОбновлениеПрогресс содержит довольно много нетривиальных вычислений.
Просто отмечу факт возможной проблемы в коде.
В3080 [CWE-476] Возможное разыменование нуля.
Рассмотрите возможность проверки «Beatmap».
РабочийBeatmap.cs 57 protected virtual Track GetVirtualTrack()
{
.
var lastObject = Beatmap.HitObjects.LastOrDefault(); .
}
Анализатор указал на опасность доступа по нулевой ссылке Карта битов .
Вот что это такое: public IBeatmap Beatmap
{
get
{
try
{
return LoadBeatmapAsync().
Result;
}
catch (TaskCanceledException)
{
return null;
}
}
}
Что ж, анализатор прав.
Подробнее о том, как PVS-Studio находит подобные ошибки, а также о нововведениях в C# 8.0, связанных с подобной тематикой (работа с потенциально нулевыми ссылками), можно прочитать в статье " Ссылочные типы, допускающие значение NULL, в C# 8.0 и статический анализ ".
В3083 [CWE-367] Возможен небезопасный вызов события ObjectConverted, NullReferenceException. Рассмотрите возможность присвоения события локальной переменной перед ее вызовом.
BeatmapConverter.cs 82 private List<T> convertHitObjects(.
) { .
if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .
}
Некритичная и довольно распространенная ошибка.
Между проверкой события на равенство нулевой и при его вызове событие может быть отписано, что приведет к сбою программы.
Одно из возможных исправлений: private List<T> convertHitObjects(.
) { .
converted = converted.ToList(); ObjectConverted?.
Invoke(obj, converted); .
}
В3095 [CWE-476] Объект «столбцы» использовался до того, как было проверено его соответствие нулю.
Проверить строки: 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();
}
Обход коллекции столбцы цикл производится небезопасно.
При этом разработчик предположил, что ссылка столбцы может быть нулевым, поскольку далее в коде для доступа к коллекции используется оператор условного доступа.
В3119 Вызов переопределенного события OnNewResult может привести к непредсказуемому поведению.
Рассмотрите возможность явной реализации методов доступа к событиям или используйте ключевое слово «sealed».
DrawableRuleset.cs 256 private void addHitObject(TObject hitObject)
{
.
drawableObject.OnNewResult += (_, r) => OnNewResult?.
Invoke(r); .
}
public override event Action<JudgementResult> OnNewResult;
Анализатор предупреждает об опасности использования переопределенного или виртуального события.
В чем именно заключается опасность – предлагаю прочитать в описание к диагностике.
Также в свое время я написал статью на эту тему» Виртуальные события в C#: что-то пошло не так ".
Еще одна похожая небезопасная конструкция в коде:
- V3119 Вызов переопределенного события может привести к непредсказуемому поведению.
Рассмотрите возможность явной реализации методов доступа к событиям или используйте ключевое слово «sealed».
DrawableRuleset.cs 257
Его приоритет ниже приоритета других операторов в левой части.
OsuScreenStack.cs 45 private void onScreenChange(IScreen prev, IScreen next)
{
parallaxContainer.ParallaxAmount =
ParallaxContainer.DEFAULT_PARALLAX_AMOUNT *
((IOsuScreen)next)?.
BackgroundParallaxAmount ?? 1.0f;
}
Чтобы лучше понять проблему, приведу синтетический пример того, как работает код на данный момент: x = (c * a) ?? b;
Ошибка произошла потому, что оператор "*" имеет более высокий приоритет, чем оператор "??" оператор.
Исправленная версия кода (добавлены скобки): private void onScreenChange(IScreen prev, IScreen next)
{
parallaxContainer.ParallaxAmount =
ParallaxContainer.DEFAULT_PARALLAX_AMOUNT *
(((IOsuScreen)next)?.
BackgroundParallaxAmount ?? 1.0f);
}
Еще одна аналогичная ошибка в коде: В3123 [CWE-783] Возможно, '??' оператор работает не так, как ожидалось.
Его приоритет ниже приоритета других операторов в левой части.
FramedReplayInputHandler.cs 103 private bool inImportantSection
{
get
{
.
return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.
Time ?? 0) <=
AllowedImportantTimeSpan;
}
}
Здесь, как и в предыдущем фрагменте кода, не учитывался приоритет операторов.
Теперь выражение передано в метод Математика.
Абс , рассчитывается следующим образом: (a – b) ?? 0
Исправленный код: private bool inImportantSection
{
get
{
.
return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.
Time ?? 0)) <=
AllowedImportantTimeSpan;
}
}
В3142 [CWE-561] Обнаружен недостижимый код. Вполне возможно, что ошибка присутствует. DrawableHoldNote.cs 214 public override bool OnPressed(ManiaAction action)
{
if (!base.OnPressed(action))
return false;
if (Result.Type == HitResult.Miss) // <=
holdNote.hasBroken = true;
.
}
Анализатор утверждает, что код обработчика OnPressed , начиная со второго оператора если , недоступен.
Это следует из предположения, что первое условие всегда верно, т. е.
метода base.OnPressed всегда вернусь ЛОЖЬ .
Давайте посмотрим на метод base.OnPressed : public virtual bool OnPressed(ManiaAction action)
{
if (action != Action.Value)
return false;
return UpdateResult(true);
}
Перейдем к методу Обновитьрезультат : protected bool UpdateResult(bool userTriggered)
{
if (Time.Elapsed < 0)
return false;
if (Judged)
return false;
.
return Judged;
}
Обратите внимание на реализацию свойства Судимый здесь не важно, так как по логике метода Обновитьрезультат отсюда следует, что последний оператор возвращаться эквивалентно этому: return false;
Итак, метод Обновитьрезультат всегда вернусь ЛОЖЬ , что приведет к ошибке с недоступным кодом в коде выше стека.
В3146 [CWE-476] Возможное нулевое разыменование «набора правил».
«FirstOrDefault» может возвращать нулевое значение по умолчанию.
APILegacyScoreInfo.cs 24 public ScoreInfo CreateScoreInfo(RulesetStore rulesets)
{
var ruleset = rulesets.GetRuleset(OnlineRulesetID);
var mods = Mods != null ? ruleset.CreateInstance() // <=
.
GetAllMods().
Where(.
) .
ToArray() : Array.Empty<Mod>(); .
}
Анализатор считает вызов небезопасным набор правил.
CreateInstance() .
Переменная набор правил ранее получает значение в результате вызова Получить набор правил : public RulesetInfo GetRuleset(int id) =>
AvailableRulesets.FirstOrDefault(.
);
Как видим, предупреждение анализатора оправдано, поскольку в цепочке вызовов есть ФёрстОрдефолт , который может возвращать значение нулевой .
Заключение
В общем, игровой проект «осу!» Порадовало небольшое количество ошибок.Однако рекомендую разработчикам обратить внимание на обнаруженные проблемы.
И пусть игра и дальше радует своих поклонников.
А для любителей поковыряться в коде, напоминаю, хорошим подспорьем станет анализатор PVS-Studio, который легко скачать с официального сайта.
Также отмечу, что разовые проверки проекта, подобные описанной выше, не имеют ничего общего с использованием статического анализатора в реальной работе.
Максимальной эффективности в борьбе с ошибками можно добиться только при регулярном использовании инструмента как на сервере сборки, так и непосредственно на компьютере разработчика (инкрементный анализ).
В этом случае задача-максимум — вообще не допустить попадания ошибок в систему контроля версий, исправляя дефекты уже на этапе написания кода.
Удачи и творческих успехов!
Ссылки
Это первая публикация 2020 года.Пользуясь случаем, приведу ссылки на статьи о проверке C#-проектов за последний год:
- Поиск ошибок в исходном коде Amazon Web Services SDK для .
NET.
- Проверка исходного кода Roslyn
- Ссылочные типы, допускающие значение NULL, в C# 8.0 и статический анализ
- WinForms: ошибки, Холмс
- История о том, как PVS-Studio нашел ошибку в библиотеке, используемой в.
PVS-Studio
- Проверка исходного кода библиотек .
NET Core статическим анализатором PVS-Studio
- Проверка анализаторов Roslyn
- Проверка Telerik UI для UWP для знакомства с PVS-Studio
- Azure PowerShell: «в основном безвреден»
- Ищем и анализируем ошибки в коде Orchard CMS.
- Проверка оболочки OpenCvSharp над OpenCV с помощью PVS-Studio
- Azure SDK для .
NET: история о сложном устранении неполадок
- SARIF SDK и его ошибки
- Топ-10 ошибок в проектах C# за 2019 год
Если вы хотите поделиться этой статьей с англоязычной аудиторией, воспользуйтесь ссылкой для перевода: Сергей Хренов.
Играйте в «осу!», но следите за ошибками.
.
Теги: #Разработка игр #C++ #.
NET #Visual Studio #pvs-studio #osu!
-
Баллистика
19 Oct, 24 -
Вся Правда О Lockerz.com
19 Oct, 24 -
10 Советов Клиентам Во Фрилансе
19 Oct, 24
