التحقق من شبكة Telegram Open باستخدام PVS-Studio

Telegram Open Network (TON) هي عبارة عن منصة من قبل نفس الفريق الذي طور رسول Telegram. بالإضافة إلى blockchain ، توفر TON مجموعة كبيرة من الخدمات. قام المطورون مؤخرًا بتكوين رمز النظام الأساسي ، والذي تمت كتابته بلغة C ++ ، وهو متاح للجمهور وتحميله إلى GitHub. قررنا التحقق من المشروع قبل صدوره الرسمي.

مقدمة

Telegram Open Network هي مجموعة من الخدمات المختلفة. من بين أشياء أخرى ، يوفر نظام دفع خاص به يستند إلى عملة تشفير Gram وجهاز افتراضي يسمى TON VM ، والذي ينفذ العقود الذكية. كما يقدم خدمة الرسائل ، رسائل تون. يُنظر إلى المشروع ككل كإجراء مضاد للرقابة على الإنترنت.

تم بناء المشروع باستخدام CMake ، لذلك لم أواجه أية صعوبات في البناء والتحقق منه. كود المصدر مكتوب بلغة C ++ 14 ويمتد إلى 210 ألف LOC:

نظرًا لأن المشروع صغير وعالي الجودة ، لا يوجد الكثير من الأخطاء فيه ، ولكن لا يزال يتعين التعامل معه.

كود الإرجاع

