PVS-Studio untuk Jawa menyentuh jalan. Perhentian berikutnya adalah Elasticsearch

Tim PVS-Studio telah menjaga blog tentang pemeriksaan proyek sumber terbuka oleh penganalisa kode statis dengan nama yang sama selama bertahun-tahun. Hingga saat ini, lebih dari 300 proyek telah diperiksa, pangkalan kesalahan berisi lebih dari 12000 kasus. Awalnya alat analisa diimplementasikan untuk memeriksa kode C dan C ++, dukungan C # ditambahkan kemudian. Oleh karena itu, dari semua proyek yang diperiksa mayoritas (> 80%) menyumbang C dan C ++. Baru-baru ini Java ditambahkan ke daftar bahasa yang didukung, yang berarti bahwa sekarang ada dunia terbuka baru untuk PVS-Studio, jadi sekarang saatnya untuk melengkapi pangkalan dengan kesalahan dari proyek-proyek Java.

Dunia Jawa sangat luas dan beragam, sehingga orang bahkan tidak tahu ke mana harus mencari pertama ketika memilih proyek untuk menguji analisa baru. Pada akhirnya, pilihan ada pada pencarian teks lengkap dan mesin analitik Elasticsearch. Ini adalah proyek yang cukup sukses, dan bahkan sangat menyenangkan untuk menemukan kesalahan dalam proyek yang signifikan. Jadi, cacat apa yang berhasil dideteksi PVS-Studio untuk Java? Pembicaraan lebih lanjut akan benar tentang hasil cek.

Secara singkat tentang elasticsearch

Elasticsearch adalah mesin pencari dan analitik terdistribusi yang tenang dengan kode sumber terbuka, yang mampu memecahkan semakin banyak kasus penggunaan. Ini memungkinkan Anda untuk menyimpan sejumlah besar data, melakukan pencarian cepat dan analitik (hampir dalam mode waktu nyata). Biasanya, ini digunakan sebagai mekanisme / teknologi yang mendasari, yang menyediakan aplikasi dengan fungsi kompleks dan persyaratan pencarian.

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

Baik, cukup perkenalan.

Seluruh cerita tentang bagaimana keadaannya

Tidak ada masalah dengan cek itu sendiri. Urutan tindakan agak sederhana dan tidak memakan banyak waktu:

• Unduh Elasticsearch dari GitHub ;
• Ikuti instruksi bagaimana menjalankan penganalisis Java dan menjalankan analisis;
• Menerima laporan penganalisa, menyelidiki dan menunjukkan kasus-kasus menarik.

Sekarang mari kita beralih ke poin utama.

Hati-hati! 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 string dari buffer tidak dibaca, panggilan metode beginWith dalam kondisi pernyataan if akan menghasilkan melempar pengecualian NullPointerException . Kemungkinan besar, ini adalah kesalahan ketik dan ketika menulis kondisi pengembang berarti operator && alih-alih || .

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 . Objek followIndexMetadata menyalakan minat saya. Metode awal menerima beberapa argumen sebagai masukan, tersangka kami benar di antara mereka. Setelah itu, berdasarkan memeriksa objek kami untuk null, objek baru dibuat, yang terlibat dalam logika metode lebih lanjut. Periksa nol menunjukkan kepada kita bahwa followIndexMetadata masih dapat datang dari luar sebagai objek nol. Baiklah, mari kita lihat lebih jauh.

Kemudian beberapa argumen didorong ke metode validasi (sekali lagi, ada objek yang kami anggap di antara mereka). Jika kita melihat penerapan metode validasi, semuanya akan berlaku. Objek nol potensial kami diteruskan ke metode validasi sebagai argumen ketiga, di mana ia tanpa syarat akan direferensikan. Potensi NullPointerException sebagai hasilnya.

 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(....); } .... }} 

Kami tidak tahu pasti dengan argumen apa metode awal dipanggil. Sangat mungkin bahwa semua argumen diperiksa di suatu tempat sebelum memanggil metode dan tidak ada dereferensi objek nol yang akan terjadi. Bagaimanapun, kita harus mengakui bahwa implementasi kode seperti itu masih terlihat 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 dengan masalah yang sama dipicu di sini. NullPointerException . Aturan itu berteriak untuk pengembang: "Guys, apa yang kamu lakukan? Bagaimana kamu bisa melakukan itu? Oh, itu mengerikan! Mengapa Anda pertama kali menggunakan objek dan memeriksa apakah ada nol di baris berikutnya? Berikut adalah bagaimana dereferensi objek nol terjadi. Sayangnya, bahkan komentar 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(); } .... } .... } 

Dalam hal ini kita harus memperhitungkan bahwa metode getCause dari kelas Throwable mungkin mengembalikan nol . Masalah di atas berulang lebih lanjut, jadi penjelasannya tidak perlu.

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 karakter spasi pertama, mulai dari indeks i . Apa yang salah Kami memiliki peringatan penganalisa bahwa s.charAt (i)! = '\ T' selalu benar, yang berarti ungkapan (s.charAt (i)! = '' || s.charAt (i)! = '\ T ') akan selalu benar juga. Apakah ini benar? Saya pikir, Anda dapat dengan mudah memastikan ini, dengan mengganti karakter apa pun.

Akibatnya, metode ini akan selalu mengembalikan indeks, sama dengan s.length () , yang salah. Saya berani menyarankan bahwa metode di atas yang harus disalahkan di sini:

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

