Geekly articles weekly

WinForms: Kesalahan, Holmes

Gambar 5

Kami senang mencari bug di proyek Microsoft. Mengapa Ini sederhana: proyek mereka biasanya mudah diperiksa (pekerjaan dapat dilakukan segera di lingkungan Visual Studio, yang mana PVS-Studio memiliki plug-in yang nyaman) dan mereka mengandung beberapa kesalahan. Oleh karena itu, algoritma pekerjaan yang biasa adalah sebagai berikut: cari dan unduh proyek terbuka dari MS; lihat itu; pilih kesalahan yang menarik; pastikan jumlahnya sedikit; menulis artikel tanpa lupa memuji pengembangnya. Hebat! Menang-menang-menang: butuh sedikit waktu, manual ini senang melihat materi baru di blog, dan karma dalam urutan yang sempurna. Tapi kali ini ada yang tidak beres. Mari kita lihat apa yang menarik ditemukan dalam kode sumber Formulir Windows dan apakah Microsoft harus dipuji saat ini.

Pendahuluan

Pada awal Desember 2018, Microsoft mengumumkan rilis .NET Core 3 Preview 1. Sedikit lebih awal (kira-kira dari pertengahan Oktober), GitHub mulai bekerja aktif pada publikasi kode sumber untuk Windows Forms , platform .NET Core UI untuk membuat aplikasi desktop Windows. Anda dapat melihat statistik komit di sini . Sekarang siapa pun dapat mengunduh kode sumber WinForms untuk ditinjau.

Saya juga mengunduh sumber untuk mencari kesalahan di sana menggunakan PVS-Studio. Cek tidak menimbulkan kesulitan. Diperlukan: Visual Studio 2019, .NET Core 3.0 SDK Preview, PVS-Studio. Dan sekarang log peringatan penganalisa diterima.

Setelah menerima log PVS-Studio, saya biasanya mengurutkannya dalam urutan nomor diagnostik (jendela dengan log pesan PVS-Studio di Visual Studio memiliki berbagai opsi untuk menyortir dan memfilter daftar). Ini memungkinkan Anda untuk bekerja dengan kelompok kesalahan dari jenis yang sama, yang sangat menyederhanakan analisis kode sumber. Saya menandai kesalahan menarik dalam daftar dengan tanda bintang, dan hanya setelah itu, setelah menganalisis seluruh log, saya menulis fragmen kode dan membuat deskripsi. Dan karena biasanya ada beberapa kesalahan, saya mencampurnya, mencoba untuk menempatkan yang paling menarik di awal dan akhir artikel. Tapi kali ini, kesalahannya ternyata agak banyak (eh, itu tidak mungkin untuk menyimpan intrik untuk waktu yang lama), dan saya akan membawanya sesuai urutan nomor diagnostik.

Apa yang ditemukan? 833 peringatan dari tingkat kepercayaan Tinggi dan Menengah (249 dan 584, masing-masing) dikeluarkan untuk 540.000 baris kode (tidak termasuk yang kosong) dalam 1670 file cs. Dan ya, menurut tradisi, saya tidak memeriksa tes dan tidak mempertimbangkan peringatan tingkat kepercayaan rendah (215 dikeluarkan). Menurut pengamatan saya sebelumnya, ada terlalu banyak peringatan untuk proyek dari MS. Tetapi tidak semua peringatan adalah kesalahan.

Untuk proyek ini, jumlah positif palsu sekitar 30%. Dalam sekitar 20% kasus, saya tidak bisa membuat kesimpulan yang pasti tentang apakah ini kesalahan atau tidak, karena saya tidak begitu mengenal kode itu. Ya, setidaknya 20% dari kesalahan yang saya lewatkan dapat dikaitkan dengan faktor manusia: tergesa-gesa, kelelahan, dll. Ngomong-ngomong, efek sebaliknya juga mungkin: Saya melihat melalui beberapa jenis pemicu yang sama, yang jumlahnya bisa mencapai 70-80, melalui yang kadang-kadang dapat meningkatkan jumlah kesalahan yang saya anggap nyata.

Dalam kasus apa pun, 30% peringatan menunjukkan kesalahan nyata, yang merupakan persentase yang sangat besar, mengingat bahwa alat analisa belum dikonfigurasikan sebelumnya.

