Geekly articles weekly

播放“ osu！”，不要忘记错误

图片1

我们欢迎所有热衷于异国情调的人，并且在代码中没有错误。 如今，在测试台上，PVS-Studio确实是一个难得的来宾-C＃游戏。 即大须！ 和往常一样：寻找错误，思考，玩耍。

游戏


大须！ -开源音乐节奏游戏。 从游戏网站上的信息来看，它非常受欢迎，因为拥有1500万注册玩家。 该项目的特点是免费玩法，具有自定义卡片功能的多彩设计，用于编制玩家在线排名的高级功能，多人游戏的存在以及大量音乐作品。 我不会详细描述游戏，有兴趣的人将很容易在网络上找到所有信息。 例如， 这里 。

我对项目的源代码更感兴趣，可以从GitHub下载该源代码。 对存储库的大量提交（超过24000次提交）立即引起关注，这表明该项目的积极开发一直持续到今天（游戏于2007年发布，但工作可能较早开始）。 同时，没有太多源代码-1813 .cs文件包含13.5万行代码，不包括空行。 这段代码包含了我通常在检查中没有考虑到的测试。 测试代码包含在306个.cs文件中，因此，包含2.5万行代码，不包括空行。 这是一个小项目：为了进行比较，PVS-Studio分析仪的C＃核心包含大约30万行代码。

总体而言，为了检查游戏中的错误，我使用了包含1507个源代码文件和11万行的非测试项目。 但是，结果使我感到有些满意，因为我不得不告诉您一些有趣的错误。

失误


V3001在'||'的左侧和右侧有相同的子表达式'result == HitResult.Perfect' 操作员。 DrawableHoldNote.cs 266

protected override void CheckForResult(....) { .... ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .... }); }

面向复制粘贴的编程的一个很好的例子。 我的同事Val​​ery Komarov最近在他的文章“ 2019年Java项目中的十大错误 ”中使用（引入）了一个漫画术语。

因此，两个相同的检查接连进行。 其中一项检查最有可能包含另一个HitResult枚举常量 ：

 public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }

需要使用哪个特定常数，或者根本不需要第二检查？ 只有开发人员才能回答的问题。 无论如何，都会产生错误，使程序的逻辑失真。

V3001在'&&'运算符的左侧和右侧有相同的子表达式'family！= GetFamilyString（TournamentTypeface.Aquatico）'。 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; .... }

并再次复制粘贴。 我格式化了代码，因此错误很容易引起注意。 在原始版本中，整个条件写在一行中。 很难说如何固定代码。 TournamentTypeface枚举包含单个常量：

 public enum TournamentTypeface { Aquatico }

该条件可能错误地使用了两次家庭变量，但这并不准确。

V3009 [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; }

该方法将始终返回false 。 对于此类错误，我通常检查调用代码，因为它们通常只是在任何地方都不使用返回值，所以没有错误（丑陋的编程风格除外）。 在这种情况下，我遇到了以下代码：

 public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));

如您所见，使用了OnPressed方法返回的结果。 并且由于它始终为false ，所以调用OnPressed的结果也将始终为false 。 我认为您应该再次仔细检查此代码，因为发生错误的可能性很高。

另一个类似的错误：

  • V3009 [CWE-393]奇怪的是，此方法始终返回一个相同的'false'值。 KeyCounterAction.cs 30

V3042 [CWE-476]可能为NullReferenceException。 '？。' 和“。” 运算符用于访问“ 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不安全。 分析器得出这样的结论，从那时起，在随后的分支中，使用安全选项通过条件访问语句val.NewValue？.Substring（....）来访问变量。

另一个类似的错误：

  • V3042 [CWE-476]可能为NullReferenceException。 '？。' 和“。” 运算符用于访问“ val.NewValue”对象TournamentTeam.cs的成员48

