PVS-Studio untuk Java dikirim ke jalur. Perhentian berikutnya adalah Elasticsearch

Jauh dari tahun pertama, tim PVS-Studio telah membuat blog tentang cek proyek open-source oleh penganalisa kode statis dengan nama yang sama. Hingga saat ini, lebih dari 300 proyek telah diperiksa, dan lebih dari 12.000 kasus telah ditulis ke basis data kesalahan yang ditemukan. Awalnya, alat analisa diimplementasikan untuk menguji kode C dan C ++, kemudian dukungan bahasa C # muncul. Oleh karena itu, di antara proyek yang diuji, mayoritas (> 80%) jatuh pada C dan C ++. Baru-baru ini, Java telah ditambahkan ke bahasa yang didukung, yang berarti bahwa PVS-Studio membuka pintu ke dunia baru, dan sekarang saatnya untuk melengkapi database dengan kesalahan dari proyek-proyek Java.

Dunia Jawa sangat besar dan beragam, jadi mata saya melebar ketika memilih proyek untuk menguji penganalisa baru. Pada akhirnya, pilihan jatuh pada mesin pencarian teks lengkap dan analytics Elasticsearch. Ini adalah proyek yang cukup berhasil, dan dalam proyek-proyek yang sukses, menemukan kesalahan dua kali lipat, atau bahkan tiga kali lipat, lebih menyenangkan. Jadi cacat apa yang dideteksi PVS-Studio untuk Java? Hasil verifikasi akan dibahas dalam artikel.

Kenalan dangkal dengan Elasticsearch

Elasticsearch adalah mesin pencarian dan analisis teks lengkap open source yang dapat diskalakan. Ini memungkinkan Anda untuk menyimpan sejumlah besar data, melakukan di antaranya pencarian cepat dan analitik (hampir secara real time). Biasanya, ini digunakan sebagai mekanisme / teknologi yang mendasari yang menyediakan aplikasi dengan fungsi kompleks dan persyaratan pencarian.

Di antara situs utama yang menggunakan Elasticsearch, Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox dicatat.

Saya pikir dengan kenalan sudah cukup.

Bagaimana itu?

Tidak ada masalah dengan verifikasi. Urutan tindakannya cukup sederhana dan tidak memakan banyak waktu:

• Unduh Elasticsearch dari GitHub ;
• Saya menggunakan instruksi untuk memulai penganalisis Java dan meluncurkan analisis;
• Saya menerima laporan analisa, menganalisisnya dan menyoroti kasus-kasus menarik.

Sekarang mari kita langsung ke intinya.

Perhatian Kemungkinan NullPointerException

V6008 Null dereference dari 'line'. GoogleCloudStorageFixture.java (451)

