Aceasta este a doua recenzie dintr-o serie de articole despre testarea programelor open source pentru lucrul cu protocolul RDP. În el ne vom uita la clientul rdesktop și la serverul xrdp.
Folosit ca instrument de identificare a erorilor . Este un analizor de cod static pentru limbajele C, C++, C# și Java, disponibil pe platformele Windows, Linux și macOS.
Articolul prezintă doar acele erori care mi s-au părut interesante. Totuși, proiectele sunt mici, așa că au fost puține greșeli :).
Nota. Un articol anterior despre verificarea proiectului FreeRDP poate fi găsit .
rdesktop
— o implementare gratuită a unui client RDP pentru sisteme bazate pe UNIX. Poate fi folosit și sub Windows dacă construiți proiectul sub Cygwin. Licențiat conform GPLv3.
Acest client este foarte popular - este folosit implicit în ReactOS și puteți găsi, de asemenea, front-end-uri grafice terță parte pentru el. Cu toate acestea, este destul de bătrân: prima sa lansare a avut loc pe 4 aprilie 2001 - la momentul scrierii, are 17 ani.
După cum am menționat mai devreme, proiectul este foarte mic. Conține aproximativ 30 de mii de linii de cod, ceea ce este puțin ciudat având în vedere vechimea sa. Pentru comparație, FreeRDP conține 320 de mii de linii. Iată rezultatul programului Cloc:
Cod inaccesibil
Cod indisponibil detectat. Este posibil să existe o eroare. 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);
}Eroarea ne întâlnește imediat în funcție principal: vedem codul venind dupa operator reveni — acest fragment efectuează curățarea memoriei. Cu toate acestea, eroarea nu reprezintă o amenințare: toată memoria alocată va fi ștearsă de sistemul de operare după ieșirea programului.
Fără tratare a erorilor
Subîncărcarea matricei este posibilă. Valoarea indicelui „n” ar putea ajunge la -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);
}
....
}Fragmentul de cod în acest caz citește din fișier într-un buffer până când fișierul se termină. Cu toate acestea, nu există nicio gestionare a erorilor aici: dacă ceva nu merge bine, atunci citit va returna -1, iar apoi matricea va fi depășită producție.
Folosind EOF în tipul char
EOF nu trebuie comparat cu o valoare de tip „char”. „(c = fgetc(fp))” ar trebui să fie de tip „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++;
}
....
}Aici vedem manipularea incorectă a atingerii sfârșitului fișierului: dacă fgetc returnează un caracter al cărui cod este 0xFF, acesta va fi interpretat ca sfârșitul fișierului (EOF).
EOF este o constantă, de obicei definită ca -1. De exemplu, în codificarea CP1251, ultima literă a alfabetului rus are codul 0xFF, care corespunde numărului -1 dacă vorbim despre o variabilă precum car de război. Se pare că simbolul 0xFF, cum ar fi EOF (-1) este interpretat ca sfârșitul fișierului. Pentru a evita astfel de erori, rezultatul funcției este fgetc ar trebui să fie stocat într-o variabilă ca int.
Tipografii
Fragmentul 1
Expresia „write_time” este întotdeauna falsă. disc.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; // <=
....
}Poate că autorul acestui cod a greșit || и && in conditie. Să luăm în considerare posibilele opțiuni pentru valori scrie_timp и schimbă timpul:
- Ambele variabile sunt egale cu 0: în acest caz vom ajunge într-o ramură altfel: variabil mod_time va fi întotdeauna 0 indiferent de condiția ulterioară.
- Una dintre variabile este 0: mod_time va fi egal cu 0 (cu condiția ca cealaltă variabilă să aibă o valoare nenegativă), deoarece MIN va alege cea mai mică dintre cele două opțiuni.
- Ambele variabile nu sunt egale cu 0: alegeți valoarea minimă.
La înlocuirea stării cu write_time && change_time comportamentul va arata corect:
- Una sau ambele variabile nu sunt egale cu 0: alegeți o valoare diferită de zero.
- Ambele variabile nu sunt egale cu 0: alegeți valoarea minimă.
Fragmentul 2
Expresia este întotdeauna adevărată. Probabil că aici ar trebui folosit operatorul „&&”. disc.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;
....
}Se pare că și operatorii sunt amestecați aici || и &&Sau == и !=: O variabilă nu poate avea valoarea 20 și 9 în același timp.
Copiere nelimitată de linii
Un apel al funcției „sprintf” va duce la depășirea bufferului „fullpath”. disc.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);
....
}Când vă uitați la funcția în întregime, va deveni clar că acest cod nu provoacă probleme. Cu toate acestea, ele pot apărea în viitor: o schimbare neglijentă și vom obține o depășire a tamponului - sprint nu este limitat de nimic, așa că atunci când concatenăm căile putem trece dincolo de granițele matricei. Este recomandat să observați acest apel snprintf(cale completă, PATH_MAX, ….).
Stare redundantă
O parte a expresiei condiționale este întotdeauna adevărată: adăugați > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка adăugați > 0 nu este nevoie aici: variabila va fi întotdeauna mai mare decât zero, deoarece citeste % 4 va returna restul diviziei, dar nu va fi niciodată egal cu 4.
xrdp
— implementarea unui server RDP cu cod sursă deschisă. Proiectul este împărțit în 2 părți:
- xrdp - implementarea protocolului. Distribuit sub licența Apache 2.0.
- xorgxrdp - Un set de drivere Xorg pentru utilizare cu xrdp. Licență - X11 (ca MIT, dar interzice utilizarea în publicitate)
Dezvoltarea proiectului se bazează pe rezultatele rdesktop și FreeRDP. Inițial, pentru a lucra cu grafică, trebuia să utilizați un server VNC separat, sau un server special X11 cu suport RDP - X11rdp, dar odată cu apariția xorgxrdp, nevoia acestora a dispărut.
În acest articol nu vom acoperi xorgxrdp.
Proiectul xrdp, ca și cel anterior, este foarte mic și conține aproximativ 80 de mii de linii.
Mai multe greșeli de scriere
Codul conține o colecție de blocuri similare. Verificați elementele „r”, „g”, „r” din rândurile 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++;
}
....
}
....
}Acest cod a fost preluat din biblioteca librfxcodec, care implementează codecul jpeg2000 pentru RemoteFX. Aici, aparent, canalele de date grafice sunt amestecate - în loc de culoarea „albastru”, este înregistrat „roșu”. Această eroare a apărut cel mai probabil ca rezultat al copy-paste.
Aceeași problemă a apărut într-o funcție similară rfx_encode_format_argb, despre care analizatorul ne-a mai spus:
Codul conține o colecție de blocuri similare. Verificați elementele „a”, „r”, „g”, „r” din rândurile 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++;
}Declarație de matrice
Depășirea matricei este posibilă. Valoarea indicelui „i — 8” ar putea ajunge la 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];
....
}
....
}Declarația și definiția matricei din aceste două fișiere sunt incompatibile - dimensiunea diferă cu 1. Cu toate acestea, nu apar erori - dimensiunea corectă este specificată în fișierul evdev-map.c, deci nu există depășiri. Deci, acesta este doar o eroare care poate fi remediată cu ușurință.
Comparație incorectă
O parte a expresiei condiționale este întotdeauna falsă: (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))
{
....
}
....
}Funcția citește o variabilă de tip scurt nesemnat într-o variabilă ca int. Verificarea nu este necesară aici deoarece citim o variabilă fără semn și atribuim rezultatul unei variabile mai mari, astfel încât variabila nu poate lua o valoare negativă.
Verificări inutile
O parte a expresiei condiționale este întotdeauna adevărată: (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;
}
....
}Verificările inegalității nu au sens aici, deoarece avem deja o comparație la început. Este probabil ca aceasta să fie o greșeală de tipar și dezvoltatorul a vrut să folosească operatorul || pentru a filtra argumentele nevalide.
Concluzie
Pe parcursul auditului nu au fost identificate erori grave, dar au fost constatate multe neajunsuri. Cu toate acestea, aceste modele sunt utilizate în multe sisteme, deși de dimensiuni reduse. Un proiect mic nu are neapărat multe erori, așa că nu ar trebui să judeci performanța analizorului doar la proiecte mici. Puteți citi mai multe despre asta în articolul „“.
Puteți descărca o versiune de încercare a PVS-Studio de la noi .
Dacă doriți să împărtășiți acest articol unui public vorbitor de engleză, vă rugăm să folosiți linkul de traducere: Sergey Larin.
Sursa: www.habr.com