Rdesktopi ja xrdp testimine PVS-Studio analüsaatori abil

See on teine ülevaade RDP-protokolli avatud rakenduste kontrollimise artiklite sarjast. Selles käsitleme rdesktopi klienti ja xrdp serverit.

Vigade tuvastamiseks kasutati tööriista PVS-Studio. See on staatiline analüsaator keelte C, C++, C# ja Java jaoks, mis on saadaval Windowsi, Linuxi ja macOSi platvormidel.

Artiklis on esitatud vaid need vead, mis tundusid mulle huvitavad. Sellegipoolest on projektid pisikesed, seega oli vigu ka vähe :).

Märkus. Eelmist artiklit FreeRDP projekti kohta saab leida siin.

rdesktop

rdesktop — avatud RDP kliendi rakendus UNIX-põhiste süsteemide jaoks. Seda saab kasutada ka Windowsis, kui koguda projekt Cygwini all. litsentseeritud GPLv3 all.

See klient on väga populaarne — see on vaikimisi kasutusel ReactOS-is, samuti on sellele saadaval kolmandate osapoolte graafilised front-end'id. Sellegipoolest on see üsna vana: esimene versioon ilmus 4. aprillil 2001 — artikli kirjutamise ajal on selle vanus 17 aastat.

Kuidas ma juba varasemalt märkisin, on projekt täiesti pisike. See sisaldab umbes 30 tuhat koodireaga, mis on natuke vähe, arvestades selle vanust. Võrdluseks, FreeRDP sisaldab 320 tuhat koodireaga. Siin on Cloci programmi väljund:

Üksikasjalik kood

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);
}

Viga ootab meid kohe funktsioonis main: näeme koodi, mis järgneb operaatorile return — see fragment teostab mälu puhastamist. Sellegipoolest ei kujuta viga ohtu: kogu jaotatud mälu puhastab operatsioonisüsteem pärast programmi lõpetamist.

Vigade töötlemise puudumine

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);
  }
  ....
}

Antud koodi fragment loeb faili puhverisse, kuni fail on lõppenud. Siiski puudub siin vigade töötlemine: kui midagi läheb valesti, siis lugema tagastab -1, ja siis toimub massiivi piirist väljumine output.

EOF kasutamine char tüübis

V739 EOF ei tohiks võrrelda ‘char’ tüübi väärtusega. ‘(c = fgetc(fp))’ peaks olema ‘int’ tüüpi. 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++;
  }
  ....
}

Siin näeme vale faili lõppu käitlemist: kui fgetc tagastab sümboli, mille kood on 0xFF, siis tõlgendatakse seda kui faili lõppu (EOF).

EOF see on konstants, mis on tavaliselt määratletud kui -1. Näiteks CP1251 kodeeringus on venekeelse tähestiku viimane täht koodiga 0xFF, mis vastab numbrile -1, kui räägime muutuja tüübist char. Tulemuseks on, et sümbol 0xFF, nagu ka EOF (-1) tõlgendatakse faili lõppena. Selliste vigade vältimiseks tuleks funktsiooni fgetc tulemus salvestada muutuja tüübiga int.

Trükivead

Fragment 1

V547 Väljend ‘write_time’ on alati vale. 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; 
  ....
}

Võib-olla autor segas siin || ja && tingimustes. Vaatame võimalikke väärtuste variante write_time ja change_time:

• Mõlemad muutujad on võrdsed 0: sel juhul satume harusse muul juhul: muutuja mod_time on alati 0, olenemata järgnevast tingimusest.
• Üks muutuja on 0: mod_time on 0 (eeldades, et teine muutuja on mitte-negatiivne), sest MIN valib kahest valikust väiksema.
• Mõlemad muutujad ei ole võrdsed 0: valime minimaalne väärtuse.

Kui tingimuse asendame write_time && change_time käitumine tundub korrektne:

• Üks või mõlemad muutujad ei ole 0: valime mitte-null väärtuse.
• Mõlemad muutujad ei ole võrdsed 0: valime minimaalne väärtuse.

Fragment 2

V547 Väljend on alati tõene. Tõenäoliselt tuleks kasutada ‘&&’ operaatorit. 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;
  ....
}

Ilmselt on siin samuti operaatorid segamini aetud || ja &&, või == ja !=: muutuja ei saa korraga võtta väärtusi 20 ja 9.

Piiramatu stringi koopia

