Geekly articles weekly

العب "osu!" ، لكن احترس من الأخطاء

الصورة 1


مرحبًا بكم جميعًا جامعي الخلل الغريبة والعادية على حد سواء! لدينا عينة نادرة على منصة اختبار PVS-Studio اليوم - لعبة تسمى "osu!" ، كتبت في C #. كالعادة ، سنبحث عن الأخطاء ونحللها ونلعبها.

اللعبة


أوسو! هي لعبة إيقاع مفتوحة المصدر. وفقًا لموقع الويب الخاص باللعبة ، فهي تحظى بشعبية كبيرة ، حيث تضم أكثر من 15 مليون حساب لاعب. يتميز المشروع باللعب المجاني والتصميم الغني بالألوان وتخصيص الخرائط ونظام تصنيف اللاعبين المتقدم عبر الإنترنت ووضع اللاعبين المتعددين ومجموعة غنية من القطع الموسيقية. ليس هناك نقطة في مزيد من التفصيل على اللعبة. يمكنك قراءة كل شيء عن ذلك على شبكة الإنترنت. ابدأ بهذه الصفحة .

أنا مهتم أكثر برمز مصدر المشروع ، والذي يتوفر على جيثب . الشيء الوحيد الذي لفت انتباهك على الفور هو العدد الكبير من عمليات تخزين المستودعات (أكثر من 24 ألف) ، وهو علامة على التطوير المستمر المكثف (تم إصدار اللعبة لأول مرة في عام 2007 ، ولكن يجب أن يكون العمل قد بدأ قبل ذلك). المشروع ليس كبيرًا على الرغم من أنه: 1813 ملف .cs فقط بإجمالي 135 ألف ملف LOC فارغ. يتضمن هذا الرقم أيضًا الاختبارات ، والتي عادةً لا أراعيها عند تشغيل الشيكات. تشكل الاختبارات 306 من ملفات .cs مع 25 ألف LOC. المشروع صغير بالفعل: على سبيل المثال ، يبلغ طول C # لبرامج PVS-Studio حوالي 300 ألف LOC.

ترك الملفات الاختبار ، راجعت 1507 ملفات 110 ألف LOC طويلة. كشف النقاب عن بعض الأخطاء المثيرة للاهتمام ، والتي أرغب في إظهارها لك.

البق


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

هذا مثال جيد على البرمجة الموجهة للنسخ ، وهو مصطلح فكاهي استخدمه مؤخرًا زميلي في العمل فاليري كوماروف في مقالته " أفضل 10 أخطاء تم العثور عليها في مشاريع جافا في عام 2019 ".

على أي حال ، يتم تنفيذ اثنين من الشيكات متطابقة في صف واحد. ربما كان من المفترض أن يقوم أحدهم بالتحقق من بعض الثابت الآخر لتعداد HitResult :

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

أي ثابت كان من المفترض أن يتم فحصه؟ أو ربما يجب ألا يكون الشيك الثاني على الإطلاق؟ هذه هي الأسئلة التي يمكن للمؤلفين الإجابة عليها فقط. على أي حال ، هذا خطأ يشوه منطق تنفيذ البرنامج.

V3001 هناك عائلة تعبيرات فرعية متطابقة! = 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; }

هذه الطريقة ترجع كاذبة في كل مرة. في مثل هذه الحالات ، عادة ما أتحقق من استدعاء الوظيفة ، لأنه قد تجد في كثير من الأحيان أن المتصل لا يستخدم قيمة الإرجاع ، مما يعني أنه لا توجد مشكلة (بخلاف الأسلوب السيئ). هذا ما تبدو عليه المكالمة في هذه الحالة:

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

كما ترون ، يستخدم المتصل القيمة التي يتم إرجاعها بواسطة أسلوب OnPressed . نظرًا لأن هذه القيمة خاطئة دائمًا ، فإن المتصل نفسه يرجع دائمًا إلى الخطأ أيضًا. من المحتمل جدًا أن يحتوي هذا الرمز على خطأ ويجب مراجعته.