V3042 [CWE-476]可能为NullReferenceException。 '？。' 和“。” 运算符用于访问“ 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的对象。 Action字段使用lambda初始化，在该lambda的正文中，使用可能为空的引用api是不安全的。 分析器认为此模式是错误的，因为此后，在初始化Value参数时， api变量可以安全运行。 我称该错误为模棱两可，因为lambda代码假定执行延迟，然后，也许开发人员以某种方式保证api链接的值不为零。 但这只是一个假设，因为lambda主体不包含任何安全链接的迹象（例如，初步检查）。

V3066 [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); .... }

分析器怀疑使用Math类的Atan2方法时，开发人员会混淆参数的顺序。 Atan2广告 ：

 // Parameters: // y: // The y coordinate of a point. // // x: // The x coordinate of a point. public static double Atan2(double y, double x);

如您所见，这些值以相反的顺序传输。 我无法判断这是否是一个错误，因为UpdateProgress方法包含许多非平凡的计算。 我只是注意到代码中可能存在问题的事实。

V3080 [CWE-476]可能的空解除引用。 考虑检查“ Beatmap”。 WorkingBeatmap.cs 57

 protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }

分析器指出了通过Beatmap空链接访问的危险。 这是什么：

 public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }

好吧，分析仪是正确的。

有关PVS-Studio如何发现此类错误以及与类似主题相关的C＃8.0创新（使用潜在的空引用）的更多信息，请参见文章“ C＃8.0和静态分析中的可空引用类型 ”。

V3083 [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); .... }

V3095 [CWE-476]在验证是否为null之前，已使用“列”对象。 检查线：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(); }

在循环中绕过列集合是不安全的。 同时，开发人员假定column链接可能为null，因为在代码的后面，使用了条件访问运算符来访问集合。

V3119调用重写的事件'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

V3123 [CWE-783]也许是“ ??” 操作员的工作方式与预期不同。 它的优先级低于其左侧的其他运营商的优先级。 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); }

代码中的另一个类似错误：

V3123 [CWE-783]也许是“ ??” 操作员的工作方式与预期不同。 它的优先级低于其左侧的其他运营商的优先级。 FramedReplayInputHandler.cs 103

 private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan; } }

在这里，与前面的代码片段一样，没有考虑运算符的优先级。 现在，传递给Math.Abs方法的表达式的计算如下：

 (a – b) ?? 0

更正的代码：
 private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= AllowedImportantTimeSpan; } }

V3142 [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; .... }

分析器声称从第二个if语句开始的OnPressed处理程序代码不可访问。 这是基于第一个条件始终为true的假设，即base.OnPressed方法将始终返回false 。 看一下base.OnPressed方法：

 public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }

我们继续执行UpdateResult方法：
 protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; }

请注意，此处的Judged属性的实现并不重要，因为UpdateResult方法的逻辑意味着最后的return语句与此等效：

 return false;

因此， UpdateResult方法将始终返回false ，这将导致堆栈上方代码中的代码无法到达的错误。

V3146 [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>(); .... }

分析器认为ruleset.CreateInstance（）调用不安全。 规则集变量先前通过调用GetRuleset来获取值：

 public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);

如您所见，分析器警告是合理的，因为调用链包含FirstOrDefault ，它可以返回null 。

结论


通常，游戏项目“ osu！”对少量错误感到满意。 不过，我建议开发人员注意发现的问题。 并让游戏继续取悦其粉丝。

对于想深入研究代码的人，我想提醒您， 可以从官方网站上轻松下载的PVS-Studio分析仪将是一个很好的帮助。 我还注意到，如上所述的一次性项目检查与在实际工作中使用静态分析器无关。 只有在构建服务器上和直接在开发人员的计算机上定期使用该工具（增量分析），才能实现最大的抗错误效率。 最大的目标是防止错误进入版本控制系统，修复在编写代码阶段已经存在的缺陷。

祝你好运，成功！

参考文献


这是2020年的第一本出版物。 借此机会，我将提供有关去年检查C＃项目的文章的链接：




如果您想与讲英语的读者分享这篇文章，请使用以下链接：Sergey Khrenov。 播放“ osu！”，但请注意错误 。

Source: https://habr.com/ru/post/zh-CN483438/

More articles:


All Articles