Geekly articles weekly

Menghitung Bug di Windows Calculator


Beberapa hari yang lalu, Microsoft membuat kode sumber Kalkulator Windows mereka tersedia untuk umum. Kalkulator adalah aplikasi yang secara tradisional dikirimkan dengan setiap versi Windows. Sejumlah proyek Microsoft menjadi open-source selama beberapa tahun terakhir, tetapi kali ini beritanya diliput bahkan oleh media non-IT pada hari pertama. Ya, ini adalah program yang populer namun kecil di C ++. Terlepas dari ukurannya, kami masih berhasil menemukan sejumlah fragmen yang mencurigakan dalam kodenya menggunakan analisa statis PVS-Studio.

Pendahuluan


Saya tidak berpikir kita perlu memperkenalkan Kalkulator karena Anda tidak akan menemukan pengguna Windows yang tidak tahu apa itu. Sekarang siapa pun dapat mengunduh kode sumber aplikasi dari GitHub dan menyarankan peningkatannya.

Fungsi berikut, misalnya, sudah menarik perhatian komunitas:

void TraceLogger::LogInvalidInputPasted(....) { if (!GetTraceLoggingProviderEnabled()) return; 

  LoggingFields fields{}; fields.AddString(L"Mode", NavCategory::GetFriendlyName(mode)->Data()); fields.AddString(L"Reason", reason); fields.AddString(L"PastedExpression", pastedExpression); fields.AddString(L"ProgrammerNumberBase", GetProgrammerType(...).c_str()); fields.AddString(L"BitLengthType", GetProgrammerType(bitLengthType).c_str()); LogTelemetryEvent(EVENT_NAME_INVALID_INPUT_PASTED, fields); }

Fungsi ini mencatat teks dari clipboard dan mengirimkannya ke server Microsoft. Namun, pos ini bukan tentang fungsi itu, tetapi Anda pasti akan melihat banyak cuplikan yang mencurigakan.

Kami menggunakan analisa statis PVS-Studio untuk memeriksa kode sumber Kalkulator. Karena tidak ditulis dalam standar C ++, banyak pembaca reguler kami meragukan cek semacam itu akan mungkin, tetapi kami melakukannya. Alat analisis mendukung C ++ / CLI dan C ++ / CX, dan meskipun beberapa diagnostik memang menghasilkan beberapa kesalahan positif, kami tidak menemukan masalah kritis yang akan menghambat pekerjaan PVS-Studio.

Sama seperti pengingat, jika Anda ketinggalan berita tentang kemampuan lain dari alat kami, PVS-Studio tidak hanya mendukung C dan C ++ tetapi C # dan Java juga.

Perbandingan string salah


Ekspresi V547 'm_resolvedName == L "en-US"' selalu salah. Untuk membandingkan string, Anda harus menggunakan fungsi wcscmp (). Kalkulator LocalizationSettings.h 180

 wchar_t m_resolvedName[LOCALE_NAME_MAX_LENGTH]; Platform::String^ GetEnglishValueFromLocalizedDigits(....) const { if (m_resolvedName == L"en-US") { return ref new Platform::String(localizedString.c_str()); } .... }

Saat melihat laporan penganalisa, saya mengurutkan peringatan dengan kode diagnostik dalam urutan menaik, dan yang ini, yang membuat contoh yang sangat jelas, adalah yang pertama dalam daftar.

Anda lihat, contoh di atas menunjukkan perbandingan string yang salah. Programmer sebenarnya membandingkan pointer daripada nilai string dengan membandingkan alamat array karakter dengan string literal. Pointer ini tidak pernah sama, jadi kondisinya selalu salah juga. Untuk perbandingan string yang benar, seseorang harus menggunakan fungsi wcscmp , misalnya.

Ngomong-ngomong, ketika saya sedang menulis artikel ini, array karakter m_resolvedName diperbaiki dalam file header dan menjadi string tipe std :: wstring , sehingga perbandingan dapat dilakukan dengan benar sekarang. Pada saat Anda akan membaca artikel ini, banyak bug lain mungkin akan diperbaiki juga berkat para penggemar dan ulasan seperti ini.

Kebocoran memori dalam kode asli


V773 Fungsi itu keluar tanpa melepaskan pointer 'temp'. Kebocoran memori dimungkinkan. CalcViewModel StandardCalculatorViewModel.cpp 529

 void StandardCalculatorViewModel::HandleUpdatedOperandData(Command cmdenum) { .... wchar_t* temp = new wchar_t[100]; .... if (commandIndex == 0) { delete [] temp; return; } .... length = m_selectedExpressionLastData->Length() + 1; if (length > 50) { return; } .... String^ updatedData = ref new String(temp); UpdateOperand(m_tokenPosition, updatedData); displayExpressionToken->Token = updatedData; IsOperandUpdatedUsingViewModel = true; displayExpressionToken->CommandIndex = commandIndex; }