Pengembang menerapkan metode ini, lalu menyalin dan mendapatkan metode kami yang salah findNextWhiteSpace, setelah melakukan beberapa pengeditan. Mereka terus memperbaiki dan memperbaiki metode tetapi belum memperbaikinya. Untuk memperbaikinya, orang 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 loop <keyLength yang kita salin dapat kita catat, yang disalin akan selalu kurang dari keyLength . Oleh karena itu, tidak ada gunanya membandingkan variabel yang tersisa dengan 0, dan itu akan selalu salah, pada titik mana loop tidak akan keluar oleh suatu kondisi. Hapus kode atau pertimbangkan kembali logika perilaku? Saya pikir, hanya pengembang yang dapat menempatkan semua titik di atas i.

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 ....; }, ....); .... } 

Ekspresi tak berarti lagi. Menurut kondisi tersebut, string healthCheckDn harus kosong dan berisi karakter '=' tidak di posisi pertama, sehingga ekspresi lambda akan mengembalikan variabel string healthCheckDn . Fiuh, itu saja kalau begitu! Seperti yang mungkin Anda pahami, itu tidak mungkin. Kami tidak akan masuk jauh ke dalam kode, mari serahkan pada kebijaksanaan pengembang.

Saya hanya mengutip beberapa contoh yang salah, tetapi di luar itu ada banyak pemicu diagnostik V6007 , yang harus dipertimbangkan satu per satu, membuat kesimpulan yang relevan.

Metode kecil bisa sangat bermanfaat

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

Jadi di sini kita memiliki metode mungil-mungil dari beberapa baris. Tapi bug ada di jam tangan! 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)

Edisi N1. Ekspresi (int) x <0 selalu salah (Ya, V6007 lagi). Variabel x tidak boleh negatif, karena merupakan tipe char . Jenis char adalah bilangan bulat yang tidak ditandatangani. Itu tidak bisa disebut kesalahan nyata, tetapi, meskipun demikian, cek itu berlebihan dan dapat dihapus.

Edisi N2. Kemungkinan indeks array keluar dari batas, menghasilkan pengecualian ArrayIndexOutOfBoundsException . Kemudian muncul pertanyaan, yang terletak di permukaan: "Tunggu, bagaimana dengan cek 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 metode char64 menerima variabel x , validitas indeks akan diperiksa. Dimana cacatnya? Mengapa indeks array di luar batas masih dimungkinkan?

Pemeriksaan (int) x> index_64.length tidak cukup benar. Jika metode char64 menerima x dengan nilai 128, centang tidak akan melindungi ArrayIndexOutOfBoundsException. Mungkin ini tidak pernah terjadi dalam kenyataan. Namun, cek ditulis secara tidak benar, dan seseorang harus mengubah operator "lebih besar dari" (>) dengan "lebih besar dari atau sama dengan (> =).

Perbandingan, yang melakukan yang terbaik

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); } 

Apa yang salah di sini adalah objek displaySize dari tipe Integer dibandingkan dengan menggunakan operator == , yaitu, dengan referensi. Sangat mungkin bahwa objek ColumnInfo , yang bidang displaySize memiliki referensi berbeda tetapi konten yang sama, akan dibandingkan. Dalam hal ini, perbandingan akan memberi kita hasil negatif, ketika kita diharapkan untuk menjadi kenyataan.

Saya berani menebak 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())) // <= && ....) } 

Perbandingan objek salah lagi. Objek waktu ini dengan tipe yang tidak kompatibel dibandingkan ( Integer dan TimeValue ). Hasil perbandingan ini jelas, dan selalu salah. Anda dapat melihat bahwa bidang kelas dibandingkan satu sama lain, hanya nama bidang yang harus diubah. Inilah intinya - pengembang memutuskan untuk mempercepat proses dengan menggunakan copy-paste dan memasukkan bug ke dalam tawar-menawar. Kelas mengimplementasikan getter untuk bidang scrollSize , jadi untuk memperbaiki kesalahan, orang harus menggunakan metode datafeed .getScrollSize ().

Mari kita lihat beberapa contoh yang salah tanpa penjelasan. Masalahnya cukup 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 membuat pengecualian, tetapi tidak membuangnya di tempat lain. Pengecualian anonim seperti itu berhasil dibuat, dan, yang paling penting, akan dihapus dengan mulus. Alasannya adalah kurangnya operator 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 hal, salah satu syarat 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. Apakah kondisi kedua berlebihan atau haruskah jenis lain digunakan alih-alih 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)); // <= } .... } 

Mari kita pertimbangkan di sini skenario keliru yang dapat menyebabkan pengecualian StringIndexOutOfBoundsException . Pengecualian akan terjadi ketika request.uri () mengembalikan string yang berisi karakter '#' sebelum '?'. Tidak ada pemeriksaan dalam metode ini, jadi jika itu terjadi, masalahnya sedang terjadi. Mungkin, ini tidak akan pernah terjadi karena berbagai pemeriksaan objek di luar metode, tetapi menetapkan harapan pada ini bukan ide terbaik.

Kesimpulan

Selama bertahun-tahun, PVS-Studio telah membantu menemukan cacat dalam kode proyek sumber terbuka komersial dan gratis. Baru-baru ini Java telah bergabung dengan daftar bahasa yang didukung untuk analisis. Elasticsearch menjadi salah satu tes pertama untuk pendatang baru kami. Kami berharap pemeriksaan ini bermanfaat bagi proyek dan menarik bagi pembaca.

PVS-Studio untuk Java membutuhkan tantangan baru, pengguna baru, umpan balik aktif, dan klien agar dapat dengan cepat beradaptasi dengan dunia baru :). Jadi saya mengundang Anda untuk mengunduh dan mencoba analisa kami pada proyek kerja Anda segera!