private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { .... // Reads the body line = reader.readLine(); byte[] batchedBody = new byte[0]; if ((line != null) || (line.startsWith("--" + boundary) == false)) // <= { batchedBody = line.getBytes(StandardCharsets.UTF_8); } .... }); .... } 

Kesalahan dalam fragmen kode ini adalah jika Anda tidak bisa membaca baris dari buffer, maka memanggil metode beginWith dalam kondisi pernyataan if akan melempar NullPointerException . Kemungkinan besar, ini adalah kesalahan ketik, dan saat menulis kondisi, operator && lebih dimaksudkan sebagai daripada || .

V6008 Potensi null dereference dari 'followIndexMetadata'. TransportResumeFollowAction.java (171), TransportResumeFollowAction.java (170), TransportResumeFollowAction.java (194)

 void start( ResumeFollowAction.Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ....) throws IOException { MapperService mapperService = followIndexMetadata != null // <= ? .... : null; validate(request, leaderIndexMetadata, followIndexMetadata, // <= leaderIndexHistoryUUIDs, mapperService); .... } 

Peringatan lain dari diagnostik V6008 . Sekarang, melihat lebih dekat telah memusatkan objek followIndexMetadata . Metode awal membutuhkan beberapa masukan argumen, termasuk tersangka kami. Kemudian, atas dasar memeriksa objek kami untuk null , objek baru terbentuk, yang berpartisipasi dalam logika lebih lanjut dari metode ini. Memeriksa null memberitahu kita bahwa followIndexMetadata masih bisa datang dari luar dengan objek nol. Ok, lihat lebih jauh.

Selanjutnya, metode validasi disebut dengan mendorong banyak argumen (sekali lagi, di antaranya adalah objek yang dimaksud). Dan jika Anda melihat penerapan metode validasi, maka semuanya jatuh pada tempatnya. Objek nol potensial kita dilewatkan oleh argumen ketiga ke metode validasi , di mana ia tanpa syarat ditinjau. Akibatnya, NullPointerException potensial .

 static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex, // <= ....) { .... Map<String, String> ccrIndexMetadata = followIndex.getCustomData(....); // <= if (ccrIndexMetadata == null) { throw new IllegalArgumentException(....); } .... }} 

Tidak diketahui dengan argumen apa metode start sebenarnya disebut. Ada kemungkinan bahwa memeriksa semua argumen dilakukan di suatu tempat sebelum pemanggilan metode, dan kami tidak menghadapi dereferencing objek nol. Tetapi, Anda harus mengakui bahwa implementasi kode seperti itu masih terlihat agak tidak dapat diandalkan dan patut mendapat perhatian.

V6060 Referensi 'simpul' digunakan sebelum diverifikasi terhadap nol. RestTasksAction.java (152), RestTasksAction.java (151)

 private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); .... // Node information. Note that the node may be null because it has // left the cluster between when we got this response and now. table.addCell(fullId ? nodeId : Strings.substring(nodeId, 0, 4)); table.addCell(node == null ? "-" : node.getHostAddress()); table.addCell(node.getAddress().address().getPort()); table.addCell(node == null ? "-" : node.getName()); table.addCell(node == null ? "-" : node.getVersion().toString()); .... } 

Aturan diagnostik lain berfungsi di sini, tetapi masalahnya sama: NullPointerException . Aturannya mengatakan: "Kawan, apa yang kamu lakukan? Bagaimana bisa begitu? Oh masalah! Mengapa Anda menggunakan objek pertama, dan kemudian di baris kode berikutnya periksa apakah ada null ?! ” Jadi ternyata di sini mendereferensi objek nol. Sayangnya, bahkan komentar salah satu pengembang tidak membantu.

V6060 Referensi 'penyebab' digunakan sebelum diverifikasi terhadap nol. StartupException.java (76), StartupException.java (73)

 private void printStackTrace(Consumer<String> consumer) { Throwable originalCause = getCause(); Throwable cause = originalCause; if (cause instanceof CreationException) { cause = getFirstGuiceCause((CreationException)cause); } String message = cause.toString(); // <= consumer.accept(message); if (cause != null) { // <= // walk to the root cause while (cause.getCause() != null) { cause = cause.getCause(); } .... } .... } 

Perlu dicatat di sini bahwa metode getCause dari kelas Throwable dapat mengembalikan nol . Selanjutnya, masalah yang dipertimbangkan di atas diulangi, dan tidak masuk akal untuk menjelaskan sesuatu secara rinci.

Kondisi tidak berarti

Ekspresi V6007 's.charAt (i)! =' \ T '' selalu benar. Cron.java (1223)

 private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) { // intentionally empty } return i; } 

Fungsi yang dipertimbangkan mengembalikan indeks spasi pertama, mulai dari indeks i . Apa yang salah Kami mendapat peringatan dari penganalisa bahwa s.charAt (i)! = '\ T' selalu benar, yang berarti bahwa akan selalu ada kebenaran dan ekspresi (s.charAt (i)! = '' || s.charAt (i)! = '\ t') . Benarkah begitu? Saya pikir Anda sendiri dapat dengan mudah memverifikasi ini dengan mengganti karakter apa pun.

Akibatnya, metode ini akan selalu mengembalikan indeks sama dengan s.length () , yang tidak benar. Saya berani berasumsi bahwa metode yang terletak sedikit lebih tinggi ini harus disalahkan:

 private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) { // intentionally empty } return i; } 

Kami menerapkan metode ini, lalu menyalinnya dan membuat beberapa koreksi kecil untuk mendapatkan metode findNextWhiteSpace kami yang salah. Metode itu disesuaikan, disesuaikan, tetapi tidak disesuaikan. Untuk memperbaiki situasi, Anda harus menggunakan operator && alih-alih || .

Ekspresi V6007 'tersisa == 0' selalu salah. PemUtils.java (439)

 private static byte[] generateOpenSslKey(char[] password, byte[] salt, int keyLength) { .... int copied = 0; int remaining; while (copied < keyLength) { remaining = keyLength - copied; .... copied += bytesToCopy; if (remaining == 0) { // <= break; } .... } .... } 

Dari kondisi siklus yang disalin <keyLength, dapat dicatat bahwa disalin akan selalu kurang dari keyLength . Oleh karena itu, perbandingan kesetaraan variabel yang tersisa dengan 0 tidak ada gunanya dan akan selalu memberikan hasil yang salah, dan oleh karena itu kondisi tidak akan keluar dari loop. Apakah kode ini layak dihapus, atau Anda perlu mempertimbangkan kembali logika perilaku? Saya pikir hanya pengembang yang akan dapat dot semua saya.

Ekspresi V6007 'healthCheckDn.indexOf (' = ')> 0' selalu salah. ActiveDirectorySessionFactory.java (73)

 ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(...., () -> { if (....) { final String healthCheckDn = ....; if (healthCheckDn.isEmpty() && healthCheckDn.indexOf('=') > 0) { return healthCheckDn; } } return ....; }, ....); .... } 

Sekali lagi ungkapan yang tidak berarti. Menurut kondisi, untuk ekspresi lambda untuk mengembalikan variabel string healthCheckDn , string healthCheckDn harus kosong dan string yang berisi karakter '=' tidak ada di posisi pertama. Fuh, semacam beres. Dan seperti yang Anda pahami dengan benar, ini tidak mungkin. Kami tidak akan mengerti logika kode, serahkan saja kepada pengembang.

Saya hanya memberikan beberapa contoh yang salah, tetapi selain itu, ada banyak kasus di mana diagnostik V6007 dipicu , yang harus dipertimbangkan secara terpisah dan kesimpulan diambil.

Metode ini kecil, tetapi udalenky

 private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; } 

Jadi, kami memiliki metode kecil beberapa baris. Tapi serangga tidak tidur! Analisis metode ini memberikan hasil sebagai berikut:

1. Ekspresi V6007 '(int) x <0' selalu salah. BCrypt.java (429)
2. V6025 Kemungkinan indeks '(int) x' di luar batas. BCrypt.java (431)

Masalah N1. Ekspresi (int) x <0 selalu salah (Ya, ya, lagi V6007 ). Variabel x tidak boleh negatif, karena ini adalah tipe char . Jenis char adalah bilangan bulat yang tidak ditandatangani. Ini tidak bisa disebut kesalahan nyata, tetapi, bagaimanapun, cek itu berlebihan dan dapat dihapus.

Masalah N2. Kemungkinan kelebihan array yang mengarah ke ArrayIndexOutOfBoundsException . Kemudian muncul pertanyaan, yang terletak di permukaan: "Tunggu, bagaimana dengan memeriksa indeks?"

Jadi, kami memiliki array ukuran tetap dari 128 elemen:

 private static final byte index_64[] = { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, -1, -1, -1, -1, -1, -1, -1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, -1, -1, -1, -1, -1, -1, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, -1, -1, -1, -1, -1 }; 

Ketika variabel x memasukkan input dari metode char64 , ia memeriksa validitas indeks. Di mana celahnya? Mengapa kasus keluar dari array masih dimungkinkan?

Memeriksa (int) x> index_64.length tidak sepenuhnya benar. Jika x dengan nilai 128 tiba pada input metode char64 , maka centang tidak akan melindungi terhadap ArrayIndexOutOfBoundsException . Mungkin ini tidak pernah terjadi dalam kenyataan. Namun, pemeriksaan ini dieja salah, dan Anda perlu mengganti operator "lebih besar dari" (>) dengan "lebih besar dari atau sama dengan" (> =).

Perbandingan yang dicoba

V6013 Numbers 'displaySize' dan 'that.displaySize' dibandingkan dengan referensi. Mungkin perbandingan kesetaraan dimaksudkan. ColumnInfo.java (122)

 .... private final String table; private final String name; private final String esType; private final Integer displaySize; .... @Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } ColumnInfo that = (ColumnInfo) o; return displaySize == that.displaySize && // <= Objects.equals(table, that.table) && Objects.equals(name, that.name) && Objects.equals(esType, that.esType); } 

Kesalahan di sini adalah bahwa objek displaySize tipe Integer dibandingkan melalui operator == , yaitu, mereka dibandingkan dengan referensi. Sangat mungkin bahwa objek ColumnInfo akan dibandingkan, yang bidang displaySize memiliki tautan berbeda tetapi isinya sama. Dan dalam hal ini, perbandingan akan memberi kita hasil negatif, sementara kami mengharapkan kebenaran.

Saya berani menyarankan bahwa perbandingan seperti itu bisa merupakan hasil dari kegagalan refactoring, dan pada awalnya bidang displaySize adalah tipe int .

V6058 Fungsi 'sama dengan' membandingkan objek dari tipe yang tidak kompatibel: Integer, TimeValue. DatafeedUpdate.java (375)

 .... private final TimeValue queryDelay; private final TimeValue frequency; .... private final Integer scrollSize; .... boolean isNoop(DatafeedConfig datafeed) { return (frequency == null || Objects.equals(frequency, datafeed.getFrequency())) && (queryDelay == null || Objects.equals(queryDelay, datafeed.getQueryDelay())) && (scrollSize == null || Objects.equals(scrollSize, datafeed.getQueryDelay())) // <= && ....) } 

Dan lagi perbandingan objek tidak benar. Sekarang bandingkan objek yang tipenya tidak kompatibel ( Integer dan TimeValue ). Hasil perbandingan ini jelas, dan selalu salah. Dapat dilihat bahwa bidang kelas dibandingkan dengan cara yang sama satu sama lain, hanya perlu mengubah nama bidang. Jadi, pengembang memutuskan untuk mempercepat proses penulisan kode dengan copy-paste, tetapi ia memberi dirinya bug yang sama. Kelas mengimplementasikan pengambil untuk bidang scrollSize , jadi untuk memperbaiki kesalahan, solusi yang benar adalah dengan menggunakan metode yang sesuai: datafeed .getScrollSize () .

Mari kita lihat beberapa contoh kesalahan lagi tanpa penjelasan apa pun. Masalahnya sudah jelas.

V6001 Ada sub-ekspresi identik 'tookInMillis' di kiri dan di kanan operator '=='. TermVectorsResponse.java (152)

 @Override public boolean equals(Object obj) { .... return index.equals(other.index) && type.equals(other.type) && Objects.equals(id, other.id) && docVersion == other.docVersion && found == other.found && tookInMillis == tookInMillis // <= && Objects.equals(termVectorList, other.termVectorList); } 

Fungsi V6009 'sama dengan' menerima argumen aneh. Objek 'shardId.getIndexName ()' digunakan sebagai argumen untuk metodenya sendiri. SnapshotShardFailure.java (208)

 @Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) && // <= Objects.equals(reason, that.reason) && Objects.equals(nodeId, that.nodeId) && status.getStatus() == that.status.getStatus(); } 

Lain-lain

V6006 Objek telah dibuat tetapi tidak sedang digunakan. Kata kunci 'lempar' bisa hilang. JdbcConnection.java (88)

 @Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } } 

Bugnya jelas dan tidak memerlukan penjelasan. Pengembang melempar pengecualian, tetapi sama sekali tidak melemparkannya lebih jauh. Pengecualian anonim seperti itu akan berhasil dibuat, dan juga berhasil dan, yang terpenting, dihancurkan tanpa jejak. Alasannya adalah tidak adanya pernyataan melempar .

V6003 Penggunaan pola 'if (A) {....} else if (A) {....}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. MockScriptEngine.java (94), MockScriptEngine.java (105)

 @Override public <T> T compile(....) { .... if (context.instanceClazz.equals(FieldScript.class)) { .... } else if (context.instanceClazz.equals(FieldScript.class)) { .... } else if(context.instanceClazz.equals(TermsSetQueryScript.class)) { .... } else if (context.instanceClazz.equals(NumberSortScript.class)) .... } 

Dalam beberapa konstruksi if-else, salah satu kondisi diulang dua kali, sehingga situasinya memerlukan peninjauan kode yang kompeten.

V6039 Ada dua pernyataan 'jika' dengan ekspresi kondisional yang identik. Pernyataan 'jika' pertama berisi pengembalian metode. Ini berarti bahwa pernyataan 'jika' yang kedua tidak masuk akal. SearchAfterBuilder.java (94), SearchAfterBuilder.java (93)

 public SearchAfterBuilder setSortValues(Object[] values) { .... for (int i = 0; i < values.length; i++) { if (values[i] == null) continue; if (values[i] instanceof String) continue; if (values[i] instanceof Text) continue; if (values[i] instanceof Long) continue; if (values[i] instanceof Integer) continue; if (values[i] instanceof Short) continue; if (values[i] instanceof Byte) continue; if (values[i] instanceof Double) continue; if (values[i] instanceof Float) continue; if (values[i] instanceof Boolean) continue; // <= if (values[i] instanceof Boolean) continue; // <= throw new IllegalArgumentException(....); } .... } 

Kondisi yang sama digunakan dua kali berturut-turut. Kondisi kedua berlebihan, atau perlu menggunakan jenis yang berbeda, bukan Boolean ?

Fungsi V6009 'substring' menerima argumen aneh. Argumen 'queryStringIndex + 1' tidak boleh lebih besar dari 'queryStringLength'. LoggingAuditTrail.java (660)

 LogEntryBuilder withRestUriAndMethod(RestRequest request) { final int queryStringIndex = request.uri().indexOf('?'); int queryStringLength = request.uri().indexOf('#'); if (queryStringLength < 0) { queryStringLength = request.uri().length(); } if (queryStringIndex < 0) { logEntry.with(....); } else { logEntry.with(....); } if (queryStringIndex > -1) { logEntry.with(...., request.uri().substring(queryStringIndex + 1,// <= queryStringLength)); // <= } .... } 

Segera pertimbangkan skenario kesalahan yang bisa melempar StringIndexOutOfBoundsException . Pengecualian akan terjadi ketika request.uri () mengembalikan string yang berisi karakter '#' lebih awal dari '?'. Untuk kasus seperti itu, tidak ada pemeriksaan dalam metode ini, dan jika ini masih terjadi, maka bencana tidak akan dapat dihindari. Mungkin ini tidak akan pernah terjadi karena berbagai pemeriksaan objek permintaan di luar metode, tetapi menurut saya, berharap ini bukan ide terbaik.

Kesimpulan

Selama bertahun-tahun, PVS-Studio telah membantu menemukan kekurangan dalam kode proyek sumber terbuka komersial dan gratis. Baru-baru ini, Java telah ditambahkan untuk mendukung bahasa yang diurai. Dan salah satu tes pertama untuk pendatang baru kami adalah Elasticsearch yang aktif berkembang. Kami berharap pemeriksaan ini bermanfaat bagi proyek dan menarik bagi pembaca.

Agar PVS-Studio untuk Java dapat dengan cepat beradaptasi dengan dunia baru untuk dirinya sendiri, kami membutuhkan pengujian baru, pengguna baru, umpan balik aktif, dan pelanggan :). Jadi, saya sarankan, tanpa penundaan, unduh dan uji penganalisa kami pada draft kerja Anda!

Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan ke terjemahan: Maxim Stefanov. PVS-Studio untuk Jawa menyentuh jalan. Perhentian berikutnya adalah Elasticsearch