Verificarea rdesktop și xrdp cu ajutorul analizadorului PVS-Studio

Aceasta este a doua recenzie dintr-o serie de articole despre verificarea programelor open source pentru protocolul RDP. În aceasta vom analiza clientul rdesktop și serverul xrdp.

Ca instrument pentru identificarea erorilor a fost utilizat PVS-Studio. Este un analizator static de cod pentru limbajele C, C++, C# și Java, disponibil pe platformele Windows, Linux și macOS.

În articol sunt prezentate doar acele erori care mi s-au părut interesante. Cu toate acestea, proiectele sunt mici, așa că și erorile au fost puține :).

Notă. Articolul anterior despre verificarea proiectului FreeRDP poate fi găsit aici.

rdesktop

rdesktop — o implementare liberă a clientului RDP pentru sistemele UNIX-based. Poate fi folosit și pe Windows, dacă proiectul este compilat sub Cygwin. Licențiat sub GPLv3.

Acest client este foarte popular — este folosit implicit în ReactOS, de asemenea, pot fi găsite interfețe grafice front-end externe pentru el. Totuși, este destul de vechi: prima lansare a avut loc pe 4 aprilie 2001 — la momentul scrierii acestui articol, are 17 ani.

Așa cum am menționat anterior, proiectul este foarte mic. Acesta conține aproximativ 30 de mii de linii de cod, ceea ce este puțin ciudat, având în vedere vârsta sa. Spre comparație, FreeRDP conține 320 de mii de linii. Iată rezultatul programului Cloc:

Cod inaccesibil

V779 Cod inaccesibil 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âmpină imediat în funcția main: vedem codul care urmează operatorului return — acest fragment efectuează curățarea memoriei. Totuși, eroarea nu prezintă o amenințare: toată memoria alocată va fi curățată de sistemul de operare după încheierea programului.

Lipsa tratării erorilor

V557 Este posibil un underrun al array-ului. 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 dintr-un fișier în buffer până când fișierul se termină. Cu toate acestea, lipsesc tratările de erori: dacă ceva nu merge bine, atunci read va returna -1, iar atunci va avea loc o ieșire din limitele array-ului output.

Utilizarea EOF în tipul char

V739 EOF nu ar trebui să fie 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 o gestionare incorectă a atingerii sfârșitului de fișier: dacă fgetc returnează un caracter a cărui cod este 0xFF, este perceput ca sfârșit de fișier (EOF).

EOF aceasta este o constantă, definită de obicei ca -1. De exemplu, în codificarea CP1251, ultima literă a alfabetului rusesc are codul 0xFF, care corespunde numărului -1 când ne referim la o variabilă de tip char. Se pare că simbolul 0xFF, la fel ca și EOF (-1) este perceput ca sfârșit de fișier. Pentru a evita astfel de erori, rezultatul funcției fgetc ar trebui să fie stocat într-o variabilă de tip int.

Greșeli de tipar

Fragment 1

V547 Expresia 'write_time' este întotdeauna falsă. 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; // <=
  ....
}

Probabil, autorul acestui cod a confundat || și && în condiție. Să luăm în considerare posibilele valori write_time și change_time:

• Ambele variabile sunt egale cu 0: în acest caz vom ajunge în ramura altfel: variabila mod_time va fi întotdeauna 0 indiferent de condiția ulterioară.
• Una dintre variabile este 0: mod_time va fi 0 (cu condiția ca cealaltă variabilă să aibă o valoare non-negativă), deoarece MIN va alege cea mai mică dintre cele două opțiuni.
• Ambele variabile nu sunt egale cu 0: alegem valoarea minimă.

Dacă schimbăm condiția în write_time && change_time comportamentul va părea corect:

• Una sau ambele variabile nu sunt egale cu 0: alegem o valoare diferită de 0.
• Ambele variabile nu sunt egale cu 0: alegem valoarea minimă.

Fragment 2

V547 Expresia este întotdeauna adevărată. Probabil operatorul ‘&&’ ar trebui utilizat aici. 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;
  ....
}

Se pare că aici, de asemenea, operatorii au fost confundați || și &&, sau == și !=: variabila nu poate primi simultan valoarea 20 și 9.

Copierea nelimitată a șirului