V512 ‘sprintf’ funktsiooni kutsumine viib ‘fullpath’ puhveri ülevooluni. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Saame teavet katalooge sisenemise kohta */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Funktsiooni täielikku kaalumist vaadates saame aru, et see kood ei põhjusta probleeme. Kuid tulevikus võivad need tekkida: üks tähelepanematult tehtud muudatus ja saame puhveri ülevoolu — sprintf ei ole piiratud, seega kadreerimise ajal teede liitmine võib viia massiivi piiride ületamiseni. Soovitame selle kutse tähele panna snprintf(fullpath, PATH_MAX, ….).

Liigne tingimus

V560 Osaline tingimus, mis on alati tõene: lisa > 0. scard.c 507

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

Kontrollimine add > 0 siin ei ole mõtet: muutuja on alati suurem kui null, sest read % 4 tagastab jagamise jäägi, mis ei saa kunagi olla 4.

xrdp

xrdp — avatud lähtekoodiga RDP serveri teostus. Projekt on jagatud kaheks osaks:

• xrdp — protokolli teostus. Levitatakse Apache 2.0 litsentsi alusel.
• xorgxrdp — Xorgi draiverite komplekt, mida saab kasutada koos xrdp-ga. Litsents — X11 (nagu MIT, kuid reklaami kasutamine on keelatud)

Projekti arendus põhineb rdesktopi ja FreeRDP tulemuste põhjal. Alguses tuli graafika jaoks kasutada eraldi VNC serverit või spetsiaalset X11 serverit RDP toe jaoks — X11rdp, kuid xorgxrdp tekkimisega ei olnud seda enam vajalik.

Selles artiklis me xorgxrdp-st ei räägi.

Projekt xrdp, nagu eelmine, on üsna väike ja sisaldab umbes 80 tuhat rida.

Veel trükivigu

V525 Kood sisaldab sarnaste plokkide kogumit. Kontrollige elemente 'r', 'g', 'r' ridadel 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++;
      }
      ....
  }
  ....
}

See kood on võetud librfxcodec teeki, mis implementeerib jpeg2000 koodeki RemoteFX-i jaoks. Siin paistavad olevat segi aetud graafikandmete kanalid — sinise värvi asemel kirjutatakse punane. Selline viga tekkis tõenäoliselt copy-paste'i tulemusena.

Sama probleem esines ka sarnases funktsioonis rfx_encode_format_argb, millest teatas meile ka analüsaator:

V525 Kood sisaldab sarnaste plokkide kogumit. Kontrollige elemente 'a', 'r', 'g', 'r' ridadel 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++;
}

Massiivi deklareerimine

V557 Massiivi ületamine on võimalik. 'i - 8' indeksi väärtus võib ulatuda 129-ni. 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];
    ....
  }
  ....
}

Massiivi deklareerimine ja määramine nendes kahes failis ei ole ühilduvad — suurus erineb 1 võrra. Kuid vigu ei esine — failis evdev-map.c on näidatud õige suurus, nii et ületamisi ei toimu. Seega on see lihtsalt puudujääk, mis on kergesti parandatav.

Vale võrdlemine

V560 Osa tingimuslikust väljendusest on alati vale: (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))
  {
    ....
  }
  ....
}

Funktsioonis toimub muutuja tüübi lugemine unsigned short tüüpi muutuja intKontrolli siin pole vajalik, kuna me loeme alla 0 tüüpi muutuja ja määrame tulemuse suurema suurusega muutujale, seetõttu ei saa muutuja võtta negatiivset väärtust.

Üksusel ei ole ühtki kontrolli

V560 Osaliselt tingimuslikus väljendis on alati tõene: (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: viga");
      return 1;
  }
  ....
}

Kontrollid mittevõrdsuse üle siin ei ole mõtet, kuna meil on juba võrreldav alguses. On üsna tõenäoline, et see on trükiviga ja arendaja tahtis kasutada operaatorit || valeargumentide filtreerimiseks.

Kokkuvõte

Kontrollimisel ei tuvastatud tõsiseid vigu, kuid leiti palju puudusi. Sellegipoolest kasutatakse neid projekte paljudes süsteemides, kuigi need on oma mahult väikesed. Väikeses projektis ei pea tingimata olema palju vigu, seetõttu ei saa analüsaatori tööd hinnata ainult väikeste projektide põhjal. Selle kohta saate rohkem lugeda artiklis "Tunded, mis numbritega kinnitust leidis«.

Saate alla laadida PVS-Studio prooviversiooni meie juures veebisaidil.

Kui soovite seda artiklit jagada ingliskeelsele publikule, siis palun kasutage tõlke linki: Sergey Larin. rdesktopi ja xrdp kontrollimine PVS-Studio abil

Allikas: habr.com