Geekly articles weekly

Memeriksa Emby dengan PVS-Studio

Gambar 3

Emby adalah server media yang cukup populer bersama Plex dan Kodi. Pada artikel ini, kita akan membahas bug yang ditemukan dalam kode sumbernya dengan analyzer statis PVS-Studio. Pernyataan "Dibangun dengan ReSharper" di situs web resmi proyek membuat analisis lebih menarik.

PVS-Studio


PVS-Studio berjalan pada sistem Windows, Linux, dan macOS 64-bit. Itu dapat mendeteksi bug dalam kode sumber perangkat lunak yang ditulis dalam C, C ++, C #, dan Java.

Proyek sedang dianalisis


Emby adalah server media; kode sumbernya tersedia di GitHub . Ini memungkinkan pengguna untuk melakukan streaming dan mengakses konten media mereka (video, audio, foto) di perangkat apa pun. Berikut ringkasan singkat fitur Emby menurut situs web resmi proyek:

  • Emby secara otomatis mengubah dan mengalirkan media Anda saat itu untuk bermain di perangkat apa pun;
  • Opsi kontrol orangtua yang luas untuk kontrol akses konten yang mudah, yang merupakan fitur penting bagi keluarga dengan anak kecil;
  • Emby mengatur konten Anda menjadi presentasi yang mudah dan elegan. Media pribadi Anda tidak akan pernah terlihat sama;
  • Streaming dengan dukungan sinkronisasi cloud;
  • Berbagi konten dengan mudah dengan teman dan keluarga Anda;
  • Dan masih banyak lagi.

Cuplikan kode paling menarik yang dilaporkan oleh penganalisa


Pesan diagnostik PVS-Studio: V3001 Ada sub-ekspresi yang identik 'c! =' <'' Di sebelah kiri dan di sebelah kanan operator '&&'. HttpListenerRequest.Managed.cs 49