Pointer temp merujuk ke array 100 elemen yang dialokasikan secara dinamis. Sayangnya, memori dilepaskan hanya di satu bagian dari fungsi, sementara sisanya berakhir dengan kebocoran memori. Ini tidak terlalu buruk, tetapi masih dianggap sebagai bug dalam kode C ++.

Pengecualian yang sulit dipahami


Kelas V702 harus selalu diturunkan dari std :: exception (dan yang sama) sebagai 'publik' (tidak ada kata kunci yang ditentukan, jadi kompiler secara default ke 'private'). CalcManager CalcException.h 4

 class CalcException : std::exception { public: CalcException(HRESULT hr) { m_hr = hr; } HRESULT GetException() { return m_hr; } private: HRESULT m_hr; };

Penganalisis telah mendeteksi kelas yang berasal dari kelas std :: exception menggunakan pengubah pribadi (yang default jika tidak ada pengubah lain yang ditentukan). Masalah dengan kode ini adalah bahwa pawang akan mengabaikan pengecualian tipe CalcException ketika mencoba untuk menangkap std :: generic generic karena warisan pribadi melarang konversi tipe implisit.

Hari yang terlewat


V719 Pernyataan beralih tidak mencakup semua nilai enum 'DateUnit': Hari. CalcViewModel DateCalculator.cpp 279

 public enum class _Enum_is_bitflag_ DateUnit { Year = 0x01, Month = 0x02, Week = 0x04, Day = 0x08 }; Windows::Globalization::Calendar^ m_calendar; DateTime DateCalculationEngine::AdjustCalendarDate(Windows::Foundation::DateTime date, DateUnit dateUnit, int difference) { m_calendar→SetDateTime(date); switch (dateUnit) { case DateUnit::Year: { .... m_calendar->AddYears(difference); m_calendar->ChangeCalendarSystem(currentCalendarSystem); break; } case DateUnit::Month: m_calendar->AddMonths(difference); break; case DateUnit::Week: m_calendar->AddWeeks(difference); break; } return m_calendar->GetDateTime(); }

Sangat mencurigakan bahwa pernyataan beralih tidak memiliki kasus DateUnit :: Hari . Karena itu, nilai hari tidak akan ditambahkan ke kalender (variabel m_calendar ), meskipun kalender tersebut memiliki metode AddDays .

Kasus mencurigakan lainnya dengan enumerasi lain:

  • V719 Pernyataan sakelar tidak mencakup semua nilai enum 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 109
  • V719 Pernyataan sakelar tidak mencakup semua nilai enum 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 204
  • V719 Pernyataan sakelar tidak mencakup semua nilai enum 'eANGLE_TYPE': ANGLE_RAD. CalcManager trans.cpp 276

Perbandingan angka nyata yang mencurigakan


V550 Perbandingan akurat yang aneh: rasio == ambang batas. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B) <Epsilon. Calculator AspectRatioTrigger.cpp 80

 void AspectRatioTrigger::UpdateIsActive(Size sourceSize) { double numerator, denominator; .... bool isActive = false; if (denominator > 0) { double ratio = numerator / denominator; double threshold = abs(Threshold); isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold))); } SetActive(isActive); }

Penganalisis menunjukkan rasio ekspresi mencurigakan == ambang batas . Variabel-variabel ini bertipe ganda dan, oleh karena itu, hampir tidak dapat dibandingkan secara tepat menggunakan operator biasa yang sama. Selain itu, nilai variabel rasio adalah hasil dari operasi divisi.

Kode seperti itu terlihat sangat aneh dalam aplikasi seperti Kalkulator. Saya menyertakan daftar lengkap peringatan jenis ini untuk berjaga-jaga:

  • V550 Perbandingan presisi yang aneh. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 752
  • V550 Perbandingan akurat yang aneh: stod (roundedString)! = 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 778
  • V550 Perbandingan presisi yang aneh. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 790
  • V550 Perbandingan akurat yang aneh: stod (roundedString)! = 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 820
  • V550 Perbandingan akurat yang aneh: conversionTable [m_toType] .ratio == 1.0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Perbandingan akurat yang aneh: conversionTable [m_toType] .offset == 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B) <Epsilon. CalcManager UnitConverter.cpp 980
  • V550 Perbandingan akurat yang aneh: returnValue! = 0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcManager UnitConverter.cpp 1000
  • V550 Perbandingan presisi yang aneh: sizeToUse! = 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 270
  • V550 Perbandingan presisi yang aneh: sizeToUse! = 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 289
  • V550 Perbandingan presisi yang aneh: sizeToUse! = 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 308
  • V550 Perbandingan presisi yang aneh: sizeToUse! = 0,0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B)> Epsilon. CalcViewModel LocalizationService.cpp 327
  • V550 Perbandingan akurat yang aneh: stod (stringToLocalize) == 0. Mungkin lebih baik menggunakan perbandingan dengan presisi yang ditentukan: fabs (A - B) <Epsilon. CalcViewModel UnitConverterViewModel.cpp 388

