Ini adalah ulasan kedua dalam siri artikel tentang menguji program sumber terbuka untuk bekerja dengan protokol RDP. Di dalamnya kita akan melihat klien rdesktop dan pelayan xrdp.
Digunakan sebagai alat untuk mengenal pasti ralat . Ia ialah penganalisis kod statik untuk bahasa C, C++, C# dan Java, tersedia pada platform Windows, Linux dan macOS.
Artikel itu hanya memaparkan ralat yang kelihatan menarik bagi saya. Walau bagaimanapun, projek itu kecil, jadi terdapat sedikit kesilapan :).
Nota. Artikel sebelumnya tentang pengesahan projek FreeRDP boleh didapati .
rdesktop
— pelaksanaan percuma klien RDP untuk sistem berasaskan UNIX. Ia juga boleh digunakan di bawah Windows jika anda membina projek di bawah Cygwin. Dilesenkan di bawah GPLv3.
Pelanggan ini sangat popular - ia digunakan secara lalai dalam ReactOS, dan anda juga boleh mencari bahagian hadapan grafik pihak ketiga untuknya. Walau bagaimanapun, dia sudah agak tua: pelepasan pertamanya berlaku pada 4 April 2001 - pada masa penulisan, dia berusia 17 tahun.
Seperti yang saya nyatakan sebelum ini, projek itu agak kecil. Ia mengandungi kira-kira 30 ribu baris kod, yang agak pelik memandangkan usianya. Sebagai perbandingan, FreeRDP mengandungi 320 ribu baris. Berikut ialah output program Cloc:
Kod tidak boleh dicapai
Kod tidak tersedia dikesan. Ada kemungkinan ralat berlaku. 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);
}Ralat menemui kami serta-merta dalam fungsi utama: kita lihat kod itu datang selepas pengendali pulangan — serpihan ini melakukan pembersihan memori. Walau bagaimanapun, ralat tidak menimbulkan ancaman: semua memori yang diperuntukkan akan dikosongkan oleh sistem pengendalian selepas program keluar.
Tiada pengendalian ralat
Array underrun adalah mungkin. Nilai indeks 'n' boleh 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);
}
....
}Coretan kod dalam kes ini dibaca daripada fail ke dalam penimbal sehingga fail tamat. Walau bagaimanapun, tiada pengendalian ralat di sini: jika ada masalah, maka membaca akan kembali -1, dan kemudian tatasusunan akan ditakluki output.
Menggunakan EOF dalam jenis char
EOF tidak boleh dibandingkan dengan nilai jenis 'char'. '(c = fgetc(fp))' hendaklah daripada jenis '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 pengendalian yang salah untuk mencapai penghujung fail: if fgetc mengembalikan aksara yang kodnya ialah 0xFF, ia akan ditafsirkan sebagai penghujung fail (EOF).
EOF ia adalah pemalar, biasanya ditakrifkan sebagai -1. Sebagai contoh, dalam pengekodan CP1251, huruf terakhir abjad Rusia mempunyai kod 0xFF, yang sepadan dengan nombor -1 jika kita bercakap tentang pembolehubah seperti tangki. Ternyata simbol 0xFF, seperti EOF (-1) ditafsirkan sebagai penghujung fail. Untuk mengelakkan ralat tersebut, hasil daripada fungsi tersebut ialah fgetc hendaklah disimpan dalam pembolehubah seperti int.
Tipu
Serpihan 1
Ungkapan 'write_time' sentiasa palsu. cakera.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 pengarang kod ini tersilap || и && dalam keadaan. Mari kita pertimbangkan pilihan yang mungkin untuk nilai masa_tulis и perubahan_masa:
- Kedua-dua pembolehubah adalah sama dengan 0: dalam kes ini kita akan berakhir di cawangan lagi: pembolehubah mod_time akan sentiasa 0 tanpa mengira keadaan seterusnya.
- Salah satu pembolehubah ialah 0: mod_time akan sama dengan 0 (dengan syarat pembolehubah lain mempunyai nilai bukan negatif), kerana MIN akan memilih yang lebih kecil daripada dua pilihan.
- Kedua-dua pembolehubah tidak sama dengan 0: pilih nilai minimum.
Apabila menggantikan keadaan dengan masa_tulis && tukar_masa tingkah laku akan kelihatan betul:
- Satu atau kedua-dua pembolehubah tidak sama dengan 0: pilih nilai bukan sifar.
- Kedua-dua pembolehubah tidak sama dengan 0: pilih nilai minimum.
Serpihan 2
Ungkapan sentiasa benar. Mungkin pengendali '&&' harus digunakan di sini. cakera.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;
....
}Rupa-rupanya operator bercampur aduk di sini juga || и &&Atau == и !=: Pembolehubah tidak boleh mempunyai nilai 20 dan 9 pada masa yang sama.
Penyalinan talian tanpa had
Panggilan fungsi 'sprintf' akan menyebabkan limpahan 'fullpath' penampan. cakera.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);
....
}Apabila anda melihat fungsi sepenuhnya, ia akan menjadi jelas bahawa kod ini tidak menyebabkan masalah. Walau bagaimanapun, ia mungkin timbul pada masa hadapan: satu perubahan cuai dan kita akan mendapat limpahan penampan - pecut tidak terhad oleh apa-apa, jadi apabila menggabungkan laluan kita boleh melangkaui sempadan tatasusunan. Adalah disyorkan untuk melihat panggilan ini dihidupkan snprintf(fullpath, PATH_MAX, ….).
Keadaan berlebihan
Sebahagian daripada ungkapan bersyarat sentiasa benar: tambah > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка tambah > 0 tidak ada keperluan di sini: pembolehubah akan sentiasa lebih besar daripada sifar, kerana baca % 4 akan mengembalikan baki bahagian, tetapi ia tidak akan sama dengan 4.
xrdp
— pelaksanaan pelayan RDP dengan kod sumber terbuka. Projek ini dibahagikan kepada 2 bahagian:
- xrdp - pelaksanaan protokol. Diedarkan di bawah lesen Apache 2.0.
- xorgxrdp - Satu set pemacu Xorg untuk digunakan dengan xrdp. Lesen - X11 (seperti MIT, tetapi melarang penggunaan dalam pengiklanan)
Pembangunan projek adalah berdasarkan hasil rdesktop dan FreeRDP. Pada mulanya, untuk bekerja dengan grafik, anda perlu menggunakan pelayan VNC yang berasingan, atau pelayan X11 khas dengan sokongan RDP - X11rdp, tetapi dengan kemunculan xorgxrdp, keperluan untuk mereka hilang.
Dalam artikel ini kami tidak akan membincangkan xorgxrdp.
Projek xrdp, seperti yang sebelumnya, sangat kecil dan mengandungi kira-kira 80 ribu baris.
Lebih banyak kesilapan menaip
Kod tersebut mengandungi koleksi blok yang serupa. Semak item 'r', 'g', 'r' dalam 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++;
}
....
}
....
}Kod ini diambil daripada perpustakaan librfxcodec, yang melaksanakan codec jpeg2000 untuk RemoteFX. Di sini, nampaknya, saluran data grafik bercampur - bukannya warna "biru", "merah" direkodkan. Ralat ini kemungkinan besar muncul akibat salin-tampal.
Masalah yang sama berlaku dalam fungsi yang sama rfx_encode_format_argb, yang juga diberitahu oleh penganalisis kepada kami:
Kod tersebut mengandungi koleksi blok yang serupa. Semak item 'a', 'r', 'g', 'r' dalam 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++;
}Pengisytiharan Array
Array overrun adalah mungkin. Nilai indeks 'i — 8' boleh 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];
....
}
....
}Pengisytiharan dan takrif tatasusunan dalam kedua-dua fail ini tidak serasi - saiznya berbeza sebanyak 1. Walau bagaimanapun, tiada ralat berlaku - saiz yang betul dinyatakan dalam fail evdev-map.c, jadi tidak ada di luar sempadan. Jadi ini hanyalah pepijat yang boleh diperbaiki dengan mudah.
Perbandingan yang salah
Sebahagian daripada ungkapan bersyarat sentiasa palsu: (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 pembolehubah jenis tidak bertanda pendek menjadi pembolehubah seperti int. Semakan tidak diperlukan di sini kerana kita sedang membaca pembolehubah yang tidak ditandatangani dan memberikan hasilnya kepada pembolehubah yang lebih besar, jadi pembolehubah tidak boleh mengambil nilai negatif.
Pemeriksaan yang tidak perlu
Sebahagian daripada ungkapan bersyarat sentiasa 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 ketidaksamaan tidak masuk akal di sini kerana kita sudah mempunyai perbandingan pada mulanya. Berkemungkinan ini adalah kesilapan menaip dan pembangun mahu menggunakan pengendali || untuk menapis hujah yang tidak sah.
Kesimpulan
Semasa pengauditan, tiada kesilapan serius yang dikenal pasti, tetapi banyak kekurangan ditemui. Walau bagaimanapun, reka bentuk ini digunakan dalam banyak sistem, walaupun skopnya kecil. Projek kecil tidak semestinya mempunyai banyak ralat, jadi anda tidak seharusnya menilai prestasi penganalisis hanya pada projek kecil. Anda boleh membaca lebih lanjut mengenai ini dalam artikel "".
Anda boleh memuat turun versi percubaan PVS-Studio daripada kami .
Jika anda ingin berkongsi artikel ini dengan khalayak berbahasa Inggeris, sila gunakan pautan terjemahan: Sergey Larin.
Sumber: www.habr.com