internal void SetRequestLine(string req) { .... if ((ic >= 'A' && ic <= 'Z') || (ic > 32 && c < 127 && c != '(' && c != ')' && c != '<' && // <= c != '<' && c != '>' && c != '@' && c != ',' && c != ';' && // <= c != ':' && c != '\\' && c != '"' && c != '/' && c != '[' && c != ']' && c != '?' && c != '=' && c != '{' && c != '}')) continue; .... }

Penganalisis telah mendeteksi subekspresi duplikat c! = '<' . Satu penjelasan adalah bahwa ini adalah kesalahan pemrograman dan pengembang bermaksud menulis sesuatu yang lain sebagai ganti '<' . Penjelasan lain yang lebih mungkin adalah bahwa subekspresi kedua tidak dimaksudkan sama sekali dan harus dihapus.

Pesan diagnostik PVS-Studio: V3001 Ada sub-ekspresi identik 'SmbConstants.AttrHidden' ke kiri dan ke kanan '|' operator. SmbComDelete.cs 29

 internal SmbComDelete(string fileName) { Path = fileName; Command = SmbComDelete; _searchAttributes = SmbConstants.AttrHidden | SmbConstants.AttrHidden | SmbConstants.AttrSystem; }

Kesalahan ketik lain yang berkaitan dengan duplikasi subekspresi. Sebagai catatan, ada terlalu banyak masalah seperti itu dalam kode sumber Emby - kesalahan yang disebabkan oleh kurangnya perhatian. Saya tidak menyalahkan pengembang; kita semua dapat linglung di waktu ( contoh , contoh , contoh ), dan inilah mengapa analisis statis ada - untuk melindungi kita dari kesalahan kita sendiri.

Pesan diagnostik PVS-Studio: V3004 Pernyataan 'then' setara dengan pernyataan 'else'. SqliteItemRepository.cs 5648

 private QueryResult<Tuple<BaseItem, ItemCounts>> GetItemValues(....) { .... if (typesToCount.Length == 0) { whereText += " And CleanName In (Select CleanValue from ItemValues where " + typeClause + " AND ItemId in (select guid from TypedBaseItems" + innerWhereText + "))"; } else { //whereText += " And itemTypes not null"; whereText += " And CleanName In (Select CleanValue from ItemValues where " + typeClause + " AND ItemId in (select guid from TypedBaseItems" + innerWhereText + "))"; } .... }

Dan yang ini terlihat sangat mirip bug salin-tempel karena blok if and else memiliki tubuh yang sama. Apa gunanya memeriksa ukuran array typesToCount jika tidak mempengaruhi logika eksekusi selanjutnya? Ini adalah sesuatu yang hanya diketahui oleh penulis.

Pesan diagnostik PVS-Studio: V3005 Variabel '_validProviderIds' ditugaskan untuk dirinya sendiri. BaseNfoParser.cs 77

 private Dictionary<string, string> _validProviderIds; .... public void Fetch(....) { .... _validProviderIds = _validProviderIds = new Dictionary<....>(....); .... }

Kesalahan ketik lain, yang menghasilkan penetapan variabel nilainya sendiri. Kode ini perlu direvisi.

Pesan diagnostik PVS-Studio: V3008 Variabel 'Bab' diberi nilai dua kali berturut-turut. Mungkin ini sebuah kesalahan. Periksa baris: 29, 28. Judul.cs 29

 public Title(uint titleNum) { ProgramChains = new List<ProgramChain>(); Chapters = new List<Chapter>(); Chapters = new List<Chapter>(); TitleNumber = titleNum; }

Ini tentang kurangnya perhatian dan kesalahan ketik lagi ... Variabel Bab diberi nilai dua kali. Tentu, tugas duplikat ini tidak akan membahayakan, tetapi Anda masih tidak menginginkan hal-hal seperti itu dalam kode Anda. Tidak ada gunanya berlama-lama untuk yang satu ini, jadi mari kita lanjutkan.

Pesan diagnostik PVS-Studio: V3013 Aneh bahwa tubuh fungsi 'Baca' sepenuhnya setara dengan tubuh fungsi 'Tulis' (407, baris 415). BaseSqliteRepository.cs 407

 public static IDisposable Read(this ReaderWriterLockSlim obj) { return new WriteLockToken(obj); } public static IDisposable Write(this ReaderWriterLockSlim obj) { return new WriteLockToken(obj); } private sealed class WriteLockToken : IDisposable { private ReaderWriterLockSlim _sync; public WriteLockToken(ReaderWriterLockSlim sync) { _sync = sync; sync.EnterWriteLock(); } public void Dispose() { if (_sync != null) { _sync.ExitWriteLock(); _sync = null; } } }

Fungsi Baca dan Tulis memiliki badan yang sama, dan itulah yang dikatakan oleh analis. Metode EnterWriteLock digunakan untuk memasukkan kunci dalam mode tulis. Jika Anda ingin memasukkan kunci dalam mode baca, gunakan metode EnterReadLock , yang memungkinkan sumber daya dibaca oleh beberapa utas sekaligus.

Pengembang harus memeriksa kode ini karena sangat mungkin mengandung bug - terlebih lagi karena ada kelas yang tidak digunakan yang ditemukan dalam kode:

 private sealed class ReadLockToken : IDisposable { private ReaderWriterLockSlim _sync; public ReadLockToken(ReaderWriterLockSlim sync) { _sync = sync; sync.EnterReadLock(); } public void Dispose() { if (_sync != null) { _sync.ExitReadLock(); _sync = null; } } }

Pesan diagnostik PVS-Studio: V3021 Ada dua pernyataan 'jika' dengan ekspresi kondisional yang identik. Pernyataan 'jika' pertama berisi pengembalian metode. Ini berarti bahwa pernyataan 'jika' yang kedua adalah SkiaEncoder.cs 537 yang tidak masuk akal

 public string EncodeImage(string inputPath, DateTime dateModified, string outputPath, bool autoOrient, ImageOrientation? orientation, int quality, ImageProcessingOptions options, ImageFormat selectedOutputFormat) { if (string.IsNullOrWhiteSpace(inputPath)) { throw new ArgumentNullException("inputPath"); } if (string.IsNullOrWhiteSpace(inputPath)) { throw new ArgumentNullException("outputPath"); } .... }

Pengembang harus mengkloning empat baris pertama tetapi lupa untuk mengubah nama variabel yang diperiksa dari inputPath ke outputPath . Ada beberapa baris lebih lanjut di mana outputPath digunakan tanpa cek nol sebelumnya, yang berarti pengecualian dapat dilemparkan.

Pesan diagnostik PVS-Studio:

  • V3022 Expression 'processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null)' selalu salah. WebSocket.cs 462
  • Ekspresi V3022 'processCloseFrame (frame)' selalu salah. WebSocket.cs 461
  • V3022 Expression 'frame.IsClose? processCloseFrame (frame): processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null) 'selalu salah. WebSocket.cs 460
  • Ekspresi V3022 'processPongFrame (frame)' selalu benar. WebSocket.cs 459
  • Ekspresi V3022 'processPingFrame (frame)' selalu benar. WebSocket.cs 457
  • Ekspresi V3022 'processDataFrame (frame)' selalu benar. WebSocket.cs 455
  • Ekspresi V3022 selalu salah. WebSocket.cs 448

 private bool processWebSocketFrame(WebSocketFrame frame) { return frame.IsCompressed && _compression == CompressionMethod.None ? processUnsupportedFrame(....) : frame.IsFragmented ? processFragmentedFrame(frame) : frame.IsData ? processDataFrame(frame) : frame.IsPing ? processPingFrame(frame) : frame.IsPong ? processPongFrame(frame) : frame.IsClose ? processCloseFrame(frame) : processUnsupportedFrame(....); } private bool processUnsupportedFrame(....) { processException(....); return false; } private bool processDataFrame(WebSocketFrame frame) { .... return true; } private bool processPingFrame(WebSocketFrame frame) { var mask = Mask.Unmask; return true; } private bool processPongFrame(WebSocketFrame frame) { _receivePong.Set(); return true; } private bool processCloseFrame(WebSocketFrame frame) { var payload = frame.PayloadData; close(payload, !payload.ContainsReservedCloseStatusCode, false); return false; }

Saya telah memeriksa lebih sedikit proyek daripada rekan setim PVS-Studio saya sejauh ini, dan ini mungkin menjelaskan mengapa saya belum pernah melihat potongan kode 13 baris yang akan memicu 7 peringatan sekaligus (yaitu sedikit lebih dari satu peringatan per dua baris). Itu sebabnya saya memasukkan kasus ini ke dalam artikel. Di bawah ini adalah analisis langkah-demi-langkah dari fragmen masalah.

  1. Frame ekspresi.IsCompressed && _compression == CompressionMethod.None dievaluasi terlebih dahulu. Jika benar, metode processUnsupportedFrame akan mengeksekusi dan mengembalikan false dalam hal apa pun (ini adalah peringatan pertama). Jika centangnya salah , kami beralih ke yang berikutnya.
  2. Frame nilai.IsFragmented dicentang. Tidak ada masalah di sini.
  3. Frame nilai.IsData diperiksa. Jika benar, metode processDataFrame akan mengembalikan true dalam hal apa pun. Ini peringatan kedua.
  4. Frame nilai .IsPing diperiksa. Jika itu benar, metode processPingFrame akan mengembalikan true . Ini peringatan ketiga.
  5. Frame nilai.IsPong diperiksa. Sama seperti yang sebelumnya.
  6. Yang terakhir: frame.IsClose . processCloseFrame dan processUnsupportedFrame return false dalam hal apa pun.

Gambar 1

Saya harap itu tidak terlalu membosankan untuk diikuti. Contoh yang tersisa tidak begitu rumit.

Pesan diagnostik PVS-Studio: V3085 Nama bidang 'RtpHeaderBytes' dalam tipe bersarang ambigu. Tipe luar berisi bidang statis dengan nama yang identik. HdHomerunUdpStream.cs 200

 public class HdHomerunUdpStream : LiveStream, IDirectStreamProvider { .... private static int RtpHeaderBytes = 12; public class UdpClientStream : Stream { private static int RtpHeaderBytes = 12; private static int PacketSize = 1316; private readonly MediaBrowser.Model.Net.ISocket _udpClient; bool disposed; .... } .... }

Kelas bersarang UdpClientStream memiliki bidang yang namanya identik dengan bidang dari kelas terlampir HdHomerunUdpStream . Ini bukan bug tapi itu alasan yang bagus untuk memeriksa kode ini lagi untuk memastikan itu benar. Memiliki variabel dengan nama yang identik membuatnya mudah untuk secara tidak sengaja menggunakan salah satu dari mereka, bukan yang lain, menghasilkan perilaku program yang tidak terduga, sedangkan kompiler tidak akan mengatakan sepatah kata pun.

Pesan diagnostik PVS-Studio:

  • V3090 Tidak aman mengunci suatu jenis. Semua instance dari suatu tipe akan memiliki objek 'Type' yang sama. Lmhosts.cs 49
  • V3090 Tidak aman mengunci suatu jenis. Semua instance dari suatu tipe akan memiliki objek 'Type' yang sama. Lmhosts.cs 57

 public class Lmhosts { public static NbtAddress GetByName(string host) { lock (typeof(Lmhosts)) { return GetByName(new Name(host, 0x20, null)); } } internal static NbtAddress GetByName(Name name) { lock (typeof(Lmhosts)) { .... } } }

Penganalisis memperingatkan tentang kunci yang tidak aman di sini. Menggunakan kunci dengan cara seperti itu tidak disarankan karena objek kunci dapat diakses secara publik dan dapat dikunci di tempat lain, dan pengembang yang pertama kali menggunakan objek ini mungkin tidak pernah tahu tentang itu. Ini dapat menyebabkan kebuntuan.

Idealnya, Anda harus menggunakan bidang pribadi untuk mengunci, misalnya:

 private Object locker = new Object(); public static NbtAddress GetByName(string host) { lock (locker) { return GetByName(new Name(host, 0x20, null)); } }

Pesan diagnostik PVS-Studio: V3142 Kode unreacheble terdeteksi. Mungkin saja ada kesalahan. HdHomerunHost.cs 621

 protected override async Task<ILiveStream> GetChannelStream(....) { .... var enableHttpStream = true; if (enableHttpStream) { mediaSource.Protocol = MediaProtocol.Http; var httpUrl = channelInfo.Path; // If raw was used, the tuner doesn't support params if (!string.IsNullOrWhiteSpace(profile) && !string.Equals(profile, "native", StringComparison.OrdinalIgnoreCase)) { httpUrl += "?transcode=" + profile; } mediaSource.Path = httpUrl; return new SharedHttpStream(....); } return new HdHomerunUdpStream(....); }

Penganalisa mengatakan bahwa baris terakhir dalam cuplikan ini tidak akan pernah dijalankan. Dan apa tujuan dari mendeklarasikan variabel enableHttpStream, menetapkan true padanya, dan memeriksanya setelah itu?

Mungkin kode ini hanya mubazir, tetapi harus direvisi.

Pesan diagnostik PVS-Studio: V3083 Doa yang tidak aman dari acara 'RefreshStarted', NullReferenceException dimungkinkan. Pertimbangkan menugaskan acara ke variabel lokal sebelum menjalankannya. ProviderManager.cs 943

 public void OnRefreshStart(BaseItem item) { .... if (RefreshStarted != null) { RefreshStarted(this, new GenericEventArgs<BaseItem>(item)); } }

Penganalisa memperingatkan kita tentang panggilan yang berpotensi tidak aman dari pengendali acara RefreshStarted .

Mari kita cari tahu mengapa panggilan ini tidak aman. Misalkan acara dibatalkan berlangganan dari utas lain saat ini antara memeriksa acara untuk null dan memanggil event handler di badan pernyataan if . Jika tidak ada pelanggan yang tersisa, acara RefreshStarted akan menjadi nol , tetapi di utas tempat cek nol telah berlalu, panggilan tetap akan dijalankan:

 RefreshStarted(this, new GenericEventArgs<BaseItem>(item));

Ini akan menghasilkan melemparkan NullReferenceException .

Pesan diagnostik PVS-Studio: V3029 Ekspresi kondisional dari pernyataan 'jika' yang terletak bersebelahan adalah identik. Periksa baris: 142, 152. LocalImageProvider.cs 142

 private void PopulateImages(....) { .... // Logo if (!isEpisode && !isSong && !isPerson) { added = AddImage(....); if (!added) { added = AddImage(....); } } // Art if (!isEpisode && !isSong && !isPerson) { AddImage(....); } .... }

Pernyataan dua jika memiliki kondisi yang sama, tetapi tubuh mereka berbeda. Saya tidak yakin apakah ini bug atau hanya kode yang berlebihan. Mungkin tidak apa-apa dan pengembang hanya ingin secara eksplisit membedakan antara dua tindakan, salah satunya berkaitan dengan "Logo" dan yang lainnya dengan "Seni", apa pun itu.

Pesan diagnostik PVS-Studio: V3041 Ekspresi ini secara implisit dilemparkan dari tipe 'int' ke tipe 'double'. Pertimbangkan untuk menggunakan pemeran tipe eksplisit untuk menghindari hilangnya bagian fraksional. Contoh: double A = (double) (X) / Y; LiveTvManager.cs 1085

 private async Task RefreshChannelsInternal(....) { .... double progressPerService = _services.Length == 0 ? 0 : 1 / _services.Length; .... }

Kode ini berisi divisi integer, dengan nilai yang dihasilkan dilemparkan ke tipe floating-point, yang tampaknya tidak tepat untuk dilakukan.

Sebenarnya, variabel progressPerService akan memiliki nilai 1.0 hanya jika _services.Length = 1 . Dengan nilai lain dari layanan _Length , hasilnya akan menjadi 0,0 .

Saya pikir yang seharusnya ditulis adalah sebagai berikut:

 double progressPerService = _services.Length == 0 ? 0 : (double)1 / _services.Length;

Pesan diagnostik PVS-Studio: V3050 Mungkin salah HTML. Tag penutup </u> ditemukan, sedangkan tag </i> diharapkan. SrtParserTests.cs 64

 public void TestParse() { var expectedSubs = new SubtitleTrackInfo { TrackEvents = new SubtitleTrackEvent[] { .... new SubtitleTrackEvent { Id = "6", StartPositionTicks = 330000000, EndPositionTicks = 339990000, Text = "This contains nested <b>bold, <i>italic, <u>underline</u> and <s>strike-through</s></u></i></b> HTML tags" }, .... } }; }

Perhatikan baris ini:

 <u>underline</u>

Itu sudah memiliki tag penutup </u> .
Kemudian kita melihat teks berikut:
 </s></u></i></b> HTML tags"

Ada tag penutup tambahan </u> di sini, yang ditunjukkan oleh penganalisa.

Pesan diagnostik PVS-Studio: V3051 Pemeriksaan tipe berlebihan. Objek sudah dari tipe 'Pengecualian'. SmbFileInputStream.cs 107

 protected internal virtual IOException SeToIoe(SmbException se) { IOException ioe = se; Exception root = se.GetRootCause(); if (root is TransportException) { ioe = (TransportException)root; root = ((TransportException)ioe).GetRootCause(); } if (root is Exception) { ioe = new IOException(root.Message); ioe.InitCause(root); } return ioe; }

Terus terang, saya tidak begitu mengerti apa yang dimaksud oleh pengembang dengan kode ini. Penganalisa mengatakan kondisi pernyataan if kedua memeriksa apakah objek root kompatibel dengan tipenya sendiri. Ini mungkin hanya kode berlebihan, tetapi memang terlihat aneh dan saya sarankan merevisinya.

Kesimpulan


Pengembang Emby telah melakukan pekerjaan besar dalam segala hal (proyek ini 215.539 LOC panjang, 4,6% di antaranya adalah komentar). Mereka melakukannya dengan baik, saya sungguh-sungguh. Tetapi PVS-Studio juga layak dipuji: ia menghasilkan 113 peringatan tingkat tinggi , 213 tingkat menengah, dan 112 tingkat rendah. Beberapa dari mereka adalah positif palsu, tetapi sebagian besar bug tidak disebutkan di sini karena mereka sangat sama. Misalnya, diagnostik V3022 (selalu kondisi salah / benar) saja dipicu 106 kali. Tentu saja, saya bisa saja memfilter positif palsu dan memasukkan sisanya ke dalam artikel, tetapi itu akan menjadi terlalu membosankan untuk dibaca.

Saya harap saya berhasil menunjukkan bagaimana analisis statis membantu dalam pengembangan perangkat lunak. Jelas, pemeriksaan satu kali saja tidak cukup; Anda harus menggunakan analisis statis secara teratur. Topik ini dibahas lebih detail dalam artikel " Godot: On Regular Use of Static Analyzers ".

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

More articles:


All Articles