Geekly articles weekly

العب "osu!" ، لا تنس الأخطاء

الصورة 1

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

اللعبة


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

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

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

أخطاء


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 . ونظرًا لأنه خطأ دائمًا ، ستكون نتيجة استدعاء 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 ، حيث يكون من غير الآمن العمل مع مرجع مرجح لاغٍ. اعتبر المحلل هذا النمط خطأً ، ومنذ ذلك الحين ، عند تهيئة المعلمة Value ، يعمل متغير api بأمان. لقد وصفت الخطأ بأنه غامض ، لأن رمز lambda يفترض تأجيل التنفيذ وبعد ذلك ، ربما يضمن المطور بطريقة ما قيمة غير صفرية لارتباط api . لكن هذا مجرد افتراض ، لأن جسم لامدا لا يحتوي على أي علامات للعمل الآمن مع الرابط (الاختبارات الأولية ، على سبيل المثال).

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 لفئة الرياضيات ، يختلط المطور بترتيب الوسيطات. إعلان 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 المتعلقة بموضوع مشابه (العمل مع المراجع التي يحتمل أن تكون خالية) ، راجع مقالة " أنواع المراجع 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 ، لا يمكن الوصول إليه. يتبع ذلك الافتراض بأن الشرط الأول صحيح دائمًا ، أي أن الطريقة الأساسية. ستُرجع الطريقة غير الظاهرة دائمًا إلى الخطأ . ألقِ نظرة على القاعدة. طريقة الضغط :

 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 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 () استدعاء غير آمنة. يحصل المتغير ruleset مسبقًا على القيمة نتيجة لاستدعاء GetRuleset :

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

كما ترون ، فإن تحذير محلل له ما يبرره ، لأن سلسلة الاتصال تحتوي على FirstOrDefault ، والتي يمكن أن تعود خالية .

استنتاج


بشكل عام ، مشروع اللعبة "osu!" يسر مع عدد قليل من الأخطاء. ومع ذلك ، فإنني أوصي المطورين بالاهتمام بالمشكلات المكتشفة. ودع اللعبة تستمر في إرضاء جماهيرها.

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

بالتوفيق والنجاح!

مراجع


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




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

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

More articles:


All Articles