🥇Provjera rdesktop i xrdp pomoću analitičara PVS-Studio

Ovo je drugi pregled iz ciklusa članaka o testiranju otvorenih programa za rad s RDP protokolom. U ovom slučaju razmatramo klijenta rdesktop i poslužitelj xrdp.

Kao alat za otkrivanje grešaka korišten je PVS-Studio. To je statički analizator koda za jezike C, C++, C# i Java, dostupan na platformama Windows, Linux i macOS.

U članku su predstavljene samo one greške koje su mi se činile zanimljivima. Ipak, projekti su mali, stoga je i grešaka bilo malo :).

Napomena. Prethodni članak o testiranju projekta FreeRDP možete pronaći ovdje.

rdesktop

rdesktop — slobodna implementacija RDP klijenta za UNIX-based sustave. Također se može koristiti i pod Windowsom, ako se projekt kompajlira pod Cygwin. Licenciran je pod GPLv3.

Ovaj klijent je vrlo popularan — koristi se po defaultu u ReactOS-u, a također su dostupni i različiti grafički front-endovi. Ipak, prilično je star: prvi put je objavljen 4. travnja 2001. godine — u vrijeme pisanja ovog članka, njegova starost iznosi 17 godina.

Kao što sam već napomenuo, projekt je vrlo mali. Sadrži otprilike 30 tisuća linija koda, što je pomalo čudno s obzirom na njegovu starost. U usporedbi, FreeRDP sadrži 320 tisuća linija. Evo izvoda iz programa Cloc:

Nedostižan kod

V779 Unreachable code detected. It is possible that an error is present. 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);
}

Greška nas dočekuje odmah u funkciji main: vidimo kod koji dolazi nakon operatora return — ovaj fragment provodi čišćenje memorije. Ipak, greška ne predstavlja prijetnju: cijela alocirana memorija bit će očišćena od strane operativnog sustava po završetku rada programa.

Nedostatak obrade grešaka

V557 Array underrun is possible. The value of ‘n’ index could reach -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);
  }
  ....
}

Fragment koda u ovom slučaju čita iz datoteke u bafer sve dok datoteka ne završi. Međutim, ovdje nedostaje obrada grešaka: ako nešto pođe po zlu, read vraća -1, i tada će doći do izlaska izvan granica niza output.

Korištenje EOF u tipu char

V739 EOF should not be compared with a value of the ‘char’ type. The ‘(c = fgetc(fp))’ should be of the ‘int’ type. 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++;
  }
  ....
}

Ovdje vidimo nepravilnu obradu postizanja kraja datoteke: ako fgetc vrati znak čija je vrijednost 0xFF, bit će protumačen kao kraj datoteke (EOF).

EOF to je konstanta koja se obično definira kao -1. Na primjer, u kodiranju CP1251, posljednje slovo ruskog alfabeta ima kod 0xFF, što odgovara broju -1, kada govorimo o varijabli tipa char. Ispada da se znak 0xFF, kao i EOF (-1) doživljava kao kraj datoteke. Da bismo izbjegli ovakve greške, rezultat rada funkcije fgetc treba pohraniti u varijablu tipa int.

Typo

Fragment 1

V547 Izraz 'write_time' je uvijek lažan. 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; // <=
  ....
}

Možda je autor ovog koda pomiješao || i && u uvjetu. Razmotrimo moguće vrijednosti write_time i change_time:

Obje varijable su jednake 0: u tom slučaju ćemo dospjeti u granu else: varijabla mod_time uvijek će biti jednaka 0, bez obzira na sljedeći uvjet.
Jedna od varijabli je jednaka 0: mod_time bit će jednako 0 (pod uvjetom da druga varijabla ima nenegativnu vrijednost), jer MIN izabire manju od dviju opcija.
Obje varijable nisu jednake 0: biramo minimalnu vrijednost.

Ako promijenimo uvjet u write_time && change_time ponašanje će izgledati ispravno:

Jedna ili obje varijable nisu jednake 0: biramo nenultu vrijednost.
Obje varijable nisu jednake 0: biramo minimalnu vrijednost.

Fragment 2

V547 Izraz je uvijek istinit. Vjerojatno bi ovdje trebao biti korišten operator '&&'. 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;
  ....
}

Ovdje su vjerojatno također pomiješani operatori || i &&, ili == i !=: varijabla ne može istovremeno imati vrijednost 20 i 9.

Neograničeno kopiranje niza

