Questa hè a seconda rivista in una seria d'articuli nantu à a prova di prugrammi open source per travaglià cù u protocolu RDP. In questu, guardemu u cliente rdesktop è u servitore xrdp.
Adupratu com'è strumentu per identificà errori Hè un analizzatore di codice staticu per C, C++, C# è Java, dispunibule nantu à e piattaforme Windows, Linux и macOS.
L'articulu presenta solu quelli errori chì mi parevanu interessanti. Tuttavia, i prughjetti sò chjuchi, cusì ci sò stati pochi sbagli :).
Vita. Un articulu precedente nantu à a verificazione di u prugettu FreeRDP pò esse truvatu .
rdesktop
— una implementazione gratuita di u cliente RDP per i sistemi basati annantu à UNIX. Pò esse ancu adupratu sottu Windows, sè custruite u prugettu sottu Cygwin. Licenziatu sottu GPLv3.
Stu cliente hè assai populari - hè utilizatu per difettu in ReactOS, è pudete ancu truvà front-end grafichi di terzu per questu. In ogni casu, hè abbastanza vechju: a so prima liberazione hè accaduta u 4 d'aprile di u 2001 - à u mumentu di a scrittura, hà 17 anni.
Comu aghju nutatu prima, u prugettu hè abbastanza chjucu. Contene circa 30 mila linee di codice, chì hè un pocu stranu in cunsiderà a so età. Per paragone, FreeRDP cuntene 320 mila linee. Eccu l'output di u prugramma Cloc:
Codice inaccessibile
Codice indisponibile rilevatu. Hè pussibule chì un errore hè presente. 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);
}L'errore ci scontra subitu in a funzione principale: vedemu u codice chì vene dopu à l'operatore ritornu - stu frammentu esegue a pulizia di memoria. Tuttavia, l'errore ùn pone micca una minaccia: tutta a memoria assignata serà sguassata da u sistema upirativu dopu chì u prugramma esce.
Nisuna manipulazione di errore
Array underrun hè pussibule. U valore di l'indice "n" puderia ghjunghje à -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);
}
....
}U snippet di codice in questu casu leghje da u schedariu in un buffer finu à a fine di u schedariu. Tuttavia, ùn ci hè micca una gestione di errore quì: se qualcosa va male, allora leghje riturnerà -1, è dopu l'array serà overrun Pruduzzioni.
Utilizendu EOF in u tipu di char
EOF ùn deve esse paragunatu cù un valore di u tipu "char". U "(c = fgetc(fp))" deve esse di u tipu "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++;
}
....
}Quì vedemu una manipulazione incorrecta di ghjunghje à a fine di u schedariu: if fgetc torna un caratteru chì u codice hè 0xFF, serà interpretatu cum'è a fine di u schedariu (EOF).
EOF hè una constante, generalmente definita cum'è -1. Per esempiu, in a codifica CP1251, l'ultima lettera di l'alfabetu russu hà u codice 0xFF, chì currisponde à u numeru -1 se parlemu di una variabile cum'è longer. Ci hè chì u simbulu 0xFF, cum'è EOF (-1) hè interpretatu cum'è a fine di u schedariu. Per evitari tali errori, u risultatu di a funzione hè fgetc deve esse guardatu in una variabile cum'è tram.
Typos
Frammentu 1
L'espressione 'write_time' hè sempre falsa. discu.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; // <=
....
}Forsi l'autore di stu codice hà sbagliatu || и && in cundizione. Cunsideremu l'opzioni pussibuli per i valori scrive_tempu и cambià_ora:
- E duie variàbili sò uguali à 0 : in questu casu, finiremu in un ramu altru: variàbbili mod_time serà sempre 0 indipendentemente da a cundizione successiva.
- Una di e variàbili hè 0: mod_time serà uguali à 0 (furnì chì l'altra variable hà un valore micca negativu), perchè MIN sceglierà u più chjucu di e duie opzioni.
- E duie variàbili ùn sò micca uguali à 0: sceglite u valore minimu.
Quandu si rimpiazza a cundizione cù write_time && change_time u cumpurtamentu parerà currettu:
- Una o e duie variàbili ùn sò micca uguali à 0: sceglite un valore micca zero.
- E duie variàbili ùn sò micca uguali à 0: sceglite u valore minimu.
Frammentu 2
L'espressione hè sempre vera. Probabilmente l'operatore "&&" deve esse usatu quì. discu.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;
....
}Pare chì l'operatori sò ancu mischiati quì || и &&, o == и !=: Una variable ùn pò micca avè u valore 20 è 9 à u stessu tempu.
Copia di linea illimitata
Una chjama di a funzione "sprintf" porta à un overflow di u buffer "fullpath". 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);
....
}Quandu avete guardatu a funzione in tuttu, diventerà chjaru chì stu codice ùn causa micca prublemi. Tuttavia, ponu esse in u futuru: un cambiamentu trascuratu è averemu un overflow di buffer - sprint ùn hè micca limitatu da nunda, cusì quandu cuncatenate i percorsi pudemu andà oltre i cunfini di l'array. Hè cunsigliatu à nutà sta chjama snprintf (percorsu pienu, PATH_MAX, ....).
Cundizione redundante
Una parte di l'espressione cundizionale hè sempre vera : aghjunghje > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}pirmittennu aghjunghje > 0 ùn ci hè micca bisognu quì: a variàbile serà sempre più grande di cero, perchè leghje % 4 turnerà u restu di a divisione, ma ùn serà mai uguali à 4.
xrdp
- implementazione di un servitore RDP cù codice open source. U prugettu hè divisu in 2 parti:
- xrdp - implementazione di u protocolu. Distribuitu sottu a licenza Apache 2.0.
- xorgxrdp - Un set di driver Xorg per l'usu cù xrdp. Licenza - X11 (cum'è MIT, ma pruibisce l'usu in publicità)
U sviluppu di u prugettu hè basatu annantu à i risultati di rdesktop è FreeRDP. In principiu, per travaglià cù gràfiche, avete avutu aduprà un servitore VNC separatu, o un servitore X11 speciale cù supportu RDP - X11rdp, ma cù l'avventu di xorgxrdp, a necessità per elli sparì.
In questu articulu ùn copreremu micca xorgxrdp.
U prughjettu xrdp, cum'è u precedente, hè assai chjucu è cuntene circa 80 mila linee.
Più typos
U codice cuntene a cullizzioni di blocchi simili. Verificate l'articuli "r", "g", "r" in e linee 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++;
}
....
}
....
}Stu codice hè statu pigliatu da a libreria librfxcodec, chì implementa u codec jpeg2000 per RemoteFX. Quì, apparentemente, i canali di dati grafichi sò mischiati - invece di u culore "blu", "rossu" hè registratu. Stu errore hè probabilmente apparsu com'è u risultatu di copia-incolla.
U listessu prublema hè accadutu in una funzione simili rfx_encode_format_argb, chì l'analizzatore ci hà dettu ancu:
U codice cuntene a cullizzioni di blocchi simili. Verificate l'articuli "a", "r", "g", "r" in e linee 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++;
}Dichjarazione Array
L'array overrun hè pussibule. U valore di l'indice "i - 8" puderia ghjunghje à 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];
....
}
....
}A dichjarazione è a definizione di l'array in questi dui schedari sò incompatibili - a dimensione differisce da 1. In ogni casu, ùn sò micca sbagliati - a dimensione curretta hè specificata in u schedariu evdev-map.c, perchè ùn ci hè micca fora di limiti. Allora questu hè solu un bug chì pò esse facilmente riparatu.
Paragone sbagliatu
Una parte di l'espressione cundizionale hè sempre falsa: (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))
{
....
}
....
}A funzione leghje una variabile di tipu cortu senza firma in una variabile cum'è tram. A verificazione ùn hè micca necessariu quì perchè leghjemu una variabile senza firmata è assignendu u risultatu à una variabile più grande, perchè a variàbile ùn pò micca piglià un valore negativu.
Cuntrolli inutili
Una parte di l'espressione cundizionale hè sempre vera: (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;
}
....
}I cuntrolli di inuguaglianza ùn anu micca sensu quì postu chì avemu digià un paragone à u principiu. Hè prubabile chì questu hè un typo è u sviluppatore vulia aduprà l'operatore || per filtrà l'argumenti invalidi.
cunchiusioni
Duranti l'auditu, ùn sò stati identificati micca errori serii, ma sò stati trovati assai difetti. Tuttavia, sti disinni sò usati in parechji sistemi, ancu s'ellu hè chjucu in u scopu. Un picculu prughjettu ùn hà micca bisognu di parechji errori, perchè ùn deve micca ghjudicà u rendiment di l'analizzatore solu in picculi prughjetti. Pudete leghje più nantu à questu in l'articulu "".
Pudete scaricà una versione di prova di PVS-Studio da noi .
Se vulete sparte stu articulu cù un publicu anglofonu, utilizate u ligame di traduzzione: Sergey Larin.
Source: www.habr.com