Jadi, jumlah kesalahan yang bisa saya deteksi adalah sekitar 240, yang berada dalam batas statistik. Saya ulangi, menurut pendapat saya, untuk proyek dari MS ini bukan hasil yang paling luar biasa (meskipun ini hanya akan 0,44 kesalahan per 1000 baris kode), dan mungkin ada lebih banyak kesalahan nyata dalam kode WinForms. Saya mengusulkan untuk membicarakan alasan di akhir artikel, tetapi sekarang mari kita lihat kesalahan yang paling menarik.

Kesalahan

PVS-Studio: V3003 Penggunaan pola 'if (A) {...} else if (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 213, 224. ButtonStandardAdapter.cs 213

void PaintWorker(PaintEventArgs e, bool up, CheckState state) { up = up && state == CheckState.Unchecked; .... if (up & IsHighContrastHighlighted()) { .... } else if (up & IsHighContrastHighlighted()) { .... } else { .... } .... }

Dalam jika dan jika blok, kondisi yang sama diperiksa. Sepertinya copy-paste. Apakah ini sebuah kesalahan? Jika Anda melihat deklarasi metode IsHighContrastHighlighted , ada keraguan:

 protected bool IsHighContrastHighlighted() { return SystemInformation.HighContrast && Application.RenderWithVisualStyles && (Control.Focused || Control.MouseIsOver || (Control.IsDefault && Control.Enabled)); }

Metode ini mungkin dapat mengembalikan nilai yang berbeda dalam panggilan yang berurutan. Dan apa yang dilakukan dalam metode pemanggilan tampak aneh, tentu saja, tetapi ia memiliki hak untuk hidup. Namun, saya akan menyarankan para penulis untuk melihat potongan kode ini. Untuk jaga-jaga. Namun, ini adalah contoh yang baik tentang betapa sulitnya untuk menarik kesimpulan ketika menganalisis kode asing.

