Geekly articles weekly

LibreOffice: mimpi buruk akuntan


LibreOffice adalah suite kantor yang kuat yang gratis untuk penggunaan pribadi, pendidikan, dan komersial. Pengembangnya membuat produk yang luar biasa, yang di banyak daerah digunakan sebagai alternatif untuk Microsoft Office. Tim PVS-Studio selalu tertarik untuk melihat kode proyek-proyek terkenal tersebut dan berusaha menemukan kesalahan di dalamnya. Kali ini mudah dilakukan. Proyek ini mengandung banyak kesalahan yang dapat menyebabkan masalah serius. Artikel ini akan membahas beberapa cacat menarik yang ditemukan dalam kode.

Pendahuluan


LibreOffice adalah proyek C ++ yang sangat besar. Mempertahankan proyek sebesar ini adalah tugas yang sulit bagi tim pengembangan. Dan, sayangnya, orang mendapat kesan bahwa kualitas kode LibreOffice gagal membayar perhatian yang cukup.

Di satu sisi, proyek ini sangat besar, tidak setiap alat analisis statis atau dinamis dapat menangani analisis file kode sumber 13k. Begitu banyak file yang terlibat dalam membangun suite kantor bersama dengan perpustakaan pihak ketiga. Repositori LibreOffice utama menyimpan sekitar 8k file kode sumber. Jumlah kode ini menciptakan masalah tidak hanya untuk pengembang:


Di sisi lain, proyek ini memiliki banyak pengguna dan perlu menemukan dan memperbaiki kesalahan sebanyak mungkin. Setiap kesalahan dapat merugikan ratusan dan ribuan pengguna. Oleh karena itu, ukuran besar basis kode tidak boleh menjadi alasan untuk menolak menggunakan alat tertentu yang dapat mendeteksi kesalahan. Saya pikir pembaca sudah menebak bahwa kita berbicara tentang analisa kode statis :).

Ya, penggunaan analisis statis tidak menjamin tidak adanya kesalahan dalam proyek. Namun, alat seperti PVS-Studio dapat menemukan sejumlah besar kesalahan pada tahap pengembangan dan dengan demikian mengurangi jumlah pekerjaan yang terkait dengan debugging dan dukungan proyek.

Mari kita lihat apa yang Anda temukan menarik dalam kode sumber LibreOffice jika Anda mengambil penganalisa kode statis PVS-Studio . Kemungkinan untuk menjalankan analisa sangat luas: Windows, Linux, macOS. Untuk menulis ulasan ini, laporan PVS-Studio digunakan, dibuat selama analisis proyek pada Windows.

Perubahan sejak cek terakhir pada 2015




Pada bulan Maret 2015, analisis pertama LibreOffice (" Verifikasi proyek LibreOffice ") dilakukan menggunakan PVS-Studio. Sejak itu, office suite telah berkembang pesat sebagai produk, tetapi di dalamnya juga mengandung banyak kesalahan. Dan beberapa pola kesalahan tidak berubah sama sekali sejak itu. Di sini, misalnya, adalah kesalahan dari artikel pertama:

V656 Variabel 'aVRP', 'aVPN' diinisialisasi melalui panggilan ke fungsi yang sama. Mungkin salah atau kode tidak dioptimalkan. Pertimbangkan untuk memeriksa ekspresi 'rSceneCamera.GetVRP ()'. Periksa baris: 177, 178. viewcontactofe3dscene.cxx 178

