مؤلفي اللعبة 0 م - أحسنت

0 AD هي لعبة ثلاثية الأبعاد في نوع الاستراتيجية التاريخية في الوقت الحقيقي ، طورها مجتمع المتطوعين. حجم قاعدة الشفرة صغير وقررت التحقق من اللعبة على أنها استراحة من المشاريع الكبيرة مثل Android و XNU Kernel. إذن ، أمامنا مشروع يحتوي على 165000 سطر من التعليمات البرمجية في C ++. دعونا نرى ما هو مثير للاهتمام يمكن العثور عليه باستخدام محلل ثابت PVS-Studio.

0 م

0 AD ( 0 AD ) هي لعبة مجانية ثلاثية الأبعاد من نوع الاستراتيجية التاريخية في الوقت الحقيقي ، تم تطويرها من قبل مجتمع من المتطوعين (توحد المطورين الرئيسيين في فريق Wildfire Games). تسمح لك اللعبة بالتحكم في الحضارات التي كانت موجودة في الفترة 500 قبل الميلاد. هـ - سنة واحدة ق. ه. اعتبارًا من صيف عام 2018 ، يكون المشروع في إصدار ألفا. [ الوصف مأخوذ من ويكيبيديا].

لماذا بالضبط 0 م؟

طلبت من زميل Egor Bredikhin ( EgorBredikhin ) اختيار بعض المشاريع الصغيرة المفتوحة التي يمكنني العمل بها بين المهام الأخرى والتحقق منها. أرسل لي سجل للمشروع 0 م إلى السؤال: "لماذا هذا المشروع؟" - كان هناك جواب: "نعم ، لقد لعبت هذه اللعبة للتو ، وهي استراتيجية جيدة في الوقت الحقيقي." حسنًا ، سيكون 0 م :).

كثافة الخطأ

أريد أن أثني على مؤلفي 0 م لجودة كود سي ++. حسنًا ، نادرًا ما تتمكن من مواجهة مثل هذه الأخطاء المنخفضة الكثافة. بالطبع ، هذه ليست كل الأخطاء ، ولكن تلك التي يمكن اكتشافها باستخدام PVS-Studio. كما قلت ، على الرغم من أن PVS-Studio لا يجد جميع الأخطاء ، ومع ذلك ، يمكننا التحدث بأمان عن العلاقة بين كثافة الأخطاء وجودة الشفرة ككل.

عدد قليل من الأرقام. العدد الإجمالي لأسطر التعليمات البرمجية غير الفارغة هو 231270. منها 28.7٪ تعليقات. في المجموع ، 165000 سطر من كود C ++ النقي.

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

لذلك ، وجدت 19 خطأ في 165000 سطر من التعليمات البرمجية. نحسب كثافة الخطأ: 19 * 1000/165000 = 0.115.

من أجل البساطة ، نقترب ونفترض أن محلل PVS-Studio يكتشف 0.1 خطأ لكل 1000 سطر من التعليمات البرمجية في رمز اللعبة.

نتيجة عظيمة! للمقارنة ، في مقالتي الأخيرة حول Android ، حسبت أنني اكتشفت على الأقل 0.25 خطأ لكل 1000 سطر من التعليمات البرمجية. في الواقع ، كثافة الأخطاء هناك أكبر من ذلك ، لم أجد القوة لتحليل التقرير بأكمله بعناية.

أو خذ مكتبات EFL الأساسية كمثال ، الذي قمت بدراسته بعناية وحساب عدد العيوب. في ذلك ، يكتشف PVS-Studio أخطاء 0.71 لكل 1000 سطر من التعليمات البرمجية.

لذا ، فإن مؤلفي 0 AD هم زملاء جيدون ، من أجل الإنصاف ، تجدر الإشارة إلى أن كمية صغيرة من التعليمات البرمجية المكتوبة في C ++ تساعدهم. لسوء الحظ ، كلما كان المشروع أكبر ، كلما ازداد تعقيده بشكل أسرع ، وزادت كثافة الخطأ بشكل غير خطي ( المزيد ).

أخطاء

دعنا الآن نلقي نظرة على الأخطاء الـ 19 التي وجدتها في اللعبة. لتحليل المشروع ، استخدمت PVS-Studio الإصدار 6.24. أقترح عليك محاولة تنزيل الإصدار التجريبي والتحقق من المشاريع التي تعمل عليها.