PVS-Studio: V3004 Pernyataan 'then' setara dengan pernyataan 'else'. RichTextBox.cs 1018

 public int SelectionCharOffset { get { int selCharOffset = 0; .... NativeMethods.CHARFORMATA cf = GetCharFormat(true); // if the effects member contains valid info if ((cf.dwMask & RichTextBoxConstants.CFM_OFFSET) != 0) { selCharOffset = cf.yOffset; // <= } else { // The selection contains characters of different offsets, // so we just return the offset of the first character. selCharOffset = cf.yOffset; // <= } .... } .... }

Dan di sini kesalahan copy-paste pasti dibuat. Terlepas dari kondisinya, variabel selCharOffset akan selalu mendapatkan nilai yang sama.

Ada dua kesalahan serupa dalam kode WinForms:
  • V3004 Pernyataan 'lalu' setara dengan pernyataan 'lain'. SplitContainer.cs 1700
  • V3004 Pernyataan 'lalu' setara dengan pernyataan 'lain'. ToolstripProfessionalRenderer.cs 371

PVS-Studio: V3008 Variabel diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 681, 680. ProfessionalColorTable.cs 681

 internal void InitSystemColors(ref Dictionary<KnownColors, Color> rgbTable) { .... rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = buttonFace; rgbTable[ProfessionalColorTable.KnownColors.msocbvcrCBBdrOuterDocked] = buttonShadow; .... }

Metode mengisi kamus rgbTable . Alat analisis menunjuk ke sepotong kode di mana dua nilai berbeda ditulis secara berurutan pada kunci yang sama. Dan semuanya akan baik-baik saja, tetapi ada 16 tempat seperti itu dalam metode ini. Ini tidak lagi terlihat seperti satu kesalahan. Tetapi mengapa melakukan ini, bagi saya itu tetap menjadi misteri. Saya tidak menemukan tanda-tanda kode autogenerated. Di editor, ini terlihat seperti ini:

Gambar 3

Saya akan memberikan sepuluh operasi pertama dengan daftar:

  1. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 785, 784. ProfessionalColorTable.cs 785
  2. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 787, 786. ProfessionalColorTable.cs 787
  3. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 789, 788. ProfessionalColorTable.cs 789
  4. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 791, 790. ProfessionalColorTable.cs 791
  5. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 797, 796. ProfessionalColorTable.cs 797
  6. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 799, 798. ProfessionalColorTable.cs 799
  7. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 807, 806. ProfessionalColorTable.cs 807
  8. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 815, 814. ProfessionalColorTable.cs 815
  9. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 817, 816. ProfessionalColorTable.cs 817
  10. V3008 Variabel ini diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 823, 822. ProfessionalColorTable.cs 823

PVS-Studio: V3011 Dua kondisi yang berlawanan ditemui. Kondisi kedua selalu salah. Periksa baris: 5242, 5240. DataGrid.cs 5242

 private void CheckHierarchyState() { if (checkHierarchy && listManager != null && myGridTable != null) { if (myGridTable == null) // <= { // there was nothing to check return; } for (int j = 0; j < myGridTable.GridColumnStyles.Count; j++) { DataGridColumnStyle gridColumn = myGridTable.GridColumnStyles[j]; } checkHierarchy = false; } }

Pernyataan pengembalian tidak akan pernah dieksekusi. Kemungkinan besar, kondisi myGridTable! = Null di eksternal jika blok ditambahkan kemudian selama refactoring. Dan sekarang memeriksa myGridTable == null tidak ada gunanya. Untuk meningkatkan kualitas kode, hapus centang ini.

PVS-Studio: V3019 Mungkin variabel yang salah dibandingkan dengan nol setelah konversi jenis menggunakan kata kunci 'sebagai'. Periksa variabel 'kiri', 'cscLeft'. TypeCodeDomSerializer.cs 611

PVS-Studio: V3019 Mungkin variabel yang salah dibandingkan dengan nol setelah konversi jenis menggunakan kata kunci 'sebagai'. Periksa variabel 'benar', 'cscRight'. TypeCodeDomSerializer.cs 615

 public int Compare(object left, object right) { OrderedCodeStatementCollection cscLeft = left as OrderedCodeStatementCollection; OrderedCodeStatementCollection cscRight = right as OrderedCodeStatementCollection; if (left == null) { return 1; } else if (right == null) { return -1; } else if (right == left) { return 0; } return cscLeft.Order - cscRight.Order; // <= }

Penganalisa segera mengeluarkan dua peringatan untuk metode Bandingkan . Apa masalahnya? Ini adalah bahwa nilai-nilai cscLeft dan cscRight tidak diperiksa untuk kesetaraan nol . Mereka bisa mendapatkan nilai ini setelah melakukan pengecoran yang gagal ke jenis OrderedCodeStatementCollection . Maka pengecualian akan dilemparkan dalam pernyataan pengembalian terakhir. Situasi seperti ini dimungkinkan ketika semua memeriksa lulus kiri dan kanan dan tidak mengarah ke jalan keluar awal dari metode.

Untuk memperbaiki kodenya, cscLeft / cscRight harus digunakan di mana-mana alih-alih dari kiri / kanan .

PVS-Studio: V3020 'istirahat' tanpa syarat dalam satu lingkaran. SelectionService.cs 421

 void ISelectionService.SetSelectedComponents( ICollection components, SelectionTypes selectionType) { .... // Handle the click case object requestedPrimary = null; int primaryIndex; if (fPrimary && 1 == components.Count) { foreach (object o in components) { requestedPrimary = o; if (o == null) { throw new ArgumentNullException(nameof(components)); } break; } } .... }

Fragmen ini merujuk, lebih tepatnya, ke kode "dengan bau". Tidak ada kesalahan di sini. Tetapi muncul pertanyaan tentang bagaimana siklus pendahuluan diatur. Mengapa diperlukan di sini - jelas: karena kebutuhan untuk mengekstrak elemen koleksi, dinyatakan sebagai ICollection . Tetapi mengapa dalam loop, awalnya dirancang untuk iterasi tunggal (prasyarat adalah kehadiran elemen tunggal dalam koleksi komponen ), jaring pengaman tambahan dalam bentuk istirahat diperlukan? Mungkin, jawabannya dapat dipertimbangkan: "Sudah berkembang secara historis." Kode terlihat jelek.

PVS-Studio: Ekspresi V3022 'ocxState! = Null' selalu benar. AxHost.cs 2186

 public State OcxState { .... set { .... if (value == null) { return; } .... ocxState = value; if (ocxState != null) // <= { axState[manualUpdate] = ocxState._GetManualUpdate(); licenseKey = ocxState._GetLicenseKey(); } else { axState[manualUpdate] = false; licenseKey = null; } .... } }

Karena kesalahan logis, "kode mati" dibentuk dalam fragmen ini. Ekspresi di blok lain tidak akan pernah dieksekusi.

PVS-Studio: V3027 Variabel 'e' digunakan dalam ekspresi logis sebelum diverifikasi terhadap nol dalam ekspresi logis yang sama. ImageEditor.cs 99

 public override object EditValue(....) { .... ImageEditor e = ....; Type myClass = GetType(); if (!myClass.Equals(e.GetType()) && e != null && myClass.IsInstanceOfType(e)) { .... } .... }

Variabel e dalam kondisi ini pertama kali digunakan dan kemudian diperiksa untuk ketimpangan nol . Hai, NullReferenceException .

Kesalahan serupa lainnya:

PVS-Studio: V3027 Variabel 'dropDownItem' digunakan dalam ekspresi logis sebelum diverifikasi terhadap nol dalam ekspresi logis yang sama. ToolStripMenuItemDesigner.cs 1351

 internal void EnterInSituEdit(ToolStripItem toolItem) { .... ToolStripDropDownItem dropDownItem = toolItem as ToolStripDropDownItem; if (!(dropDownItem.Owner is ToolStripDropDownMenu) && dropDownItem != null && dropDownItem.Bounds.Width < commitedEditorNode.Bounds.Width) { .... } .... }

Situasi yang mirip dengan yang sebelumnya, hanya dengan variabel dropDownItem . Saya pikir kesalahan seperti itu muncul sebagai akibat dari kurangnya perhatian selama refactoring. Mungkin bagian dari kondisinya ! (DropDownItem.Owner adalah ToolStripDropDownMenu) ditambahkan ke kode nanti.

PVS-Studio: V3030 Pemeriksaan berulang. Kondisi 'kolomCount> 0' sudah diverifikasi di baris 3900. ListView.cs 3903

 internal ColumnHeader InsertColumn( int index, ColumnHeader ch, bool refreshSubItems) { .... // Add the column to our internal array int columnCount = (columnHeaders == null ? 0 : columnHeaders.Length); if (columnCount > 0) { ColumnHeader[] newHeaders = new ColumnHeader[columnCount + 1]; if (columnCount > 0) { System.Array.Copy(columnHeaders, 0, newHeaders, 0, columnCount); } .... } .... }

Kesalahan yang mungkin tampak tidak berbahaya. Memang, well, pemeriksaan ekstra dilakukan, yang tidak mempengaruhi logika kerja. Dan kadang-kadang mereka bahkan melakukannya ketika Anda perlu memeriksa kembali status komponen visual, misalnya, dengan mendapatkan jumlah entri dalam daftar. Hanya dalam kasus ini, mereka memeriksa ulang kolom Variabel lokal . Ini sangat mencurigakan. Entah mereka ingin memeriksa variabel lain, atau di salah satu cek mereka menggunakan kondisi yang salah.

PVS-Studio: V3061 Parameter 'lprcClipRect' selalu ditulis ulang dalam tubuh metode sebelum digunakan. WebBrowserSiteBase.cs 281

 int UnsafeNativeMethods.IOleInPlaceSite.GetWindowContext( out UnsafeNativeMethods.IOleInPlaceFrame ppFrame, out UnsafeNativeMethods.IOleInPlaceUIWindow ppDoc, NativeMethods.COMRECT lprcPosRect, NativeMethods.COMRECT lprcClipRect, NativeMethods.tagOIFI lpFrameInfo) { ppDoc = null; ppFrame = Host.GetParentContainer(); lprcPosRect.left = Host.Bounds.X; lprcPosRect.top = Host.Bounds.Y; .... lprcClipRect = WebBrowserHelper.GetClipRect(); // <= if (lpFrameInfo != null) { lpFrameInfo.cb = Marshal.SizeOf<NativeMethods.tagOIFI>(); lpFrameInfo.fMDIApp = false; .... } return NativeMethods.S_OK; }

Kesalahan yang tidak terlihat. Ya, parameter lprcClipRect benar - benar diinisialisasi dengan nilai baru, tanpa menggunakannya dengan cara apa pun. Tetapi apa akibatnya ini? Saya pikir di suatu tempat di kode panggilan, tautan yang melewati parameter ini akan tetap tidak berubah, meskipun itu dimaksudkan secara berbeda. Memang, lihat bekerja dengan variabel lain dalam metode ini. Bahkan namanya (awalan "Get") mengisyaratkan bahwa beberapa inisialisasi akan dibuat di dalam metode melalui parameter yang diteruskan. Begitulah. Dua parameter pertama ( ppFrame dan ppDoc ) dilewatkan dengan pengubah keluar dan mendapatkan nilai baru. Tautan lprcPosRect dan lpFrameInfo digunakan untuk mengakses bidang kelas dan menginisialisasi mereka. Dan hanya lprcClipRect yang keluar dari daftar umum. Kemungkinan besar, pengubah out atau ref diperlukan untuk parameter ini.

PVS-Studio: V3066 Kemungkinan urutan argumen yang salah diteruskan ke metode 'AdjustCellBorderStyle': 'isFirstDisplayedRow' dan 'isFirstDisplayedColumn'. DataGridViewComboBoxCell.cs 1934

 protected override void OnMouseMove(DataGridViewCellMouseEventArgs e) { .... dgvabsEffective = AdjustCellBorderStyle( DataGridView.AdvancedCellBorderStyle, dgvabsPlaceholder, singleVerticalBorderAdded, singleHorizontalBorderAdded, isFirstDisplayedRow, // <= isFirstDisplayedColumn); // <= .... }

Penganalisa curiga bahwa dua argumen terakhir tercampur aduk. Mari kita lihat deklarasi metode AdjustCellBorderStyle :

 public virtual DataGridViewAdvancedBorderStyle AdjustCellBorderStyle( DataGridViewAdvancedBorderStyledataGridViewAdvancedBorderStyleInput, DataGridViewAdvancedBorderStyle dataGridViewAdvancedBorderStylePlaceholder, bool singleVerticalBorderAdded, bool singleHorizontalBorderAdded, bool isFirstDisplayedColumn, bool isFirstDisplayedRow) { .... }

Tampak seperti kesalahan. Ya, sering beberapa argumen disengaja secara terbalik, misalnya, untuk menukar beberapa variabel. Tetapi saya tidak berpikir bahwa inilah masalahnya. Tidak ada dalam pemanggilan atau metode dipanggil mengatakan pola penggunaan seperti itu. Pertama, variabel tipe bool bingung. Kedua, nama-nama metode juga umum: tidak ada "Swap" atau "Reverse". Lagipula, tidak begitu sulit untuk melakukan kesalahan seperti itu. Orang sering merasakan urutan pasangan baris / kolom berbeda. Bagi saya, misalnya, hanya "baris / kolom" sudah biasa. Tetapi bagi penulis metode AdjustCellBorderStyle yang disebut, jelas, urutan yang lebih akrab adalah "kolom / baris".

PVS-Studio: V3070 Variabel tidak diinisialisasi 'LANG_USER_DEFAULT' digunakan ketika menginisialisasi variabel 'LOCALE_USER_DEFAULT'. NativeMethods.cs 890

 internal static class NativeMethods { .... public static readonly int LOCALE_USER_DEFAULT = MAKELCID(LANG_USER_DEFAULT); public static readonly int LANG_USER_DEFAULT = MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT); .... }

Kesalahan langka. Urutan inisialisasi bidang kelas dicampur. Untuk menghitung nilai bidang LOCALE_USER_DEFAULT , gunakan bidang LANG_USER_DEFAULT , yang belum diinisialisasi dan memiliki nilai 0. By the way, variabel LANG_USER_DEFAULT tidak digunakan di tempat lain dalam kode. Saya tidak terlalu malas dan menulis program konsol kecil yang mensimulasikan situasi. Alih-alih nilai dari beberapa konstanta yang digunakan dalam kode WinForms, saya menggantikan nilai aktualnya:

 internal static class NativeMethods { public static readonly int LOCALE_USER_DEFAULT = MAKELCID(LANG_USER_DEFAULT); public static readonly int LANG_USER_DEFAULT = MAKELANGID(0x00, 0x01); public static int MAKELANGID(int primary, int sub) { return ((((ushort)(sub)) << 10) | (ushort)(primary)); } public static int MAKELCID(int lgid) { return MAKELCID(lgid, 0x0); } public static int MAKELCID(int lgid, int sort) { return ((0xFFFF & lgid) | (((0x000f) & sort) << 16)); } } class Program { static void Main() { System.Console.WriteLine(NativeMethods.LOCALE_USER_DEFAULT); } }

Sebagai hasil dari peluncuran, berikut ini akan ditampilkan pada konsol: 0. Sekarang kita menukar deklarasi bidang LOCALE_USER_DEFAULT dan LANG_USER_DEFAULT . Hasil program dalam bentuk ini: 1024. Saya pikir tidak ada lagi yang perlu dikomentari di sini.

PVS-Studio: V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'ces'. CodeDomSerializerBase.cs 562

 protected void DeserializeStatement( IDesignerSerializationManager manager, CodeStatement statement) { .... CodeExpressionStatement ces = statement as CodeExpressionStatement; if (ces != null) { .... } else { .... DeserializeExpression(manager, null, ces.Expression); // <= .... } .... }

Kode yang harus "jatuh" cukup stabil, karena Anda bisa masuk ke cabang lain tepat ketika referensi ces adalah nol .

Contoh serupa lainnya:

PVS-Studio: V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'kotak kombo'. ComboBox.cs 6610

 public void ValidateOwnerDrawRegions(ComboBox comboBox, ....) { .... if (comboBox != null) { return; } Rectangle topOwnerDrawArea = new Rectangle(0, 0, comboBox.Width, innerBorder.Top); .... }

Kode paradoks. Rupanya, mereka mencampuradukkan tes dengan menulis jika (comboBox! = Null) bukannya if (comboBox == null) . Maka - kami akan menerima NullReferenceException berikutnya.

Kami memeriksa dua kesalahan yang cukup jelas, V3080 , di mana Anda dapat secara visual melacak situasi kemungkinan penggunaan referensi nol dalam metode ini. Tetapi diagnostik V3080 jauh lebih cerdas dan dapat mencari kesalahan serupa untuk rangkaian panggilan metode. Belum lama ini, kami secara signifikan memperkuat mekanisme aliran data dan analisis antar-prosedur. Anda dapat membaca tentang ini di artikel " Jenis Referensi Nullable dalam C # 8.0 dan Analisis Statis ". Dan di sini ada kesalahan serupa yang ditemukan di WinForms:

PVS-Studio: V3080 Kemungkinan ada null dereference di dalam metode di 'reader.NameTable'. Pertimbangkan memeriksa argumen 1: contentReader. ResXResourceReader.cs 267

 private void EnsureResData() { .... XmlTextReader contentReader = null; try { if (fileContents != null) { contentReader = new XmlTextReader(....); } else if (reader != null) { contentReader = new XmlTextReader(....); } else if (fileName != null || stream != null) { .... contentReader = new XmlTextReader(....); } SetupNameTable(contentReader); // <= .... } finally { .... } .... }

Lihat apa yang terjadi dengan contentReader variabel di tubuh metode. Setelah inisialisasi dengan referensi nol, sebagai hasil dari salah satu pemeriksaan, tautan akan diinisialisasi. Namun, serangkaian cek tidak berakhir dengan blok lain . Ini berarti bahwa dalam beberapa kasus yang jarang terjadi (atau karena refactoring di masa depan), tautan masih dapat tetap nol. Selanjutnya, itu akan diteruskan ke metode SetupNameTable , di mana ia digunakan tanpa verifikasi:

 private void SetupNameTable(XmlReader reader) { reader.NameTable.Add(ResXResourceWriter.TypeStr); reader.NameTable.Add(ResXResourceWriter.NameStr); .... }

Ini berpotensi kode tidak aman.

Dan satu kesalahan lagi, di mana penganalisa harus melalui serangkaian panggilan untuk mengidentifikasi masalah:

PVS-Studio: V3080 Kemungkinan null dereference. Pertimbangkan untuk memeriksa 'tata letak'. DockAndAnchorLayout.cs 156

 private static Rectangle GetAnchorDestination( IArrangedElement element, Rectangle displayRect, bool measureOnly) { .... AnchorInfo layout = GetAnchorInfo(element); int left = layout.Left + displayRect.X; .... }

Penganalisa mengklaim bahwa adalah mungkin untuk mendapatkan referensi nol dari metode GetAnchorInfo , yang akan menghasilkan pengecualian saat menghitung nilai kiri . Mari kita telusuri seluruh rantai panggilan dan periksa apakah ini benar:

 private static AnchorInfo GetAnchorInfo(IArrangedElement element) { return (AnchorInfo)element.Properties.GetObject(s_layoutInfoProperty); } public object GetObject(int key) => GetObject(key, out _); public object GetObject(int key, out bool found) { short keyIndex = SplitKey(key, out short element); if (!LocateObjectEntry(keyIndex, out int index)) { found = false; return null; } // We have found the relevant entry. See if // the bitmask indicates the value is used. if (((1 << element) & s_objEntries[index].Mask) == 0) { found = false; return null; } found = true; switch (element) { case 0: return s_objEntries[index].Value1; .... default: Debug.Fail("Invalid element obtained from LocateObjectEntry"); return null; } }

Memang, dalam beberapa kasus, metode GetObject yang menutup rantai panggilan akan kembali nol , yang tanpa pemeriksaan tambahan akan diteruskan ke metode panggilan. Mungkin, metode GetAnchorDestination harus menyediakan situasi seperti itu.

Dalam kode WinForms ada beberapa kesalahan seperti itu, lebih dari 70 . Mereka semua serupa dan saya tidak akan memberikan deskripsi mereka di artikel.

PVS-Studio: V3091 Analisis empiris. Mungkin salah ketik ada di dalam string literal: "ShowCheckMargin". Kata 'ShowCheckMargin' mencurigakan. PropertyNames.cs 136

 internal class PropertyNames { .... public static readonly string ShowImageMargin = "ShowCheckMargin"; ... public static readonly string ShowCheckMargin = "ShowCheckMargin"; .... }

Contoh kesalahan yang baik yang tidak mudah dideteksi. Saat menginisialisasi bidang kelas, mereka menggunakan nilai yang sama, meskipun pembuat kode jelas tidak berpikir begitu (salin-tempel yang harus disalahkan). Penganalisa membuat kesimpulan ini dengan membandingkan nama-nama variabel dan nilai-nilai string yang ditugaskan. Saya hanya memberi baris kesalahan, tetapi lihat tampilannya di editor kode:

Gambar 2

Ini adalah deteksi kesalahan seperti itu yang menunjukkan kekuatan dan perawatan alat analisis statis tanpa batas.

PVS-Studio: V3095 Objek 'currentForm' digunakan sebelum diverifikasi terhadap nol. Periksa baris: 3386, 3404. Application.cs 3386

 private void RunMessageLoopInner(int reason, ApplicationContext context) { .... hwndOwner = new HandleRef( null, UnsafeNativeMethods.GetWindowLong( new HandleRef(currentForm, currentForm.Handle), // <= NativeMethods.GWL_HWNDPARENT)); .... if (currentForm != null && ....) .... }

Klasik Variabel currentForm digunakan tanpa pemeriksaan. Tetapi lebih lanjut dalam kode ada pemeriksaan untuk kesetaraan nol . Dalam hal ini, saya dapat menyarankan Anda untuk lebih berhati-hati ketika bekerja dengan jenis referensi, serta menggunakan analisis statis :).

Kesalahan serupa lainnya:

PVS-Studio: V3095 Objek 'backgroundBrush' digunakan sebelum diverifikasi terhadap nol. Periksa baris: 2331, 2334. DataGrid.cs 2331

 public Color BackgroundColor { .... set { .... if (!value.Equals(backgroundBrush.Color)) // <= { if (backgroundBrush != null && BackgroundBrush != DefaultBackgroundBrush) .... } } }

Dalam kode WinForms, saya telah menemukan lebih dari 60 kesalahan seperti itu. Menurut saya, semuanya cukup kritis dan membutuhkan perhatian pengembang. Tetapi dalam artikel itu tidak begitu menarik untuk membicarakannya, jadi saya akan membatasi diri pada keduanya yang dibahas di atas.

PVS-Studio: V3125 Objek '_propInfo' digunakan dan diverifikasi terhadap null di cabang eksekusi yang berbeda. Periksa baris: 996, 982. Binding.cs 996

 private void SetPropValue(object value) { .... if (....) { if .... else if (_propInfo != null) .... } else { _propInfo.SetValue(_control, value); } .... }

Untuk melengkapi gambar - juga semacam klasik, bug V3125 . Situasi sebaliknya. Pada awalnya, referensi yang berpotensi nol digunakan dengan aman, memeriksa nol , tetapi kemudian mereka tidak melakukan ini lagi dalam kode.

Dan kesalahan serupa lainnya:

PVS-Studio: V3125 Objek 'pemilik' digunakan setelah diverifikasi terhadap nol. Periksa baris: 64, 60. FlatButtonAppearance.cs 64

 public int BorderSize { .... set { .... if (owner != null && owner.ParentInternal != null) { LayoutTransaction.DoLayoutIf(....); } owner.Invalidate(); // <= .... } }

Kecantikan Tapi ini dari sudut pandang peneliti luar. Memang, selain dua V3125 ini , alat analisa menemukan lebih dari 50 pola yang sama dalam kode WinForms. Pengembang harus bekerja.

Dan akhirnya - kesalahan yang agak menarik, menurut saya.

PVS-Studio: V3137 Variabel 'hCurrentFont' ditugaskan tetapi tidak digunakan pada akhir fungsi. DeviceContext2.cs 241

 sealed partial class DeviceContext : .... { WindowsFont selectedFont; .... internal void DisposeFont(bool disposing) { if (disposing) { DeviceContexts.RemoveDeviceContext(this); } if (selectedFont != null && selectedFont.Hfont != IntPtr.Zero) { IntPtr hCurrentFont = IntUnsafeNativeMethods.GetCurrentObject( new HandleRef(this, hDC), IntNativeMethods.OBJ_FONT); if (hCurrentFont == selectedFont.Hfont) { // select initial font back in IntUnsafeNativeMethods.SelectObject(new HandleRef(this, Hdc), new HandleRef(null, hInitialFont)); hCurrentFont = hInitialFont; // <= } selectedFont.Dispose(disposing); selectedFont = null; } } .... }

Mari kita lihat apa yang disiagakan oleh penganalisa, dan mengapa fakta bahwa suatu variabel diberi nilai, tetapi tidak digunakan dalam metode di masa depan, dapat mengindikasikan masalah.

Kelas parsial dideklarasikan di file DeviceContext2.cs . Metode DisposeFont digunakan untuk membebaskan sumber daya setelah bekerja dengan grafik: konteks perangkat dan font. Untuk pemahaman yang lebih baik, saya telah menyediakan seluruh metode DisposeFont . Perhatikan variabel lokal hCurrentFont . Masalahnya adalah bahwa mendeklarasikan variabel ini dalam suatu metode menyembunyikan bidang kelas dengan nama yang sama. Saya menemukan dua metode kelas DeviceContext yang menggunakan bidang yang disebut hCurrentFont :

 public IntPtr SelectFont(WindowsFont font) { .... hCurrentFont = font.Hfont; .... } public void ResetFont() { .... hCurrentFont = hInitialFont; }

Lihatlah metode ResetFont . Baris terakhir ada persis apa yang dilakukan metode DisposeFont di blok bersarang jika (penganalisis menunjuk ke tempat ini). Dan bidang hCurrentFont ini dengan nama yang sama dideklarasikan di bagian lain dari kelas parsial dalam file DeviceContext.cs :

 sealed partial class DeviceContext : .... { .... IntPtr hInitialFont; .... IntPtr hCurrentFont; // <= .... }

Jadi, kesalahan yang jelas telah dibuat. Pertanyaan lain adalah kekritisannya. Sekarang, sebagai hasil dari metode DisposeFont yang berfungsi di bagian yang ditandai dengan komentar “pilih kembali font awal”, bidang hCurrentFont tidak akan diberi nilai awal. Saya pikir hanya penulis kode yang dapat memberikan putusan yang tepat.

Kesimpulan

Jadi, kali ini saya harus "memarahi" MS sedikit. Di WinForms ada banyak kesalahan yang membutuhkan perhatian pengembang. Mungkin ini karena beberapa terburu-buru dengan yang MS sedang mengerjakan .NET Core 3 dan komponen, termasuk WinForms.Menurut pendapat saya, kode WinForms masih "lembab", tapi saya harap situasinya akan segera berubah menjadi lebih baik.

Alasan kedua untuk sejumlah besar kesalahan mungkin karena alat analisa kami menjadi lebih baik untuk mencarinya :).

Ngomong-ngomong, artikel oleh rekan saya Sergey Vasiliev akan segera diterbitkan di mana dia mencari dan menemukan beberapa masalah dalam kode .NET Core libraries. Saya berharap karyanya juga akan berkontribusi untuk meningkatkan kinerja platform .NET, karena kami selalu mencoba untuk mengomunikasikan hasil analisis proyek mereka kepada pengembang.

Nah, bagi mereka yang ingin meningkatkan produk mereka sendiri atau melakukan penelitian untuk menemukan kesalahan dalam proyek orang lain, saya sarankan mengunduh dan mencoba PVS-Studio.

Semua kode bersih!


Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Sergey Khrenov. WinForms: Kesalahan, Holmes

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

More articles:


All Articles