خطأ آخر مماثل:

  • 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 ، التي تتعامل مع مرجع المرجع المحتمل الفارغ بطريقة خطيرة. يعتقد المحلل أن هذا النمط خطأ لأنه يتم التعامل مع متغير api بطريقة آمنة لاحقًا ، عند تهيئة المعلمة Value . لقد وصفت هذه الحالة بأنها غامضة لأن الكود الموجود في وظيفة 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); .... }

يشتبه المحلل في أن وسائط أسلوب 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(); .... }

المحلل يشير إلى dereference فارغة محتملة من Beatmap :

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

حسنا ، المحلل هو الصحيح.

لمعرفة المزيد حول كيفية اكتشاف PVS-Studio لأخطاء من هذا القبيل ، وحول الميزات الجديدة المضافة في C # 8.0 والتي لها علاقة بمعالجة المراجع التي قد تكون لاغية ، راجع مقالة " أنواع المراجع Nullable في 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] تم استخدام كائن "الأعمدة" قبل التحقق من صحته. خطوط التحقق: 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" قد يؤدي إلى سلوك غير متوقع. فكر في تطبيق أدوات الوصول إلى الأحداث بشكل صريح أو استخدم الكلمة الأساسية "المختومة". DrawableRuleset.cs 256

 private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult;

يقول المحلل إنه من الخطر استخدام حدث افتراضي أو افتراضي. انظر وصف التشخيص للحصول على شرح. كتبت أيضًا مقالًا حول هذا الموضوع: " الأحداث الافتراضية في C #: حدث خطأ ما ".

إليك بنية أخرى غير آمنة مماثلة:

  • V3119 استدعاء حدث تم تجاوزه قد يؤدي إلى سلوك غير متوقع. فكر في تطبيق أدوات الوصول إلى الأحداث بشكل صريح أو استخدم الكلمة الأساسية "المختومة". 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; .... }

يعتقد المحلل أن رمز معالج OnPressed لا يمكن الوصول إليه بدءًا من العبارة if الثانية. يتبع ذلك حقيقة أن الشرط الأول صحيح دائمًا ، أي أن الأسلوب 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; }

لاحظ أن تطبيق خاصية التحكيم لا يهم هنا لأن منطق أسلوب UpdateResult يعني أن بيان الإرجاع الأخير مكافئ لما يلي:

 return false;

هذا يعني أن أسلوب UpdateResult سيعود كاذبة طوال الوقت ، مما يؤدي إلى مشكلة رمز يتعذر الوصول إليه في وقت سابق في بنية تخزين العناصر.

V3146 [CWE-476] احتمال إلغاء فارغة لـ "ruleset". يمكن لـ '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 ، والذي يمكن أن يُرجع فارغًا .

استنتاج


لا توجد العديد من الأخطاء في رمز "osu!" ، وهذا جيد. لكن ما زلت أوصي المؤلفين بالتحقق من المشكلات التي أبلغ عنها المحلل. آمل أن يساعد هذا في الحفاظ على الجودة العالية وأن تستمر اللعبة في جلب الفرح للاعبين.

كتذكير ، يعد PVS-Studio اختيارًا جيدًا إذا كنت ترغب في استخدام التعليمات البرمجية المصدر. المحلل متاح للتحميل على الموقع الرسمي. شيء آخر أود أن تضعه في اعتبارك هو أن الشيكات لمرة واحدة مثل هذا الشيك لا علاقة لها بالاستخدام العادي للتحليل الثابت في عملية التطوير الحقيقية. يكون أكثر فاعلية فقط عند استخدامه بشكل منتظم على كلٍ من خادم الإنشاء وعلى أجهزة كمبيوتر المطورين (وهذا ما يسمى التحليل التزايدي). هدفك النهائي هو منع الأخطاء من الانزلاق إلى نظام التحكم في الإصدار عن طريق اصطيادها في مرحلة الترميز.

حظا سعيدا ، والبقاء الإبداعي!

مراجع


هذا هو مقالنا الأول في عام 2020. بينما نحن موجودون فيه ، إليك الروابط الخاصة بمشاريع C # التي تم تنفيذها على مدار العام الماضي:

Source: https://habr.com/ru/post/ar483436/

More articles:


All Articles