V512 Poziv funkcije 'sprintf' će dovesti do preljevanja bafera 'fullpath'. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Dobij informacije za unos direktorija */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Proučavanjem funkcije u cijelosti postaje jasno da ovaj kod ne izaziva probleme. Međutim, one se mogu pojaviti u budućnosti: jedna neoprezna promjena i dobit ćemo preljevanje bafera — sprintf nije ničim ograničen, tako da prilikom spajanja puteva možemo izaći izvan granica niza. Preporučuje se da se ovaj poziv zamijeni s snprintf(fullpath, PATH_MAX, ….).

Višak uvjeta

V560 Dio uvjetnog izraza je uvijek istinit: dodaj > 0. scard.c 507

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

Provjera add > 0 ovdje nije potrebno: varijabla će uvijek biti veća od nule, jer read % 4 vratit će ostatak od dijeljenja, a nikada neće biti jednak 4.

xrdp

xrdp — implementacija RDP poslužitelja s otvorenim izvorom. Projekt je podijeljen na 2 dijela:

xrdp — implementacija protokola. Distribuira se pod Apache 2.0 licencom.
xorgxrdp — skup Xorg drajvera za korištenje s xrdp-om. Licenca — X11 (poput MIT, ali zabranjuje korištenje u oglašavanju)

Razvoj projekta temelji se na rezultatima rdesktop i FreeRDP. Prvotno je za rad s grafikom bilo potrebno koristiti odvojeni VNC poslužitelj, ili posebni X11 poslužitelj s podrškom za RDP — X11rdp, no s pojavom xorgxrdp potreba za njima otpala je.

U ovom članku nećemo raspravljati o xorgxrdp-u.

Projekt xrdp, kao i prethodni, je vrlo mali i sadrži otprilike 80 tisuća redaka.

Još tipfeler

V525 Kod sadrži kolekciju sličnih blokova. Provjerite stavke ‘r’, ‘g’, ‘r’ u redovima 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++;
      }
      ....
  }
  ....
}

Ovaj kod je preuzet iz biblioteke librfxcodec, koja implementira jpeg2000 kodek za rad s RemoteFX-om. Ovdje su očito pomiješani kanali grafičkih podataka — umjesto plave boje zapisuje se crvena. Takva greška, vjerojatno, nastala je uslijed copy-paste.

Ova ista problema zahvatila je i sličnu funkciju rfx_encode_format_argb, o čemu nam također izvještava analizator:

V525 Kod sadrži kolekciju sličnih blokova. Provjerite stavke ‘a’, ‘r’, ‘g’, ‘r’ u redovima 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++;
}

Deklaracija niza

V557 Moguće je pretjerivanje s nizom. Vrijednost indeksa ‘i — 8’ mogla bi dostići 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];
    ....
  }
  ....
}

Deklaracija i definicija niza u ova dva datoteke su nekompatibilne — veličina se razlikuje za 1. No, ne nastaju nikakve greške — u datoteci evdev-map.c je navedena ispravna veličina, pa izlazak iz granica nije moguć. Tako da je to samo propust koji se lako može ispraviti.

Neispravno uspoređivanje

V560 Dio uvjetnog izraza je uvijek lažan: (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))
  {
    ....
  }
  ....
}

U funkciji se čita varijabla tipa unsigned short u varijablu tipa intOvdje provjera nije potrebna, budući da čitamo varijablu bez znaka i dodjeljujemo rezultat varijabli većeg tipa, pa varijabla ne može primiti negativnu vrijednost.

Nepotrebne provjere

V560 Dio uvjetnog izraza je uvijek istinit: (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: greška");
      return 1;
  }
  ....
}

Provjere nejednakosti ovdje nemaju smisla jer već imamo usporedbu na početku. Vjerojatno je da je ovo bila tipfeler, a programer je želio koristiti operator || za filtriranje neispravnih argumenata.

Zaključak

Tijekom provjere nisu otkrivene ozbiljne greške, ali su pronađene brojne manjkavosti. Ipak, ovi projekti se koriste u mnogim sustavima, iako su mali po obujmu. U malom projektu ne mora nužno biti puno grešaka, stoga ne treba suditi o radu analizatora samo na malim projektima. Više o tome možete pročitati u članku "Osjećaji koje su brojke potvrdile«.

Možete preuzeti probnu verziju PVS-Studio kod nas na web stranici.

Ako želite podijeliti ovaj članak s anglofonim publikom, molim vas koristite vezu na prijevod: Sergey Larin. Provjera rdesktop i xrdp s PVS-Studio

Izvor: habr.com