ملاحظة نضع PVS-Studio كحل B2B. بالنسبة للمشروعات الصغيرة والمطورين الأفراد ، لدينا خيار ترخيص مجاني: " كيفية استخدام PVS-Studio مجانًا ".

خطأ N1

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

void WaterManager::CreateWaveMeshes() { .... int nbNeighb = 0; .... bool found = false; nbNeighb = 0; for (int p = 0; p < 8; ++p) { if (CoastalPointsSet.count(xx+around[p][0] + (yy + around[p][1])*SideSize)) { if (nbNeighb >= 2) { CoastalPointsSet.erase(xx + yy*SideSize); continue; } ++nbNeighb; // We've found a new point around us. // Move there xx = xx + around[p][0]; yy = yy + around[p][1]; indexx = xx + yy*SideSize; if (i == 0) Chain.push_back(CoastalPoint(indexx,CVector2D(xx*2,yy*2))); else Chain.push_front(CoastalPoint(indexx,CVector2D(xx*2,yy*2))); CoastalPointsSet.erase(xx + yy*SideSize); found = true; break; } } if (!found) endedChain = true; .... } 

تحذير PVS-Studio: التعبير V547 CWE-570 'nbNeighb> = 2' خطأ دائمًا. WaterManager.cpp 581

للوهلة الأولى ، تبدو رسالة المحلل غريبة. لماذا الشرط nbNeighb> = 2 دائمًا خاطئ؟ في الواقع ، في جسم الحلقة هناك زيادة في المتغير nbNeighb !

انظر أدناه وستشاهد بيان فاصل يقاطع تنفيذ الحلقة. لذلك ، إذا تم زيادة المتغير nbNeighb ، سيتم إيقاف الدورة. وبالتالي ، فإن قيمة المتغير nbNeighb لن تصل أبدًا إلى قيمة أكبر من 1.

من الواضح أن الرمز يحتوي على نوع من الخطأ المنطقي.

خطأ N2

 void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments) { .... segments.erase(segments.end()); .... } 

تحذير PVS-Studio: قد يتم V783 CWE-119 إلغاء الإشارة إلى المقاطع غير الصالحة 'segments.end ()'. 1290

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

 segments.erase(segments.end() - 1); 

على الرغم من ذلك ، سيكون من الأسهل الكتابة:

 segments.pop_back(); 

لنكون صادقين ، ليس من الواضح تمامًا بالنسبة لي بالضبط ما كان يجب كتابته هنا.

خطأ N3 ، N4

قررت النظر في خطأين معًا ، لأنهما يتعلقان بتسرب موارد ويتطلبان أولاً إظهار ما هو الماكرو WARN_RETURN .

 #define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0) 

لذا ، كما ترى ، يتسبب الماكرو WARN_RETURN في خروج الدالة من الجسم. الآن دعونا نلقي نظرة على طرق غير دقيقة لاستخدام هذا الماكرو.

الجزء الأول.

 Status sys_generate_random_bytes(u8* buf, size_t count) { FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK; } 

تحذير PVS-Studio: V773 CWE-401 تم إنهاء الوظيفة بدون تحرير المقبض 'f'. من الممكن حدوث تسرب في الموارد. 332

إذا لم تستطع وظيفة fread قراءة البيانات ، فسيتم إنهاء وظيفة sys_generate_random_bytes دون تحرير واصف الملف. من الناحية العملية ، هذا بالكاد ممكن. من المشكوك فيه أنك لن تتمكن من قراءة البيانات من "/ dev / urandom". ومع ذلك ، فإن الرمز قذر.

الجزء الثاني.

 Status sys_cursor_create(....) { .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK; } 

تحذير PVS-Studio: V773 CWE-401 تم إنهاء الوظيفة بدون تحرير مؤشر "impl". من الممكن حدوث تسرب للذاكرة. xcpp 421

إذا فشل تحميل المؤشر ، يحدث تسرب للذاكرة.

خطأ N5

 Status LoadHeightmapImageOs(....) { .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); .... } 

تحذير PVS-Studio: V554 CWE-762 الاستخدام غير الصحيح لـ Shared_ptr. سيتم تنظيف الذاكرة المخصصة لـ "new []" باستخدام "delete". MapIO.cpp 54

الخيار الصحيح:

 shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]); 

خطأ N6

 FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; } 

تحذير PVS-Studio: V570 يتم تعيين المتغير "ptr" لنفسه. FUTracker.h 122

خطأ N7 ، N8

 std::wstring TraceEntry::EncodeAsText() const { const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf; } 