static int process_workchain_shard_hashes(....) { .... if (f == 1) { if ((shard.shard & 1) || cs.size_ext() != 0x20000) { return false; // <= } .... int r = process_workchain_shard_hashes(....); if (r < 0) { return r; } .... return cb.store_bool_bool(true) && cb.store_ref_bool(std::move(left)) && cb.store_ref_bool(std::move(right)) && cb.finalize_to(branch) ? r : -1; .... } 

رسالة تشخيص PVS-Studio: V601 يتم رسم القيمة "false" ضمنيًا على نوع العدد الصحيح. mc-config.cpp 884

يبدو أن الدالة ترجع النوع الخطأ من حالة الخطأ هنا. يجب أن تقوم الدالة بإرجاع قيمة سالبة للفشل بدلاً من صواب / خطأ. هذا على الأقل ما يفعله بشكل أكبر في الكود ، حيث يتم إرجاع -1.

مقارنة متغير مع نفسه

 class LastBlock : public td::actor::Actor { .... ton::ZeroStateIdExt zero_state_id_; .... }; void LastBlock::update_zero_state(ton::ZeroStateIdExt zero_state_id) { .... if (zero_state_id_ == zero_state_id_) { return; } LOG(FATAL) << ....; } 

رسالة تشخيص PVS-Studio: V501 هناك تعبيرات فرعية مماثلة إلى اليسار وإلى يمين عامل التشغيل "==": zero_state_id_ == zero_state_id_ LastBlock.cpp 66

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

ماكرو غير آمن

 namespace td { namespace detail { [[noreturn]] void process_check_error(const char *message, const char *file, int line); } // namespace detail } #define CHECK(condition) \ if (!(condition)) { \ ::td::detail::process_check_error(#condition, __FILE__, __LINE__); \ } void BlockDb::get_block_handle(BlockIdExt id, ....) { if (!id.is_valid()) { promise.set_error(....); return; } CHECK(id.is_valid()); // <= .... } 

رسالة تشخيص PVS-Studio: V581 التعبيرات الشرطية لعبارات "if" الموجودة بجانب بعضها البعض متطابقة. خطوط التحقق: 80 ، 84. blockdb.cpp 84

لن يتم تنفيذ الشرط الموجود داخل الماكرو CHECK نظرًا لأنه قد تم التحقق منه بالفعل بواسطة العبارة if السابقة.

يوجد أيضًا خطأ آخر موجود هنا: الماكرو CHECK غير آمن نظرًا لأن الشرط الموجود داخله غير ملفوف في بنية do {...} بينما (0) . هناك حاجة إلى مثل هذا التغليف لتجنب الاصطدام مع الظروف الأخرى في الفرع الآخر . بمعنى آخر ، لن تعمل التعليمة البرمجية التالية كما هو متوقع:

 if (X) CHECK(condition) else foo(); 

التحقق من متغير موقع

 class Slice { .... char operator[](size_t i) const; .... }; td::Result<int> CellSerializationInfo::get_bits(td::Slice cell) const { .... int last = cell[data_offset + data_len - 1]; if (!last || last == 0x80) { // <= return td::Status::Error("overlong encoding"); } .... } 

رسالة تشخيص PVS-Studio: V560 جزء من التعبير الشرطي خطأ دائمًا: last == 0x80. boc.cpp 78

لن يتم تنفيذ الجزء الثاني من الشرط أبدًا لأن النوع char يتم توقيعه في هذه الحالة. عند تخصيص قيمة لمتغير type int ، ستحدث امتداد الإشارة ، لذلك ستظل قيمها ضمن النطاق [-128 ، 127] ، وليس [0 ، 256].

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

تحول في الاتجاه الصحيح رقم سالب

 template <class Tr> bool AnyIntView<Tr>::export_bits_any(....) const { .... int mask = (-0x100 >> offs) & 0xff; .... } 

رسالة تشخيص PVS-Studio: V610 سلوك غير محدد. تحقق من مشغل التحول '>>'. المعامل الأيسر '-0x100' سلبي. bigint.hpp 1925

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

تحقق خالية بعد الجديد

 CellBuilder* CellBuilder::make_copy() const { CellBuilder* c = new CellBuilder(); if (!c) { // <= throw CellWriteError(); } .... } 

رسالة تشخيص PVS-Studio: V668 لا يوجد أي معنى في اختبار المؤشر "c" ضد قيمة خالية ، حيث تم تخصيص الذاكرة باستخدام عامل التشغيل "الجديد". سيتم إنشاء الاستثناء في حالة خطأ تخصيص الذاكرة. CellBuilder.cpp 531

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

الاختيار زائدة عن الحاجة

 int main(int argc, char* const argv[]) { .... if (!no_env) { const char* path = std::getenv("FIFTPATH"); if (path) { parse_include_path_set(path ? path : "/usr/lib/fift", source_include_path); } } .... } 

رسالة تشخيص PVS-Studio: التعبير عن المسار V547 دائمًا صحيح. fift-main.cpp 136

هذا المقتطف مأخوذ من إحدى الأدوات المساعدة الداخلية للمشروع. المشغل الثلاثي ضروري في هذه الحالة: الشرط الذي يتحقق منه محدد بالفعل من قبل العبارة if . يبدو أن المطورين نسوا إزالة عامل التشغيل الثلاثي هذا عندما قرروا تجاهل استخدام المسارات القياسية (لا يوجد على الأقل ذكر لتلك الموجودة في رسالة المساعدة).

متغير غير مستخدم

 bool Op::set_var_info_except(const VarDescrList& new_var_info, const std::vector<var_idx_t>& var_list) { if (!var_list.size()) { return set_var_info(new_var_info); } VarDescrList tmp_info{new_var_info}; tmp_info -= var_list; return set_var_info(new_var_info); // <= } 

رسالة تشخيص PVS-Studio: V1001 يتم تعيين متغير 'tmp_info' ولكن لا يتم استخدامه في نهاية الوظيفة. analyser.cpp 140

يبدو أن المطورين كانوا سيستخدمون متغيرًا يسمى tmp_info في السطر الأخير من هذه الوظيفة. إليك رمز نفس الوظيفة ولكن مع محددات المعلمات الأخرى:

 bool Op::set_var_info_except(VarDescrList&& new_var_info, const std::vector<var_idx_t>& var_list) { if (var_list.size()) { new_var_info -= var_list; // <= } return set_var_info(std::move(new_var_info)); } 

أكبر أم أقل من؟

 int compute_compare(const VarDescr& x, const VarDescr& y, int mode) { switch (mode) { case 1: // > return x.always_greater(y) ? 1 : (x.always_leq(y) ? 2 : 3); case 2: // = return x.always_equal(y) ? 1 : (x.always_neq(y) ? 2 : 3); case 3: // >= return x.always_geq(y) ? 1 : (x.always_less(y) ? 2 : 3); case 4: // < return x.always_less(y) ? 1 : (x.always_geq(y) ? 2 : 3); case 5: // <> return x.always_neq(y) ? 1 : (x.always_equal(y) ? 2 : 3); case 6: // >= return x.always_geq(y) ? 1 : (x.always_less(y) ? 2 : 3); case 7: // <=> return x.always_less(y) ? 1 : (x.always_equal(y) ? 2 : (x.always_greater(y) ? 4 : (x.always_leq(y) ? 3 : (x.always_geq(y) ? 6 : (x.always_neq(y) ? 5 : 7))))); default: return 7; } } 

رسالة تشخيص PVS-Studio: V1037 يقوم اثنان أو أكثر من فروع الحالات بتنفيذ نفس الإجراءات. خطوط الفحص: 639 ، 645 buildins.cpp 639

إذا قرأت بعناية ، فقد لاحظت أن هذا الرمز يفتقر إلى <= عملية. في الواقع ، هذه هي العملية التي ينبغي أن تتعامل معها القضية 6. يمكننا استنتاج ذلك من خلال النظر إلى موقعين. الأول هو رمز التهيئة:

 AsmOp compile_cmp_int(std::vector<VarDescr>& res, std::vector<VarDescr>& args, int mode) { .... if (x.is_int_const() && y.is_int_const()) { r.set_const(compute_compare(x.int_const, y.int_const, mode)); x.unused(); y.unused(); return push_const(r.int_const); } int v = compute_compare(x, y, mode); .... } void define_builtins() { .... define_builtin_func("_==_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 2)); define_builtin_func("_!=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 5)); define_builtin_func("_<_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 4)); define_builtin_func("_>_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 1)); define_builtin_func("_<=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 6)); define_builtin_func("_>=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 3)); define_builtin_func("_<=>_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 7)); .... } 

تحتوي الدالة ident_builtins ، كما ترون ، على استدعاء compile_cmp_int لـ <= عامل التشغيل مع تعيين معلمة الوضع على 6.

البقعة الثانية هي وظيفة compile_cmp_int نفسها ، والتي تسرد أسماء العمليات:

 AsmOp compile_cmp_int(std::vector<VarDescr>& res, std::vector<VarDescr>& args, int mode) { .... static const char* cmp_names[] = {"", "GREATER", "EQUAL", "GEQ", "LESS", "NEQ", "LEQ", "CMP"}; .... return exec_op(cmp_names[mode], 2); } 

يتوافق الفهرس 6 مع كلمة LEQ ، مما يعني "أقل أو يساوي".

إنه خطأ لطيف آخر من فئة الأخطاء الموجودة في وظائف المقارنة .

متنوع

 #define VM_LOG_IMPL(st, mask) \ LOG_IMPL_FULL(get_log_interface(st), ...., VERBOSITY_NAME(DEBUG), \ (get_log_mask(st) & mask) != 0, "") // <= 

رسالة تشخيص PVS-Studio: V1003 الماكرو "VM_LOG_IMPL" تعبير خطير. يجب أن تكون المعلمة 'قناع' محاطة بأقواس. log.h 23

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

استنتاج

تبين أن TON صغيرة جدًا ، لذلك هناك القليل من الأخطاء التي يمكن العثور عليها هناك ، والتي من المؤكد أن فريق المطورين Telegram سيحصل عليها. لكن الجميع يرتكبون أخطاء بين الحين والآخر ، حتى هؤلاء الرجال. أدوات تحليل الشفرات هي أدوات قوية قادرة على اكتشاف المواقع الخطرة في الكود المصدري في مراحل التطوير المبكرة حتى في أكثر قواعد الكود جودة ، لذلك لا تهملها. لا يُقصد بالتحليل الثابت أن يتم تشغيله من وقت لآخر ، ولكن يجب أن يكون جزءًا من عملية التطوير: " تقديم تحليل ثابت في العملية ، وليس فقط البحث عن الأخطاء باستخدامه ".