Urutan fungsi yang mencurigakan


V1020 Fungsi keluar tanpa memanggil fungsi 'TraceLogger :: GetInstance (). LogNewWindowCreationEnd'. Periksa baris: 396, 375. Aplikasi Kalkulator.xaml.cpp 396

 void App::OnAppLaunch(IActivatedEventArgs^ args, String^ argument) { .... if (!m_preLaunched) { auto newCoreAppView = CoreApplication::CreateNewView(); newCoreAppView->Dispatcher->RunAsync(....([....]() { TraceLogger::GetInstance().LogNewWindowCreationBegin(....); // <= Begin .... TraceLogger::GetInstance().LogNewWindowCreationEnd(....); // <= End })); } else { TraceLogger::GetInstance().LogNewWindowCreationBegin(....); // <= Begin ActivationViewSwitcher^ activationViewSwitcher; auto activateEventArgs = dynamic_cast<IViewSwitcherProvider^>(args); if (activateEventArgs != nullptr) { activationViewSwitcher = activateEventArgs->ViewSwitcher; } if (activationViewSwitcher != nullptr) { activationViewSwitcher->ShowAsStandaloneAsync(....); TraceLogger::GetInstance().LogNewWindowCreationEnd(....); // <= End TraceLogger::GetInstance().LogPrelaunchedAppActivatedByUser(); } else { TraceLogger::GetInstance().LogError(L"Null_ActivationViewSwitcher"); } } m_preLaunched = false; .... }

Diagnostik V1020 memeriksa blok kode dan mencari cabang dengan panggilan fungsi yang hilang menggunakan heuristik.

Cuplikan di atas berisi blok dengan panggilan ke fungsi LogNewWindowCreationBegin dan LogNewWindowCreationEnd . Ini diikuti oleh blok lain di mana fungsi LogNewWindowCreationEnd dipanggil hanya jika kondisi tertentu terpenuhi, yang terlihat sangat mencurigakan.

Tes tidak dapat diandalkan


V621 Pertimbangkan untuk memeriksa operator 'untuk'. Ada kemungkinan bahwa loop akan dieksekusi secara tidak benar atau tidak akan dieksekusi sama sekali. CalculatorUnitTests UnitConverterViewModelUnitTests.cpp 500

 public enum class NumbersAndOperatorsEnum { .... Add = (int) CM::Command::CommandADD, // 93 .... None = (int) CM::Command::CommandNULL, // 0 .... }; TEST_METHOD(TestButtonCommandFiresModelCommands) { .... for (NumbersAndOperatorsEnum button = NumbersAndOperatorsEnum::Add; button <= NumbersAndOperatorsEnum::None; button++) { if (button == NumbersAndOperatorsEnum::Decimal || button == NumbersAndOperatorsEnum::Negate || button == NumbersAndOperatorsEnum::Backspace) { continue; } vm.ButtonPressed->Execute(button); VERIFY_ARE_EQUAL(++callCount, mock->m_sendCommandCallCount); VERIFY_IS_TRUE(UCM::Command::None == mock->m_lastCommand); } .... }

Alat analisa telah mendeteksi loop untuk yang tidak berjalan sama sekali, yang berarti tes tidak mengeksekusi baik. Nilai awal dari tombol penghitung putaran (93) lebih besar dari nilai akhir (0) sejak awal.

V760 Dua blok teks yang identik ditemukan. Blok kedua dimulai dari baris 688. CalculatorUnitTests UnitConverterViewModelUnitTests.cpp 683

 TEST_METHOD(TestSwitchAndReselectCurrentlyActiveValueDoesNothing) { shared_ptr<UnitConverterMock> mock = make_shared<UnitConverterMock>(); VM::UnitConverterViewModel vm(mock); const WCHAR * vFrom = L"1", *vTo = L"234"; vm.UpdateDisplay(vFrom, vTo); vm.Value2Active = true; // Establish base condition VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount); vm.Value2Active = true; VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount); VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount); }

Tes mencurigakan lainnya. Penganalisis telah mendeteksi dua fragmen kode identik yang segera dieksekusi satu demi satu. Sepertinya kode ini ditulis menggunakan teknik salin-tempel dan programmer lupa untuk memodifikasi salinan.

V601 Nilai 'salah' secara implisit dilemparkan ke tipe integer. Periksa argumen kedua. CalculatorUnitTests CalcInputTest.cpp 352

 Rational CalcInput::ToRational(uint32_t radix, int32_t precision) { .... } TEST_METHOD(ToRational) { .... auto rat = m_calcInput.ToRational(10, false); .... }

Fungsi ToRational disebut dengan nilai Boolean false , sedangkan parameter yang sesuai adalah tipe int32_t dan disebut presisi .

Saya memutuskan untuk melacak nilai kode dan melihat bahwa itu kemudian diteruskan ke fungsi StringToRat :

 PRAT StringToRat(...., int32_t precision) { .... }

dan kemudian ke StringToNumber :

 PNUMBER StringToNumber(...., int32_t precision) { .... stripzeroesnum(pnumret, precision); .... }

Inilah tubuh fungsi target:

 bool stripzeroesnum(_Inout_ PNUMBER pnum, long starting) { MANTTYPE *pmant; long cdigits; bool fstrip = false; pmant=pnum->mant; cdigits=pnum->cdigit; if ( cdigits > starting ) // <= { pmant += cdigits - starting; cdigits = starting; } .... }

Variabel presisi sekarang bernama starting dan berpartisipasi dalam ekspresi cdigits> starting , yang sangat mencurigakan karena false dilewatkan sebagai nilai asli.

Redundansi


V560 Bagian dari ekspresi kondisional selalu benar: NumbersAndOperatorsEnum :: Tidak ada! = Op. CalcViewModel UnitConverterViewModel.cpp 991

 void UnitConverterViewModel::OnPaste(String^ stringToPaste, ViewMode mode) { .... NumbersAndOperatorsEnum op = MapCharacterToButtonId(*it, canSendNegate); if (NumbersAndOperatorsEnum::None != op) // <= { .... if (NumbersAndOperatorsEnum::None != op && // <= NumbersAndOperatorsEnum::Negate != op) { .... } .... } .... }

Variabel op sudah dibandingkan dengan nilai NumbersAndOperatorsEnum :: Tidak ada , sehingga pemeriksaan duplikat dapat dihapus.

V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A && B) || Ekspresi (! A &&! B) 'sama dengan ekspresi' bool (A) == bool (B) '. Calculator Calculator.xaml.cpp 239

 void Calculator::AnimateCalculator(bool resultAnimate) { if (App::IsAnimationEnabled()) { m_doAnimate = true; m_resultAnimate = resultAnimate; if (((m_isLastAnimatedInScientific && IsScientific) || (!m_isLastAnimatedInScientific && !IsScientific)) && ((m_isLastAnimatedInProgrammer && IsProgrammer) || (!m_isLastAnimatedInProgrammer && !IsProgrammer))) { this->OnStoryboardCompleted(nullptr, nullptr); } } }

Ekspresi bersyarat besar ini awalnya 218 karakter, tetapi saya membaginya menjadi beberapa baris untuk tujuan demonstrasi. Ini dapat ditulis ulang menjadi versi yang jauh lebih pendek dan, yang paling penting, lebih jelas:

 if ( m_isLastAnimatedInScientific == IsScientific && m_isLastAnimatedInProgrammer == IsProgrammer) { this->OnStoryboardCompleted(nullptr, nullptr); }

V524 Aneh bahwa tubuh fungsi 'ConvertBack' sepenuhnya setara dengan tubuh fungsi 'Konversi'. Kalkulator BooleanNegationConverter.cpp 24

 Object^ BooleanNegationConverter::Convert(....) { (void) targetType; // Unused parameter (void) parameter; // Unused parameter (void) language; // Unused parameter auto boxedBool = dynamic_cast<Box<bool>^>(value); auto boolValue = (boxedBool != nullptr && boxedBool->Value); return !boolValue; } Object^ BooleanNegationConverter::ConvertBack(....) { (void) targetType; // Unused parameter (void) parameter; // Unused parameter (void) language; // Unused parameter auto boxedBool = dynamic_cast<Box<bool>^>(value); auto boolValue = (boxedBool != nullptr && boxedBool->Value); return !boolValue; }

Penganalisa telah mendeteksi dua fungsi yang diimplementasikan secara identik. Seperti yang disarankan oleh nama mereka, Convert dan ConvertBack , mereka seharusnya melakukan hal-hal yang berbeda, tetapi para pengembang harus lebih tahu.

Kesimpulan


Saya kira setiap proyek Microsoft yang dibuat open-source memberi kami kesempatan untuk menunjukkan pentingnya analisis statis - bahkan pada proyek sekecil Kalkulator. Perusahaan besar, seperti Microsoft, Google, Amazon, dll., Mempekerjakan banyak pengembang berbakat, tetapi mereka masih manusia membuat kesalahan. Alat analisis statis adalah salah satu cara terbaik untuk membantu tim pengembang mana pun meningkatkan kualitas produk mereka.

Selamat mengunduh PVS-Studio dan cobalah "Kalkulator" Anda sendiri. :-)

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

More articles:


All Articles