تحذير PVS-Studio: V576 CWE-628 تنسيق غير صحيح. ضع في اعتبارك التحقق من الوسيطة الفعلية الخامسة للدالة "swprintf_s". من المتوقع أن تكون وسيطة نوع الحرف. أثر. cpp 93

هنا صادفنا تاريخًا مرتبكًا وغير واضح لتنفيذ بديل لوظيفة swprintf في Visual C ++. لن أقوم بإعادة بيعه ، ولكن ارجع إلى وثائق تشخيص V576 (انظر قسم "الخطوط العريضة").

في هذه الحالة ، على الأرجح ، سيعمل هذا الرمز بشكل صحيح عند التحويل البرمجي في Visual C ++ لـ Windows وبشكل غير صحيح عند البناء لنظام التشغيل Linux أو macOS.

خطأ مماثل: V576 CWE-628 تنسيق غير صحيح. ضع في اعتبارك التحقق من الوسيطة الفعلية الرابعة للدالة "swprintf_s". من المتوقع أن تكون وسيطة نوع الحرف. 211

خطأ N9 و N10 و N11

كلاسيك في البداية ، يتم استخدام المؤشر ، ثم يتم فحصه فقط.

 static void TEST_CAT2(char* dst, ....) { strcpy(dst, dst_val); // <= int ret = strcat_s(dst, max_dst_chars, src); TS_ASSERT_EQUALS(ret, expected_ret); if(dst != 0) // <= TS_ASSERT(!strcmp(dst, expected_dst)); } 

تحذير PVS-Studio: V595 CWE-476 تم استخدام مؤشر "dst" قبل التحقق من صحته باستخدام nullptr. خطوط التحقق: 140 ، 143. test_secure_crt.h 140

أعتقد أن الخطأ لا يتطلب شرحًا. تحذيرات مماثلة:

• V595 CWE-476 تم استخدام مؤشر "dst" قبل التحقق منه من خلال nullptr. تحقق من الخطوط: 150 ، 153. test_secure_crt.h 150
• V595 CWE-476 تم استخدام مؤشر "dst" قبل التحقق منه من خلال nullptr. خطوط التحقق: 314 ، 317. test_secure_crt.h 314

خطأ N12

 typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... } 

V674 CWE-682 تتم مقارنة الحرف "0.5" الحرفي من النوع "المزدوج" بقيمة النوع "int". ضع في الاعتبار فحص تعبير "bIsOrientationPreserving> 0.5". MikktspaceWrap.cpp 137

ليس من المنطقي مقارنة متغير من نوع int بثابت 0.5. علاوة على ذلك ، من حيث المعنى ، هذا بشكل عام متغير من النوع المنطقي ، مما يعني أن مقارنته بـ 0.5 تبدو غريبة للغاية. افترض أنه بدلاً من bIsOrientationPreserving متغير آخر يجب استخدامه هنا.

خطأ N13

 virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size) { ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE); // There is no such file, create it. if (st == ERR::VFS_FILE_NOT_FOUND) { s.~ScopedLock(); return CreateFile(pathname, fileContents, size); } .... } 

تحذير PVS-Studio: V749 CWE-675 سيتم استدعاء مدمر كائن 's' مرة أخرى بعد مغادرة نطاق الكائن. vfs.cpp 165

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

خطأ N14 ، N15 ، N16 ، N17

 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... } 

تحذير PVS-Studio: V668 CWE-570 ليس هناك أي معنى في اختبار مؤشر "pEvent" مقابل القيمة الخالية ، حيث تم تخصيص الذاكرة باستخدام عامل التشغيل "الجديد". سيتم إنشاء الاستثناء في حالة خطأ تخصيص الذاكرة. fsm.cpp 259

التحقق من المؤشر لا معنى له ، لأنه في حالة وجود خطأ في تخصيص الذاكرة ، سيتم طرح استثناء std :: bad_alloc .

