Ini adalah ulasan kedua dalam serangkaian artikel tentang pengujian program sumber terbuka untuk bekerja dengan protokol RDP. Di dalamnya kita akan melihat klien rdesktop dan server xrdp.
Digunakan sebagai alat untuk mengidentifikasi kesalahan . Ini adalah penganalisis kode statis untuk bahasa C, C++, C# dan Java, tersedia di platform Windows, Linux dan macOS.
Artikel ini hanya menyajikan kesalahan-kesalahan yang menurut saya menarik. Namun, proyeknya kecil, jadi hanya ada sedikit kesalahan :).
Catatan. Artikel sebelumnya tentang verifikasi proyek FreeRDP dapat ditemukan .
desktop
— implementasi gratis klien RDP untuk sistem berbasis UNIX. Ini juga dapat digunakan di Windows jika Anda membangun proyek di bawah Cygwin. Berlisensi di bawah GPLv3.
Klien ini sangat populer - digunakan secara default di ReactOS, dan Anda juga dapat menemukan front-end grafis pihak ketiga untuk klien ini. Namun, dia sudah cukup tua: rilis pertamanya terjadi pada tanggal 4 April 2001 - pada saat penulisan, dia berusia 17 tahun.
Seperti yang saya sebutkan sebelumnya, proyek ini cukup kecil. Ini berisi sekitar 30 ribu baris kode, yang agak aneh mengingat usianya. Sebagai perbandingan, FreeRDP berisi 320 ribu baris. Berikut keluaran program Cloc:
Kode yang tidak dapat dijangkau
Kode yang tidak tersedia terdeteksi. Mungkin saja ada kesalahan. rdesktop.c 1502
int
main(int argc, char *argv[])
{
....
return handle_disconnect_reason(deactivated, ext_disc_reason);
if (g_redirect_username)
xfree(g_redirect_username);
xfree(g_username);
}Kesalahan langsung menemui kita di fungsinya utama: kita melihat kode muncul setelah operator kembali — fragmen ini melakukan pembersihan memori. Namun, kesalahan tersebut tidak menimbulkan ancaman: semua memori yang dialokasikan akan dihapus oleh sistem operasi setelah program keluar.
Tidak ada penanganan kesalahan
Array yang tidak berjalan mungkin terjadi. Nilai indeks 'n' bisa mencapai -1. rdesktop.c 1872
RD_BOOL
subprocess(char *const argv[], str_handle_lines_t linehandler, void *data)
{
int n = 1;
char output[256];
....
while (n > 0)
{
n = read(fd[0], output, 255);
output[n] = ' '; // <=
str_handle_lines(output, &rest, linehandler, data);
}
....
}Cuplikan kode dalam hal ini dibaca dari file ke dalam buffer hingga file berakhir. Namun, tidak ada penanganan kesalahan di sini: jika terjadi kesalahan, maka Baca baca akan mengembalikan -1, dan kemudian array akan dibanjiri keluaran.
Menggunakan EOF dalam tipe char
EOF tidak boleh dibandingkan dengan nilai tipe 'char'. '(c = fgetc(fp))' harus bertipe 'int'. ctrl.c 500
int
ctrl_send_command(const char *cmd, const char *arg)
{
char result[CTRL_RESULT_SIZE], c, *escaped;
....
while ((c = fgetc(fp)) != EOF && index < CTRL_RESULT_SIZE && c != 'n')
{
result[index] = c;
index++;
}
....
}Di sini kita melihat penanganan yang salah saat mencapai akhir file: if fgetc mengembalikan karakter yang kodenya 0xFF, itu akan diartikan sebagai akhir file (EOF).
EOF itu adalah konstanta, biasanya didefinisikan sebagai -1. Misalnya, dalam pengkodean CP1251, huruf terakhir alfabet Rusia memiliki kode 0xFF, yang sesuai dengan angka -1 jika kita berbicara tentang variabel seperti tangki. Ternyata simbolnya 0xFF, seperti EOF (-1) diartikan sebagai akhir file. Untuk menghindari kesalahan seperti itu, hasil dari fungsinya adalah fgetc harus disimpan dalam variabel seperti int.
salah ketik
Fragmen 1
Ekspresi 'write_time' selalu salah. disk.c 805
RD_NTSTATUS
disk_set_information(....)
{
time_t write_time, change_time, access_time, mod_time;
....
if (write_time || change_time)
mod_time = MIN(write_time, change_time);
else
mod_time = write_time ? write_time : change_time; // <=
....
}Mungkin pembuat kode ini salah || и && dalam kondisi. Mari kita pertimbangkan opsi nilai yang memungkinkan waktu_tulis и perubahan_waktu:
- Kedua variabel sama dengan 0: dalam hal ini kita akan berakhir di sebuah cabang lain: variabel mod_time akan selalu 0 terlepas dari kondisi selanjutnya.
- Salah satu variabelnya adalah 0: mod_time akan sama dengan 0 (asalkan variabel lain bernilai non-negatif), karena MIN akan memilih yang lebih kecil dari dua opsi.
- Kedua variabel tidak sama dengan 0: pilih nilai minimum.
Saat mengganti kondisi dengan waktu_tulis && waktu_perubahan perilakunya akan terlihat benar:
- Salah satu atau kedua variabel tidak sama dengan 0: pilih nilai yang bukan nol.
- Kedua variabel tidak sama dengan 0: pilih nilai minimum.
Fragmen 2
Ekspresi selalu benar. Mungkin operator '&&' harus digunakan di sini. disk.c 1419
static RD_NTSTATUS
disk_device_control(RD_NTHANDLE handle, uint32 request, STREAM in,
STREAM out)
{
....
if (((request >> 16) != 20) || ((request >> 16) != 9))
return RD_STATUS_INVALID_PARAMETER;
....
}Rupanya operator juga ikut campur di sini || и &&Atau == и !=: Suatu variabel tidak boleh memiliki nilai 20 dan 9 secara bersamaan.
Penyalinan baris tanpa batas
Pemanggilan fungsi 'sprintf' akan menyebabkan buffer 'fullpath' meluap. disk.c 1257
RD_NTSTATUS
disk_query_directory(....)
{
....
char *dirname, fullpath[PATH_MAX];
....
/* Get information for directory entry */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Jika Anda melihat fungsinya secara lengkap, terlihat jelas bahwa kode ini tidak menimbulkan masalah. Namun, hal tersebut mungkin muncul di masa mendatang: satu perubahan yang ceroboh dan kita akan mendapatkan buffer overflow - lari cepat tidak dibatasi oleh apa pun, jadi saat menggabungkan jalur, kita bisa melampaui batas array. Disarankan untuk memperhatikan panggilan ini snprintf(jalur lengkap, PATH_MAX,….).
Kondisi berlebihan
Bagian dari ekspresi kondisional selalu benar: tambahkan > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка tambahkan > 0 tidak perlu di sini: variabel akan selalu lebih besar dari nol, karena membaca% 4 akan mengembalikan sisa pembagian, tetapi tidak akan pernah sama dengan 4.
xrdp
— implementasi server RDP dengan kode sumber terbuka. Proyek ini dibagi menjadi 2 bagian:
- xrdp - implementasi protokol. Didistribusikan di bawah lisensi Apache 2.0.
- xorgxrdp - Satu set driver Xorg untuk digunakan dengan xrdp. Lisensi - X11 (seperti MIT, tetapi melarang penggunaan dalam periklanan)
Pengembangan proyek ini didasarkan pada hasil rdesktop dan FreeRDP. Awalnya, untuk bekerja dengan grafik, Anda harus menggunakan server VNC terpisah, atau server X11 khusus dengan dukungan RDP - X11rdp, tetapi dengan munculnya xorgxrdp, kebutuhan akan server tersebut menghilang.
Pada artikel ini kami tidak akan membahas xorgxrdp.
Proyek xrdp, seperti proyek sebelumnya, berukuran sangat kecil dan berisi sekitar 80 ribu baris.
Lebih banyak kesalahan ketik
Kode tersebut berisi kumpulan blok serupa. Centang item 'r', 'g', 'r' pada baris 87, 88, 89. rfxencode_rgb_to_yuv.c 87
static int
rfx_encode_format_rgb(const char *rgb_data, int width, int height,
int stride_bytes, int pixel_format,
uint8 *r_buf, uint8 *g_buf, uint8 *b_buf)
{
....
switch (pixel_format)
{
case RFX_FORMAT_BGRA:
....
while (x < 64)
{
*lr_buf++ = r;
*lg_buf++ = g;
*lb_buf++ = r; // <=
x++;
}
....
}
....
}Kode ini diambil dari perpustakaan librfxcodec, yang mengimplementasikan codec jpeg2000 untuk RemoteFX. Di sini, tampaknya, saluran data grafis tercampur - alih-alih warna "biru", "merah" yang direkam. Kesalahan ini kemungkinan besar muncul akibat copy-paste.
Masalah yang sama terjadi pada fungsi serupa rfx_encode_format_argb, yang juga diberitahukan oleh penganalisa kepada kami:
Kode tersebut berisi kumpulan blok serupa. Centang item 'a', 'r', 'g', 'r' pada baris 260, 261, 262, 263. rfxencode_rgb_to_yuv.c 260
while (x < 64)
{
*la_buf++ = a;
*lr_buf++ = r;
*lg_buf++ = g;
*lb_buf++ = r;
x++;
}Deklarasi Array
Pembebanan array mungkin terjadi. Nilai indeks 'i — 8' bisa mencapai 129. genkeymap.c 142
// evdev-map.c
int xfree86_to_evdev[137-8+1] = {
....
};
// genkeymap.c
extern int xfree86_to_evdev[137-8];
int main(int argc, char **argv)
{
....
for (i = 8; i <= 137; i++) /* Keycodes */
{
if (is_evdev)
e.keycode = xfree86_to_evdev[i-8];
....
}
....
}Deklarasi dan definisi array dalam kedua file ini tidak kompatibel - ukurannya berbeda 1. Namun, tidak ada kesalahan yang terjadi - ukuran yang benar ditentukan dalam file evdev-map.c, jadi tidak ada batasan. Jadi ini hanya bug yang bisa diperbaiki dengan mudah.
Perbandingan yang salah
Bagian dari ekspresi kondisional selalu salah: (cap_len < 0). xrdp_caps.c 616
// common/parse.h
#if defined(B_ENDIAN) || defined(NEED_ALIGN)
#define in_uint16_le(s, v) do
....
#else
#define in_uint16_le(s, v) do
{
(v) = *((unsigned short*)((s)->p));
(s)->p += 2;
} while (0)
#endif
int
xrdp_caps_process_confirm_active(struct xrdp_rdp *self, struct stream *s)
{
int cap_len;
....
in_uint16_le(s, cap_len);
....
if ((cap_len < 0) || (cap_len > 1024 * 1024))
{
....
}
....
}Fungsi membaca variabel tipe pendek unsigned menjadi variabel seperti int. Pengecekan tidak diperlukan disini karena kita membaca variabel unsigned dan menugaskan hasilnya ke variabel yang lebih besar, sehingga variabel tersebut tidak dapat mengambil nilai negatif.
Pemeriksaan yang tidak perlu
Bagian dari ekspresi kondisional selalu benar: (bpp != 16). libxrdp.c 704
int EXPORT_CC
libxrdp_send_pointer(struct xrdp_session *session, int cache_idx,
char *data, char *mask, int x, int y, int bpp)
{
....
if ((bpp == 15) && (bpp != 16) && (bpp != 24) && (bpp != 32))
{
g_writeln("libxrdp_send_pointer: error");
return 1;
}
....
}Pemeriksaan ketimpangan tidak masuk akal di sini karena kita sudah mempunyai perbandingan di awal. Kemungkinan ini salah ketik dan pengembang ingin menggunakan operator tersebut || untuk menyaring argumen yang tidak valid.
Kesimpulan
Selama audit, tidak ditemukan kesalahan serius, namun banyak ditemukan kekurangan. Namun, desain ini digunakan di banyak sistem, meskipun cakupannya kecil. Sebuah proyek kecil belum tentu memiliki banyak kesalahan, jadi Anda tidak boleh menilai kinerja penganalisis hanya pada proyek kecil. Anda dapat membaca lebih lanjut tentang ini di artikel “".
Anda dapat mengunduh PVS-Studio versi uji coba dari kami .
Jika Anda ingin membagikan artikel ini kepada audiens berbahasa Inggris, silakan gunakan tautan terjemahan: Sergey Larin.
Sumber: www.habr.com