V512 O apelare a funcției ‘sprintf’ va conduce la overflow-ul buffer-ului ‘fullpath’. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Obține informații pentru intrarea de director */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Analizând funcția complet, devine clar că acest cod nu creează probleme. Totuși, ele pot apărea în viitor: o modificare neglijentă și putem avea overflow de buffer — sprintf nu este limitat, astfel că, în timpul concatenării căilor, putem depăși limitele array-ului. Este recomandat să observăm această apelare pe snprintf(fullpath, PATH_MAX, ….).

Condiție redundantă

V560 O parte a expresiei condiționale este întotdeauna adevărată: adaugă > 0. scard.c 507

static void
inRepos(STREAM in, unsigned int read)
{
  SERVER_DWORD add = 4 - read % 4;
  if (add < 4 && add > 0)
  {
    ....
  }
}

Verificare add > 0 aici nu are sens: variabila va fi întotdeauna mai mare decât zero, deoarece read % 4 va returna restul împărțirii, care nu va fi niciodată 4.

xrdp

xrdp — o implementare a serverului RDP cu sursă deschisă. Proiectul este împărțit în 2 părți:

• xrdp — o implementare a protocolului. Este distribuit sub licența Apache 2.0.
• xorgxrdp — un set de drivere Xorg pentru utilizare cu xrdp. Licența — X11 (similar cu MIT, dar interzice utilizarea în publicitate)

Dezvoltarea proiectului se bazează pe rezultatele rdesktop și FreeRDP. Inițial, pentru a lucra cu grafică, era necesar să se folosească un server VNC separat sau un server special X11 cu suport pentru RDP — X11rdp, dar odată cu apariția xorgxrdp, această nevoie a dispărut.

În acest articol nu vom aborda xorgxrdp.

Proiectul xrdp, ca și precedentul, este destul de mic și conține aproximativ 80 de mii de linii.

Încă erori de tipar

V525 Codul conține o colectare de blocuri similare. Verificați elementele 'r', 'g', 'r' în liniile 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ă codec jpeg2000 pentru RemoteFX. Aici, probabil, canalele de date grafice au fost amestecate — în loc de culoarea „albastră” se scrie „roșie”. Această eroare a apărut, cel mai probabil, în urma copy-paste-ului.

Aceeași problemă a apărut și în funcția similară rfx_encode_format_argb, despre care ne-a informat și analizatorul:

V525 Codul conține o colectare de blocuri similare. Verificați elementele 'a', 'r', 'g', 'r' în liniile 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ția array-ului

V557 Este posibilă depășirea array-ului. Valoarea indexului ‘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 array-ului în aceste două fișiere nu sunt compatibile — dimensiunea diferă cu 1. Cu toate acestea, nu apar erori — în fișierul evdev-map.c este specificată dimensiunea corectă, astfel încât nu există ieșire din limite. Deci, acesta este doar un neajuns, care poate fi corectat ușor.

Compararea incorectă

V560 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))
  {
    ....
  }
  ....
}

În funcție are loc citirea unei variabile de tip unsigned short într-o variabilă de tip int. Verificarea aici nu este necesară, deoarece citim o variabilă de tip nesemnat și atribuim rezultatul unei variabile de dimensiune mai mare, deci variabila nu poate lua o valoare negativă.

Verificări inutile

V560 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: eroare");
      return 1;
  }
  ....
}

Verificările de inegalitate aici nu au sens, deoarece avem deja o comparație la început. Este foarte probabil că aceasta este o greșeală de tipar și dezvoltatorul a dorit să folosească operatorul || pentru a filtra argumentele incorecte.

Concluzie

În timpul verificării nu au fost descoperite erori grave, dar au fost găsite multe neajunsuri. Cu toate acestea, aceste proiecte sunt utilizate în multe sisteme, deși sunt mici ca volum. Într-un proiect mic nu este obligatoriu să existe multe erori, deci nu ar trebui să judecăm munca analistului doar pe baza proiectelor mici. Mai multe detalii pot fi citite în articolul „Senzațiile care au fost confirmate de cifre«.

Puteți descărca versiunea de probă a PVS-Studio de la noi pe site.

Dacă doriți să împărtășiți acest articol cu o audiență anglofonă, vă rog să folosiți linkul pentru traducere: Sergey Larin. Verificând rdesktop și xrdp cu PVS-Studio

Sursa: habr.com