لذا ، فإن الشيك غير ضروري ، لكن الخطأ ليس جسيماً. ومع ذلك ، كل شيء أسوأ بكثير عندما يتم تنفيذ بعض المنطق في نص جملة if . خذ بعين الاعتبار الحالة التالية:

 CFsmTransition* CFsm::AddTransition(....) { .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL; // Create new transition CFsmTransition* pNewTransition = new CFsmTransition( state ); if ( !pNewTransition ) { delete pEvent; return NULL; } .... } 

تحذير المحلل: V668 CWE-570 ليس هناك معنى في اختبار مؤشر "pNewTransition" مقابل قيمة خالية ، حيث تم تخصيص الذاكرة باستخدام عامل التشغيل "الجديد". سيتم إنشاء الاستثناء في حالة خطأ تخصيص الذاكرة. 289

جرت محاولة لتحرير الذاكرة التي يتم تخزين عنوانها في مؤشر pEvent . بطبيعة الحال ، لن يحدث هذا وسيحدث تسرب للذاكرة.

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

 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { CFsmEvent* pEvent = NULL; // Lookup event by type EventMap::iterator it = m_Events.find( eventType ); if ( it != m_Events.end() ) { pEvent = it->second; } else { pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; // Store new event into internal map m_Events[ eventType ] = pEvent; } return pEvent; } 

لاحظ أن الدالة لا تعيد المؤشر دائمًا إلى كائن جديد تم إنشاؤه باستخدام عامل التشغيل الجديد . في بعض الأحيان يأخذ كائن موجود من حاوية m_Events . وبالمناسبة ، سيتم وضع مؤشر الكائن الذي تم إنشاؤه حديثًا في m_Events .

وهنا يطرح السؤال: من يملك وينبغي أن يدمر الأشياء التي يتم تخزين مؤشراتها في حاوية m_Events ؟ لست على دراية بالمشروع ، ولكن على الأرجح في مكان ما يوجد رمز يدمر كل هذه الأشياء. ثم حذف كائن داخل دالة CFsm :: AddTransition غير ضروري بشكل عام.

لدي انطباع أنه يمكنك ببساطة حذف جزء التعليمات البرمجية التالي:

 if ( !pNewTransition ) { delete pEvent; return NULL; } 

أخطاء أخرى:

• V668 CWE-571 لا معنى لاختبار مؤشر "ret" مقابل قيمة خالية ، حيث تم تخصيص الذاكرة باستخدام عامل التشغيل "الجديد". سيتم إنشاء الاستثناء في حالة خطأ تخصيص الذاكرة. الحلقة 120
• V668 CWE-571 لا يوجد معنى في اختبار مؤشر "الجواب" مقابل القيمة الخالية ، حيث تم تخصيص الذاكرة باستخدام عامل التشغيل "الجديد". سيتم إنشاء الاستثناء في حالة خطأ تخصيص الذاكرة. 542

خطأ N18 ، N19

 static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) { // TODO(lsm): propagate an error to the caller dsd->num_entries = 0; } else { dsd->entries[dsd->num_entries].file_name = mg_strdup(de->file_name); dsd->entries[dsd->num_entries].st = de->st; dsd->entries[dsd->num_entries].conn = de->conn; dsd->num_entries++; } } 

تحذير PVS-Studio: V701 CWE-401 realloc () تسرب محتمل: عندما يفشل realloc () في تخصيص الذاكرة ، يتم فقدان المؤشر الأصلي 'dsd-> الإدخالات'. خذ بعين الاعتبار تعيين realloc () لمؤشر مؤقت. 2462

إذا أصبح حجم الصفيف غير كافٍ ، فسيتم تخصيص الذاكرة باستخدام وظيفة realloc . الخطأ هو أن قيمة المؤشر إلى كتلة الذاكرة الأصلية يتم استبدالها على الفور بالقيمة الجديدة التي تم إرجاعها بواسطة دالة realloc .

إذا فشل تخصيص الذاكرة ، فسوف تقوم دالة realloc بإرجاع NULL ، وستتم كتابة NULL إلى متغير الإدخالات dsd-> . بعد ذلك سيكون من المستحيل تحرير كتلة الذاكرة التي تم تخزين عنوانها مسبقًا في إدخالات dsd-> . سيحدث تسرب للذاكرة.

خطأ آخر: تسرب محتمل لـ V701 CWE-401 realloc (): عندما يفشل realloc () في تخصيص الذاكرة ، يتم فقدان المؤشر الأصلي "المخزن المؤقت". خذ بعين الاعتبار تعيين realloc () لمؤشر مؤقت. المعالج المسبق. cpp 84

الخلاصة

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

شكرا لكم على اهتمامكم. لإجراء تغيير ، سأنهي المقالة بدعوتي لمتابعتني على Instagrampvs_studio_unicorn وعلى TwitterCode_Analysis .

إذا كنت تريد مشاركة هذه المقالة مع جمهور ناطق باللغة الإنجليزية ، فيرجى استخدام رابط الترجمة: Andrey Karpov. عمل جيد ، مؤلفي اللعبة 0 م!