void ViewContactOfE3dScene::createViewInformation3D(....) { .... const basegfx::B3DPoint aVRP(rSceneCamera.GetVRP()); const basegfx::B3DVector aVPN(rSceneCamera.GetVRP()); // <= const basegfx::B3DVector aVUV(rSceneCamera.GetVUV()); .... }

Bug ini telah diperbaiki, tetapi inilah yang ditemukan dalam versi terbaru dari kode:

V656 Variabel 'aSdvURL', 'aStrURL' diinisialisasi melalui panggilan ke fungsi yang sama. Mungkin salah atau kode tidak dioptimalkan. Pertimbangkan untuk memeriksa ekspresi 'pThm-> GetSdvURL ()'. Periksa baris: 658, 659. gallery1.cxx 659

 const INetURLObject& GetThmURL() const { return aThmURL; } const INetURLObject& GetSdgURL() const { return aSdgURL; } const INetURLObject& GetSdvURL() const { return aSdvURL; } const INetURLObject& GetStrURL() const { return aStrURL; } bool Gallery::RemoveTheme( const OUString& rThemeName ) { .... INetURLObject aThmURL( pThm->GetThmURL() ); INetURLObject aSdgURL( pThm->GetSdgURL() ); INetURLObject aSdvURL( pThm->GetSdvURL() ); INetURLObject aStrURL( pThm->GetSdvURL() ); // <= .... }

Seperti yang mungkin telah Anda perhatikan, nama fungsi majemuk halus masih menjadi sumber kesalahan.

Contoh lain yang menarik dari kode lama:

V656 Variabel 'nDragW', 'nDragH' diinisialisasi melalui panggilan ke fungsi yang sama. Mungkin salah atau kode tidak dioptimalkan. Pertimbangkan untuk memeriksa ekspresi 'rMSettings.GetStartDragWidth ()'. Periksa baris: 471, 472. winproc.cxx 472

 class VCL_DLLPUBLIC MouseSettings { .... long GetStartDragWidth() const; long GetStartDragHeight() const; .... } bool ImplHandleMouseEvent( .... ) { .... long nDragW = rMSettings.GetStartDragWidth(); long nDragH = rMSettings.GetStartDragWidth(); .... }

Sepotong kode ini benar-benar berisi bug yang sekarang sudah diperbaiki. Tetapi kesalahan dalam kode tidak semakin kecil ... Situasi serupa sekarang telah diidentifikasi:

V656 Variabel 'defaultZoomX', 'defaultZoomY' diinisialisasi melalui panggilan ke fungsi yang sama. Mungkin salah atau kode tidak dioptimalkan. Pertimbangkan untuk memeriksa ekspresi 'pViewData-> GetZoomX ()'. Periksa baris: 5673, 5674. gridwin.cxx 5674

 OString ScGridWindow::getCellCursor(....) const { .... SCCOL nX = pViewData->GetCurX(); SCROW nY = pViewData->GetCurY(); Fraction defaultZoomX = pViewData->GetZoomX(); Fraction defaultZoomY = pViewData->GetZoomX(); // <= .... }

Kesalahan dimasukkan ke dalam kode secara harfiah dengan analogi.

Jangan tertipu




V765 Ekspresi penugasan majemuk 'x - = x - ...' mencurigakan. Pertimbangkan memeriksanya untuk kemungkinan kesalahan. swdtflvr.cxx 3509

 bool SwTransferable::PrivateDrop(...) { .... if ( rSrcSh.IsSelFrameMode() ) { //Hack: fool the special treatment aSttPt -= aSttPt - rSrcSh.GetObjRect().Pos(); } .... }

Berikut ini adalah "Hack" yang menarik ditemukan menggunakan diagnostik V765 . Jika Anda menyederhanakan satu baris kode dengan komentar, Anda bisa mendapatkan hasil yang tidak terduga:

Langkah 1:

 aSttPt = aSttPt - (aSttPt - rSrcSh.GetObjRect().Pos());

Langkah 2:

 aSttPt = aSttPt - aSttPt + rSrcSh.GetObjRect().Pos();

Langkah ketiga

 aSttPt = rSrcSh.GetObjRect().Pos();

Lalu apa itu retas?

Contoh lain tentang topik ini:

V567 Modifikasi variabel 'nCount' tidak dilakukan relatif terhadap operasi lain pada variabel yang sama. Ini dapat menyebabkan perilaku yang tidak terdefinisi. stgio.cxx 214

 FatError EasyFat::Mark(....) { if( nCount > 0 ) { --nCount /= GetPageSize(); nCount++; } .... }

Eksekusi kode dalam situasi seperti itu mungkin tergantung pada kompiler dan standar bahasa. Mengapa tidak menulis ulang cuplikan kode ini dengan cara yang lebih sederhana, lebih jelas, dan lebih dapat diandalkan?

Bagaimana tidak menggunakan array dan vektor




Untuk beberapa alasan, seseorang membuat banyak kesalahan serupa ketika bekerja dengan array dan vektor. Mari kita lihat contoh-contoh ini.

V557 Array overrun dimungkinkan. Indeks 'nPageNum' menunjuk di luar batas array. pptx-epptooxml.cxx 1168

 void PowerPointExport::ImplWriteNotes(sal_uInt32 nPageNum) { .... // add slide implicit relation to notes if (mpSlidesFSArray.size() >= nPageNum) addRelation(mpSlidesFSArray[ nPageNum ]->getOutputStream(), oox::getRelationship(Relationship::NOTESSLIDE), OUStringBuffer() .append("../notesSlides/notesSlide") .append(static_cast<sal_Int32>(nPageNum) + 1) .append(".xml") .makeStringAndClear()); .... }

Indeks yang valid terakhir harus berupa nilai yang sama dengan ukuran () - 1 . Tetapi dalam fragmen kode ini, situasi diizinkan ketika indeks nPageNum dapat memiliki nilai mpSlidesFSArray.size () , karena ada array di luar batas dan bekerja dengan elemen yang terdiri dari "sampah".

V557 Array overrun dimungkinkan. Indeks 'mnSelectedMenu' menunjuk di luar batas array. checklistmenu.cxx 826

 void ScMenuFloatingWindow::ensureSubMenuNotVisible() { if (mnSelectedMenu <= maMenuItems.size() && maMenuItems[mnSelectedMenu].mpSubMenuWin && maMenuItems[mnSelectedMenu].mpSubMenuWin->IsVisible()) { maMenuItems[mnSelectedMenu].mpSubMenuWin->ensureSubMenuNotVisible(); } EndPopupMode(); }

Menariknya, dalam fragmen kode ini mereka menulis cek indeks lebih jelas, tetapi pada saat yang sama membuat kesalahan yang sama.

V557 Array overrun dimungkinkan. Indeks 'nXFIndex' menunjuk di luar batas array. xestyle.cxx 2613

 sal_Int32 XclExpXFBuffer::GetXmlStyleIndex( sal_uInt32 nXFIndex ) const { OSL_ENSURE( nXFIndex < maStyleIndexes.size(), "...." ); if( nXFIndex > maStyleIndexes.size() ) return 0; // should be caught/debugged via above assert; return maStyleIndexes[ nXFIndex ]; }

Dan kesalahan ini sangat menarik! Di makro debugging, mereka menulis cek indeks yang benar, dan di tempat lain mereka kembali membuat kesalahan, memungkinkan mereka untuk keluar dari array.

Sekarang pertimbangkan jenis kesalahan berbeda yang tidak terkait dengan indeks.

V554 Penggunaan shared_ptr salah. Memori yang dialokasikan dengan 'baru [] akan dibersihkan menggunakan' hapus '. dx_vcltools.cxx 158

 struct RawRGBABitmap { sal_Int32 mnWidth; sal_Int32 mnHeight; std::shared_ptr< sal_uInt8 > mpBitmapData; }; RawRGBABitmap bitmapFromVCLBitmapEx( const ::BitmapEx& rBmpEx ) { .... // convert transparent bitmap to 32bit RGBA // ======================================== const ::Size aBmpSize( rBmpEx.GetSizePixel() ); RawRGBABitmap aBmpData; aBmpData.mnWidth = aBmpSize.Width(); aBmpData.mnHeight = aBmpSize.Height(); aBmpData.mpBitmapData.reset( new sal_uInt8[ 4*aBmpData.mnWidth *aBmpData.mnHeight ] ); .... }

Sepotong kode ini mengandung kesalahan yang mengarah ke perilaku program yang tidak ditentukan. Faktanya adalah bahwa memori dialokasikan dan dibebaskan dengan berbagai cara. Untuk benar-benar membebaskan memori, Anda harus mendeklarasikan bidang kelas seperti ini:

 std::shared_ptr< sal_uInt8[] > mpBitmapData;

Cara membuat macro dua kali




V568 Aneh bahwa argumen operator sizeof () adalah 'bTextFrame? aProps: ekspresi aShapeProps. wpscontext.cxx 134

 oox::core::ContextHandlerRef WpsContext::onCreateContext(....) { .... OUString aProps[] = { .... }; OUString aShapeProps[] = { .... }; for (std::size_t i = 0; i < SAL_N_ELEMENTS(bTextFrame ? aProps : aShapeProps); //1 ++i) if (oInsets[i]) xPropertySet->setPropertyValue((bTextFrame ? aProps : aShapeProps)[i], //2 uno::makeAny(*oInsets[i])); .... }

Sayangnya untuk banyak pengembang, argumen makro tidak berperilaku seperti argumen fungsi. Mengabaikan fakta ini sering menyebabkan kesalahan. Dalam kasus # 1 dan # 2, konstruksi yang hampir identik digunakan menggunakan operator ternary. Tetapi dalam kasus pertama - makro, yang kedua - fungsi. Namun, ini hanya masalah utama.

Dalam kasus # 1, penganalisa sebenarnya mendeteksi kode kesalahan berikut:

 for (std::size_t i = 0; i < (sizeof (bTextFrame ? aProps : aShapeProps) / sizeof ((bTextFrame ? aProps : aShapeProps)[0])); ++i)

Ini adalah loop kami dengan SAL_N_ELEMENTS makro. Ukuran operator tidak mengevaluasi ekspresi dalam operator ternary. Dalam hal ini, aritmatika dilakukan dengan ukuran pointer, yang hasilnya adalah nilai yang jauh dari ukuran sebenarnya dari array yang ditunjukkan. Perhitungan nilai yang salah juga dipengaruhi oleh kedalaman bit aplikasi.

Tetapi ternyata ada 2 makro SAL_N_ELEMENTS ! Yaitu preprocessor membuka makro yang salah, bagaimana ini bisa terjadi? Definisi makro dan komentar pengembang akan membantu kami:

 #ifndef SAL_N_ELEMENTS # if defined(__cplusplus) && ( defined(__GXX_EXPERIMENTAL_CXX0X__) || __cplusplus >= 201103L ) /* * Magic template to calculate at compile time the number of elements * in an array. Enforcing that the argument must be a array and not * a pointer, eg * char *pFoo="foo"; * SAL_N_ELEMENTS(pFoo); * fails while * SAL_N_ELEMENTS("foo"); * or * char aFoo[]="foo"; * SAL_N_ELEMENTS(aFoo); * pass * * Unfortunately if arr is an array of an anonymous class then we need * C++0x, ie see * http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#757 */ template <typename T, size_t S> char (&sal_n_array_size( T(&)[S] ))[S]; # define SAL_N_ELEMENTS(arr) (sizeof(sal_n_array_size(arr))) # else # define SAL_N_ELEMENTS(arr) (sizeof (arr) / sizeof ((arr)[0])) # endif #endif

Versi lain dari makro berisi fungsi templat yang aman, tetapi ada yang tidak beres:

  1. Makro aman tidak termasuk dalam kode;
  2. Masih tidak mungkin menggunakan makro lain, karena instantiasi fungsi templat yang berhasil dilakukan hanya jika array dengan ukuran yang sama dipindahkan ke operator ternary. Dan dalam hal ini penggunaan makro semacam itu kehilangan artinya.

Kesalahan ketik dan salin-tempel




V1013 Subekspresi mencurigakan f1.Pitch == f2.CharSet dalam urutan perbandingan yang serupa. xmldlg_export.cxx 1251

 inline bool equalFont( Style const & style1, Style const & style2 ) { awt::FontDescriptor const & f1 = style1._descr; awt::FontDescriptor const & f2 = style2._descr; return ( f1.Name == f2.Name && f1.Height == f2.Height && f1.Width == f2.Width && f1.StyleName == f2.StyleName && f1.Family == f2.Family && f1.CharSet == f2.CharSet && // <= f1.Pitch == f2.CharSet && // <= f1.CharacterWidth == f2.CharacterWidth && f1.Weight == f2.Weight && f1.Slant == f2.Slant && f1.Underline == f2.Underline && f1.Strikeout == f2.Strikeout && f1.Orientation == f2.Orientation && bool(f1.Kerning) == bool(f2.Kerning) && bool(f1.WordLineMode) == bool(f2.WordLineMode) && f1.Type == f2.Type && style1._fontRelief == style2._fontRelief && style1._fontEmphasisMark == style2._fontEmphasisMark ); }

Kesalahannya adalah kandidat yang layak untuk memperbarui artikel " Jahat hidup dalam fungsi perbandingan " jika kita pernah memutuskan untuk memperbarui atau memperluasnya. Saya pikir probabilitas menemukan kesalahan seperti itu (lulus f2.Pitch ) saja sangat kecil. Apa yang kamu pikirkan

V501 Ada sub-ekspresi identik 'mpTable [ocArrayColSep]! = MpTable [eOp]' di sebelah kiri dan di sebelah kanan operator '&&'. formulacompiler.cxx 632

 void FormulaCompiler::OpCodeMap::putOpCode(....) { .... case ocSep: bPutOp = true; bRemoveFromMap = (mpTable[eOp] != ";" && mpTable[ocArrayColSep] != mpTable[eOp] && mpTable[ocArrayColSep] != mpTable[eOp]); break; .... }

Hasil dari penyalinan tanpa berpikir adalah suatu kode. Mungkin ekspresi kondisional hanya digandakan sekali lagi, tetapi masih dalam kode tidak ada tempat untuk ambiguitas seperti itu.

V517 Penggunaan pola 'jika (A) {...} else jika (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 781, 783. mysqlc_databasemetadata.cxx 781

 Reference<XResultSet> SAL_CALL ODatabaseMetaData::getColumns(....) { .... bool bIsCharMax = !xRow->wasNull(); if (sDataType.equalsIgnoreAsciiCase("year")) nColumnSize = sColumnType.copy(6, 1).toInt32(); else if (sDataType.equalsIgnoreAsciiCase("date")) // <= nColumnSize = 10; else if (sDataType.equalsIgnoreAsciiCase("date")) // <= nColumnSize = 8; else if (sDataType.equalsIgnoreAsciiCase("datetime") || sDataType.equalsIgnoreAsciiCase("timestamp")) nColumnSize = 19; else if (!bIsCharMax) nColumnSize = xRow->getShort(7); else nColumnSize = nCharMaxLen; .... }

Sebagai hasil dari menyalin ekspresi kondisional, kesalahan dibuat dalam kode karena nilai 8 untuk variabel nColumnSize tidak pernah disetel.

V523 Pernyataan 'lalu' sama dengan pernyataan 'lain'. svdpdf.hxx 146

 /// Transform the rectangle (left, right, top, bottom) by this Matrix. template <typename T> void Transform(....) { .... if (top > bottom) top = std::max(leftTopY, rightTopY); else top = std::min(leftTopY, rightTopY); if (top > bottom) bottom = std::max(leftBottomY, rightBottomY); // <= else bottom = std::max(leftBottomY, rightBottomY); // <= }

Di sini fungsi min () dan max () bingung. Tentunya, karena kesalahan ketik ini di antarmuka, ada yang anehnya diskalakan.

Siklus aneh




V533 Kemungkinan variabel yang salah sedang bertambah di dalam operator 'untuk'. Pertimbangkan untuk meninjau 'i'. javatypemaker.cxx 602

 void printConstructors(....) { .... for (std::vector< unoidl::SingleInterfaceBasedServiceEntity::Constructor:: Parameter >::const_iterator j(i->parameters.begin()); j != i->parameters.end(); ++i) { o << ", "; printType(o, options, manager, j->type, false); if (j->rest) { o << "..."; } o << ' ' << codemaker::java::translateUnoToJavaIdentifier( u2b(j->name), "param"); } .... }

Ekspresi ++ i di loop terlihat sangat mencurigakan. Mungkin seharusnya ada ++ j di sana .

V756 Counter 'nIndex2' tidak digunakan di dalam loop bersarang. Pertimbangkan untuk memeriksa penggunaan penghitung 'nIndex'. treex.cxx 34

 SAL_IMPLEMENT_MAIN_WITH_ARGS(argc, argv) { OString sXHPRoot; for (int nIndex = 1; nIndex != argc; ++nIndex) { if (std::strcmp(argv[nIndex], "-r") == 0) { sXHPRoot = OString( argv[nIndex + 1] ); for( int nIndex2 = nIndex+3; nIndex2 < argc; nIndex2 = nIndex2 + 2 ) { argv[nIndex-3] = argv[nIndex-1]; argv[nIndex-2] = argv[nIndex]; } argc = argc - 2; break; } } common::HandledArgs aArgs; if( !common::handleArguments(argc, argv, aArgs) ) { WriteUsage(); return 1; } .... }

Ada beberapa jenis kesalahan di bagian dalam untuk loop. Karena variabel nIndex tidak berubah, dua elemen array yang sama ditimpa pada setiap iterasi. Kemungkinan besar, alih-alih nIndex , variabel nIndex2 seharusnya digunakan di mana-mana .

V1008 Pertimbangkan untuk memeriksa operator 'untuk'. Tidak lebih dari satu iterasi dari loop akan dilakukan. diagramhelper.cxx 292

 void DiagramHelper::setStackMode( const Reference< XDiagram > & xDiagram, StackMode eStackMode ) { .... sal_Int32 nMax = aChartTypeList.getLength(); if( nMax >= 1 ) nMax = 1; for( sal_Int32 nT = 0; nT < nMax; ++nT ) { uno::Reference< XChartType > xChartType( aChartTypeList[nT] ); .... } .... }

Loop untuk sengaja dibatasi hingga 1 iterasi. Tidak jelas mengapa ini dilakukan dengan cara ini.

V612 Suatu 'pengembalian' tanpa syarat dalam satu lingkaran. pormulti.cxx 891

 SwTextAttr const* MergedAttrIterMulti::NextAttr(....) { .... SwpHints const*const pHints(m_pNode->GetpSwpHints()); if (pHints) { while (m_CurrentHint < pHints->Count()) { SwTextAttr const*const pHint(pHints->Get(m_CurrentHint)); ++m_CurrentHint; rpNode = m_pNode; return pHint; } } return nullptr; .... }

Contoh loop aneh yang lebih sederhana dari satu iterasi, yang lebih baik untuk menulis ulang ke pernyataan kondisional.

Beberapa tempat lagi:

  • V612 Suatu 'pengembalian' tanpa syarat dalam satu lingkaran. txtfrm.cxx 144
  • V612 Suatu 'pengembalian' tanpa syarat dalam satu lingkaran. txtfrm.cxx 202
  • V612 Suatu 'pengembalian' tanpa syarat dalam satu lingkaran. txtfrm.cxx 279

Kondisi aneh




V637 Dua kondisi yang berlawanan ditemui. Kondisi kedua selalu salah. Periksa baris: 281, 285. authfld.cxx 281

 sal_uInt16 SwAuthorityFieldType::GetSequencePos(sal_IntPtr nHandle) { .... SwTOXSortTabBase* pOld = aSortArr[i].get(); if(*pOld == *pNew) { //only the first occurrence in the document //has to be in the array if(*pOld < *pNew) pNew.reset(); else // remove the old content aSortArr.erase(aSortArr.begin() + i); break; } .... }

Penganalisa menemukan perbandingan yang saling bertentangan. Jelas ada yang salah dengan bagian kode ini.

Kode yang sama terlihat di tempat ini:

  • V637 Dua kondisi yang berlawanan ditemui. Kondisi kedua selalu salah. Periksa baris: 1827, 1829. doctxm.cxx 1827

V590 Pertimbangkan untuk memeriksa ungkapan ini. Ekspresi berlebihan atau mengandung kesalahan cetak. fileurl.cxx 55

 OUString convertToFileUrl(char const * filename, ....) { .... if ((filename[0] == '.') || (filename[0] != SEPARATOR)) { .... } .... }

Masalah dengan fragmen kode di atas adalah bahwa ekspresi kondisional pertama tidak mempengaruhi hasil keseluruhan ekspresi.

Berdasarkan kesalahan seperti itu, saya bahkan menulis artikel teoritis: " Ekspresi logis dalam C / C ++. Bagaimana profesional salah ."

V590 Pertimbangkan untuk memeriksa ungkapan ini. Ekspresi berlebihan atau mengandung kesalahan cetak. unoobj.cxx 1895

 uno::Sequence< beans::PropertyState > SwUnoCursorHelper::GetPropertyStates(....) { .... if (((SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION == eCaller) || (SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION_TOLERANT == eCaller)) && pEntry->nWID < FN_UNO_RANGE_BEGIN && pEntry->nWID > FN_UNO_RANGE_END && pEntry->nWID < RES_CHRATR_BEGIN && pEntry->nWID > RES_TXTATR_END ) { pStates[i] = beans::PropertyState_DEFAULT_VALUE; } .... }

Saya tidak segera mengerti apa masalah dari kondisi ini, oleh karena itu, sebuah fragmen kode yang diperluas ditulis dari file yang telah diproses:

 if (((SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION == eCaller) || (SW_PROPERTY_STATE_CALLER_SWX_TEXT_PORTION_TOLERANT == eCaller)) && pEntry->nWID < (20000 + 1600) && pEntry->nWID > ((20000 + 2400) + 199) && pEntry->nWID < 1 && pEntry->nWID > 63 ) { pStates[i] = beans::PropertyState_DEFAULT_VALUE; }

Kebetulan bahwa tidak ada satu nomor pun yang secara bersamaan termasuk dalam 4 rentang yang ditentukan dalam kondisi dengan angka. Pengembang melakukan kesalahan.

V590 Pertimbangkan untuk memeriksa ekspresi '* pData <= MAXLEVEL && * * pData <= 9'. Ekspresi berlebihan atau mengandung kesalahan cetak. ww8par2.cxx 756

 const sal_uInt8 MAXLEVEL = 10; void SwWW8ImplReader::Read_ANLevelNo(....) { .... // Range WW:1..9 -> SW:0..8 no bullets / numbering if (*pData <= MAXLEVEL && *pData <= 9) { .... } else if( *pData == 10 || *pData == 11 ) { // remember type, the rest happens at Sprm 12 m_xStyles->mnWwNumLevel = *pData; } .... }

Karena kenyataan bahwa kondisi pertama menggunakan konstanta dengan nilai 10 , kondisi tersebut ternyata mubazir. Potongan kode ini dapat ditulis ulang sebagai berikut:

 if (*pData <= 9) { .... } else if( *pData == 10 || *pData == 11 ) { .... }

Tapi mungkin ada masalah lain dalam kode yang disajikan.

V757 Ada kemungkinan bahwa variabel yang salah dibandingkan dengan nullptr setelah konversi jenis menggunakan 'dynamic_cast'. Periksa baris: 2709, 2710. menu.cxx 2709

 void PopupMenu::ClosePopup(Menu* pMenu) { MenuFloatingWindow* p = dynamic_cast<MenuFloatingWindow*>(ImplGetWindow()); PopupMenu *pPopup = dynamic_cast<PopupMenu*>(pMenu); if (p && pMenu) p->KillActivePopup(pPopup); }

Kemungkinan besar, kondisi tersebut mengandung kesalahan. Itu perlu untuk memeriksa pointer p dan pPopup .

V668 Tidak ada gunanya menguji pointer 'm_pStream' terhadap null, karena memori dialokasikan menggunakan operator 'baru'. Pengecualian akan dihasilkan jika terjadi kesalahan alokasi memori. zipfile.cxx 408

 ZipFile::ZipFile(const std::wstring &FileName) : m_pStream(nullptr), m_bShouldFree(true) { m_pStream = new FileStream(FileName.c_str()); if (m_pStream && !isZipStream(m_pStream)) { delete m_pStream; m_pStream = nullptr; } }

Penganalisa mendeteksi situasi di mana nilai pointer dikembalikan oleh operator baru dibandingkan dengan nol. Menurut standar bahasa C ++, jika alokasi memori tidak memungkinkan, operator baru akan melempar pengecualian std :: bad_alloc . Hanya 45 tempat seperti itu ditemukan dalam proyek LibreOffice, yang sangat kecil untuk volume kode seperti itu. Tapi tetap saja, ini bisa menimbulkan masalah bagi pengguna. Pengembang harus menghapus cek yang tidak perlu atau membuat objek dengan cara ini:

 m_pStream = new (std::nothrow) FileStream(FileName.c_str());

V728 Pemeriksaan berlebihan dapat disederhanakan. The '(A &&! B) || Ekspresi (! A && B) 'sama dengan ekspresi' bool (A)! = Bool (B) '. toolbox2.cxx 1042

 void ToolBox::SetItemImageMirrorMode( sal_uInt16 nItemId, bool bMirror ) { ImplToolItems::size_type nPos = GetItemPos( nItemId ); if ( nPos != ITEM_NOTFOUND ) { ImplToolItem* pItem = &mpData->m_aItems[nPos]; if ((pItem->mbMirrorMode && !bMirror) || // <= (!pItem->mbMirrorMode && bMirror)) // <= { .... } } }

Dahulu sekali, diagnostik V728 diperluas ke kasus-kasus yang, kemungkinan besar, tidak salah, tetapi menyulitkan kode. Dan dalam kode yang kompleks, kesalahan cepat atau lambat masih dilakukan.

Kondisi ini disederhanakan sebagai berikut:

 if (pItem->mbMirrorMode != bMirror) { .... }

Sekitar 60 struktur serupa ditemukan dalam proyek ini, beberapa di antaranya sangat, sangat besar. Para penulis proyek lebih baik untuk berkenalan dengan laporan lengkap dari alat analisa PVS-Studio.

Masalah keamanan




V523 Pernyataan 'lalu' sama dengan pernyataan 'lain'. docxattributeoutput.cxx 1571

 void DocxAttributeOutput::DoWritePermissionTagEnd( const OUString & permission) { OUString permissionIdAndName; if (permission.startsWith("permission-for-group:", &permissionIdAndName)) { const sal_Int32 sparatorIndex = permissionIdAndName.indexOf(':'); const OUString permissionId = permissionIdAndName.copy(....); const OString rId = OUStringToOString(....).getStr(); m_pSerializer->singleElementNS(XML_w, XML_permEnd, FSNS(XML_w, XML_id), rId.getStr(), FSEND); } else { const sal_Int32 sparatorIndex = permissionIdAndName.indexOf(':'); const OUString permissionId = permissionIdAndName.copy(....); const OString rId = OUStringToOString(....).getStr(); m_pSerializer->singleElementNS(XML_w, XML_permEnd, FSNS(XML_w, XML_id), rId.getStr(), FSEND); } }

Sepotong kode yang sangat besar disalin di sini. Untuk fungsi yang mengubah hak-hak tertentu, masalah yang diidentifikasi terlihat sangat mencurigakan.

V1001 Variabel 'DL' diberikan tetapi tidak digunakan pada akhir fungsi. cipher.cxx 811

 static void BF_updateECB( CipherContextBF *ctx, rtlCipherDirection direction, const sal_uInt8 *pData, sal_uInt8 *pBuffer, sal_Size nLength) { CipherKeyBF *key; sal_uInt32 DL, DR; key = &(ctx->m_key); if (direction == rtl_Cipher_DirectionEncode) { RTL_CIPHER_NTOHL64(pData, DL, DR, nLength); BF_encode(key, &DL, &DR); RTL_CIPHER_HTONL(DL, pBuffer); RTL_CIPHER_HTONL(DR, pBuffer); } else { RTL_CIPHER_NTOHL(pData, DL); RTL_CIPHER_NTOHL(pData, DR); BF_decode(key, &DL, &DR); RTL_CIPHER_HTONL64(DL, DR, pBuffer, nLength); } DL = DR = 0; }

Variabel DL dan DR dibatalkan oleh tugas sederhana di akhir fungsi dan tidak lagi digunakan. Untuk kompiler, ini adalah alasan untuk menghapus perintah untuk optimasi.

Untuk membersihkan variabel di aplikasi Windows dengan benar, Anda dapat menulis ulang kode dengan cara ini:

 RtlSecureZeroMemory(&DL, sizeof(DL)); RtlSecureZeroMemory(&DR, sizeof(DR));

Tetapi untuk LibreOffice, solusi lintas platform diperlukan di sini.

Peringatan serupa dari fungsi lain:
  • V1001 Variabel 'DL' diberikan tetapi tidak digunakan pada akhir fungsi. cipher.cxx 860

V764 Kemungkinan urutan argumen yang salah diteruskan ke fungsi 'queryStream': 'rUri' dan 'rPassword'. tdoc_storage.cxx 271

 css::uno::Reference< css::io::XStream > queryStream( const css::uno::Reference< css::embed::XStorage > & xParentStorage, const OUString & rPassword, const OUString & rUri, StorageAccessMode eMode, bool bTruncate ); uno::Reference< io::XOutputStream > StorageElementFactory::createOutputStream( const OUString & rUri, const OUString & rPassword, bool bTruncate ) { .... uno::Reference< io::XStream > xStream = queryStream( xParentStorage, rUri, rPassword, READ_WRITE_CREATE, bTruncate ); .... }

Harap dicatat bahwa dalam fungsi queryStream , dalam daftar parameter, rPassword pertama dan kemudian rUri . Ada tempat di kode di mana argumen yang sesuai dicampur ketika fungsi ini dipanggil.

V794 Operator penugasan harus dilindungi dari kasus 'this == & rToBeCopied'. hommatrixtemplate.hxx 121

 ImplHomMatrixTemplate& operator=(....) { // complete initialization using copy for(sal_uInt16 a(0); a < (RowSize - 1); a++) { memcpy(&maLine[a], &rToBeCopied.maLine[a], sizeof(....)); } if(rToBeCopied.mpLine) { mpLine.reset( new ImplMatLine< RowSize >(....) ); } return *this; }

Kasus ini lebih tentang penulisan kode aman. Tidak ada cek dalam pernyataan salin untuk menetapkan objek untuk dirinya sendiri. Secara total, proyek ini memiliki sekitar 30 implementasi seperti operator salinan.

Kesalahan dengan SysAllocString




V649 Ada dua pernyataan 'jika' dengan ekspresi kondisional yang identik. Pernyataan 'jika' pertama berisi pengembalian fungsi. Ini berarti bahwa pernyataan 'jika' yang kedua tidak masuk akal. Periksa baris: 125, 137. acctable.cxx 137

 STDMETHODIMP CAccTable::get_columnDescription(long column, BSTR * description) { SolarMutexGuard g; ENTER_PROTECTED_BLOCK // #CHECK# if(description == nullptr) return E_INVALIDARG; // #CHECK XInterface# if(!pRXTable.is()) return E_FAIL; .... SAFE_SYSFREESTRING(*description); *description = SysAllocString(o3tl::toW(ouStr.getStr())); if(description==nullptr) // <= return E_FAIL; return S_OK; LEAVE_PROTECTED_BLOCK }

Fungsi SysAllocString () mengembalikan pointer yang bisa NULL . Sesuatu yang aneh ditulis oleh penulis kode ini. Pointer ke memori yang dialokasikan tidak dicentang, yang dapat menyebabkan masalah dalam program.

Peringatan serupa dari fungsi lain:

  • V649 Ada dua pernyataan 'jika' dengan ekspresi kondisional yang identik. Pernyataan 'jika' pertama berisi pengembalian fungsi. Ini berarti bahwa pernyataan 'jika' yang kedua tidak masuk akal. Periksa baris: 344, 356. acctable.cxx 356
  • V649 Ada dua pernyataan 'jika' dengan ekspresi kondisional yang identik. Pernyataan 'jika' pertama berisi pengembalian fungsi. Ini berarti bahwa pernyataan 'jika' yang kedua tidak masuk akal. Periksa baris: 213, 219. trvlfrm.cxx 219

V530 Nilai balik fungsi 'SysAllocString' harus digunakan. maccessible.cxx 1077

 STDMETHODIMP CMAccessible::put_accValue(....) { .... if(varChild.lVal==CHILDID_SELF) { SysAllocString(m_pszValue); m_pszValue=SysAllocString(szValue); return S_OK; } .... }

Hasil dari salah satu panggilan ke fungsi SysAllocString () tidak digunakan. Pengembang harus memperhatikan potongan kode ini.

Lain-lain


V716 Konversi tipe yang mencurigakan dalam pernyataan pengembalian: HRESULT yang dikembalikan, tetapi fungsi sebenarnya mengembalikan BOOL. maccessible.cxx 2649

 BOOL CMAccessible::get_IAccessibleFromXAccessible(....) { ENTER_PROTECTED_BLOCK // #CHECK# if(ppIA == nullptr) { return E_INVALIDARG; // <= } BOOL isGet = FALSE; if(g_pAgent) isGet = g_pAgent->GetIAccessibleFromXAccessible(....); if(isGet) return TRUE; else return FALSE; LEAVE_PROTECTED_BLOCK }

Salah satu cabang eksekusi dari fungsi mengembalikan nilai yang tipenya tidak cocok dengan jenis nilai balik fungsi. Tipe HRESULT memiliki format yang lebih kompleks daripada tipe BOOL dan dirancang untuk menyimpan status operasi.Misalnya, nilai E_INVALIDARG adalah 0x80070057L . Itu akan benar untuk menulis, misalnya, seperti ini:

 return FAILED(E_INVALIDARG);

Anda dapat membaca lebih lanjut tentang ini di dokumentasi diagnostik V716.

Beberapa tempat serupa:

  • V716 Konversi tipe mencurigakan dalam pernyataan kembali: HRESULT dikembalikan, tetapi fungsi sebenarnya mengembalikan BOOL. inprocembobj.cxx 1299
  • V716 Konversi tipe mencurigakan dalam pernyataan kembali: HRESULT dikembalikan, tetapi fungsi sebenarnya mengembalikan BOOL. maccessible.cxx 2660

V670 Anggota kelas yang tidak diinisialisasi 'm_aMutex' digunakan untuk menginisialisasi anggota 'm_aModifyListeners'. Ingat bahwa anggota diinisialisasi dalam urutan deklarasi mereka di dalam kelas. fmgridif.cxx 1033

 FmXGridPeer::FmXGridPeer(const Reference< XComponentContext >& _rxContext) :m_aModifyListeners(m_aMutex) ,m_aUpdateListeners(m_aMutex) ,m_aContainerListeners(m_aMutex) ,m_aSelectionListeners(m_aMutex) ,m_aGridControlListeners(m_aMutex) ,m_aMode( getDataModeIdentifier() ) ,m_nCursorListening(0) ,m_bInterceptingDispatch(false) ,m_xContext(_rxContext) { // Create must be called after this constructor m_pGridListener.reset( new GridListenerDelegator( this ) ); } class __declspec(dllexport) FmXGridPeer: public cppu::ImplInheritanceHelper<....> { .... ::comphelper::OInterfaceContainerHelper2 m_aModifyListeners, m_aUpdateListeners, m_aContainerListeners, m_aSelectionListeners, m_aGridControlListeners; .... protected: css::uno::Reference< css::uno::XComponentContext > m_xContext; ::osl::Mutex m_aMutex; .... };

Menurut standar bahasa, urutan inisialisasi anggota kelas dalam konstruktor terjadi dalam urutan deklarasi mereka di kelas. Dalam kasus kami, bidang m_aMutex akan diinisialisasi setelah berpartisipasi dalam inisialisasi lima bidang lain dari kelas.

Berikut ini bentuk konstruktor dari salah satu bidang kelas:

 OInterfaceContainerHelper2( ::osl::Mutex & rMutex );

Objek mutex dilewatkan dengan referensi. Dalam kasus ini, berbagai masalah dapat muncul: mulai dari mengakses memori yang tidak diinisialisasi hingga hilangnya perubahan objek berikutnya.

V763 Parameter 'nNativeNumberMode' selalu ditulis ulang di badan fungsi sebelum digunakan. calendar_jewish.cxx 286

 OUString SAL_CALL Calendar_jewish::getDisplayString(...., sal_Int16 nNativeNumberMode ) { // make Hebrew number for Jewish calendar nNativeNumberMode = NativeNumberMode::NATNUM2; if (nCalendarDisplayCode == CalendarDisplayCode::SHORT_YEAR) { sal_Int32 value = getValue(CalendarFieldIndex::YEAR) % 1000; return mxNatNum->getNativeNumberString(...., nNativeNumberMode ); } else return Calendar_gregorian::getDisplayString(...., nNativeNumberMode ); }

Argumen fungsi ditimpa karena berbagai alasan: melawan peringatan kompiler, kompatibilitas mundur, kruk, dll. Namun, salah satu solusi ini tidak baik, dan kodenya terlihat membingungkan.

Anda harus meninjau kode untuk tempat ini dan beberapa yang serupa:

  • V763 Parameter 'bExtendedInfo' selalu ditulis ulang di badan fungsi sebelum digunakan. graphicfilter2.cxx 442
  • V763 Parameter 'nVerbID' selalu ditulis ulang di badan fungsi sebelum digunakan. oleembed.cxx 841
  • V763 Parameter 'pCursor' selalu ditulis ulang di badan fungsi sebelum digunakan. edlingu.cxx 843
  • V763 Parameter 'pOutput' selalu ditulis ulang di badan fungsi sebelum digunakan. vnew.cxx 181
  • V763 Parameter 'pOutput' selalu ditulis ulang di badan fungsi sebelum digunakan. vnew.cxx 256

Kesimpulan


Keinginan untuk memeriksa kode LibreOffice muncul setelah saya menggunakan produk secara pribadi. Untuk beberapa alasan, dengan beberapa peluncuran acak, teks menghilang dari semua item menu. Hanya ikon dan garis-garis monoton yang tersisa. Kesalahan ini kemungkinan besar tingkat tinggi dan, mungkin, itu tidak dapat ditemukan menggunakan alat analisis statis. Namun demikian, penganalisa menemukan banyak masalah yang tidak terkait dengan ini, dan saya akan senang jika pengembang LibreOffice memperhatikan penganalisa kode statis dan mencoba menggunakannya untuk meningkatkan kualitas dan keandalan proyek. Ini akan bermanfaat bagi semua orang.

Terima kasih atas perhatian anda Berlangganan saluran kami dan tetap ikuti berita dari dunia pemrograman!




Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Svyatoslav Razmyslov. LibreOffice: Mimpi Buruk Akuntan

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

More articles:


All Articles