Geekly articles weekly

播放“ osu！”，但请注意错误

图片1


嗨，大家都很喜欢异国和普通bug的收藏家！ 今天，我们的PVS-Studio测试台上有一个罕见的标本-用C＃编写的名为“ osu！”的游戏。 像往常一样，我们将寻找错误，对其进行分析和播放。

游戏


大须！ 是一款开源节奏游戏。 根据该游戏的网站 ，它非常受欢迎，拥有超过1500万玩家帐户。 该项目具有免费的游戏玩法，色彩缤纷的设计，地图定制，先进的在线玩家排名系统，多人游戏模式以及丰富的音乐作品。 没有必要进一步阐述游戏了。 您可以在Internet上阅读有关它的所有信息。 从此页面开始。

我对项目的源代码更感兴趣，该源代码可在GitHub上找到 。 立即引起您注意的一件事是，大量的存储库提交（超过2万4千次），这表明正在持续进行紧张的开发（该游戏于2007年首次发布，但工作必须更早开始）。 但是，该项目并不大：只有1813个.cs文件，总共有13.5万个非空LOC。 该数字还包括测试，运行检查时通常不会考虑这些测试。 测试组成了306个.cs文件，具有2.5万个LOC。 该项目确实很小：例如，PVS-Studio的C＃内核大约有30万个LOC。

除去测试文件，我检查了1105个LOC长的1507个文件。 检查确实发现了一些有趣的错误，我愿意向您展示。

虫子


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​​eriy 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 ，因此调用者本身也总是返回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变量。 使分析器如此认为的事实是，在随后的then分支中，使用条件访问运算符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类型的对象。 使用lambda函数初始化Action字段，该函数以危险的方式处理可能为空的引用api 。 分析器认为此模式是错误的，因为在初始化Value参数时，以后会以安全的方式处理api变量。 我之所以称这种情况为模棱两可，是因为lambda函数中的代码暗含了执行延迟，此时开发人员可能会以某种方式保证api引用为非null。 但是我不确定，因为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); .... }

分析器怀疑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(); }

列集合上的迭代以危险的方式进行。 开发人员假定列引用可以为空，这通过使用条件访问运算符来进一步访问代码中的集合来指示。

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＃项目进行检查的链接：

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

More articles:


All Articles