rdesktop ja xrdp kontrollimine PVS-Studio analüsaatori abil

See on teine ülevaade artiklite sarjast avatud programmide kontrollimise kohta RDP-protokolliga. Selles vaatleme rdesktop kliendi ja xrdp serveri funktsioone.

Vigade avastamiseks kasutati tööriista PVS-Studio. See on staatiline koodianalüsaator C, C++, C# ja Java keelte jaoks, mis on saadaval Windowsi, Linuxi ja macOS-i platvormidel.

Artiklis esitatakse vaid need vead, mis mulle huvitavad tundusid. Tõele au andes on projektid siiski väikesed, seega on ka vigu olnud vähe :)

Märkus. Eelmist artiklit FreeRDP projekti kontrollimisest saab leida siit.

rdesktop

rdesktop — tasuta RDP kliendi rakendus UNIX-põhiste süsteemide jaoks. Seda saab kasutada ka Windowsis, kui projekti Cygwini all koostada. Litsentseeritud GPLv3 alusel.

See klient on saavutanud suure populaarsuse — seda kasutatakse vaikimisi ReactOSis ning sellele on saadaval ka kolmandate osapoolte graafilised front-end’id. Siiski on see üsna vana: esimene väljalase toimus 4. aprillil 2001 — artikli kirjutamise hetkel on see 17 aastat vana.

Nagu juba mainitud, on projekt väga väike. See sisaldab umbes 30 tuhat rida koodi, mis on veidi kummaline, arvestades selle vanust. Võrdluseks: FreeRDP sisaldab endas 320 tuhat rida. Siin on Cloci programmi väljund:

Käimata kood

V779 Tuvastatud on ulatuseta kood. Võimalik, et viga on olemas. 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 ilmneb kohe funktsioonis main: näeme koodi, mis järgneb operaatorile return — see fragment teostab mälu puhastust. Siiski ei kujuta viga ohtu: kogu eraldatud mälu puhastab operatsioonisüsteem pärast programmi lõpetamist.

Vigade töötlemise puudumine

V557 Massiivi alarünnak on võimalik. Väärtus 'n' indeks võib ulatuda -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);
  }
  ....
}

Selles koodifragmendis loetakse faili sisu puhvris seni, kuni fail on otsas. Siiski puudub siin vigade töötlemine: kui midagi läheb valesti, siis read tagastab -1, ja siis toimub massiivi piiride ületamine output.

EOF kasutamine char tüübina

V739 EOF ei tohiks võrrelda 'char' tüübi väärtusega. '(c = fgetc(fp))' peaks olema 'int' tüübist. 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 töötlemise lõpuni fail: kui fgetc tagastab sümboli, mille kood on 0xFF, siis see tõlgendatakse lõpuks failina (EOF).

EOF see on konstant, mis on tavaliselt määratletud kui -1. Näiteks koodimisvormis CP1251 on vene tähestiku viimane täht koodiga 0xFF, mis vastab numbrile -1, kui me räägime tüüpi char. Tulemuseks on, et sümbol 0xFF, nagu ka EOF (-1) tõlgendatakse kui lõpp faili. Selliste vigade vältimiseks tuleks funktsiooni tulemused hoida tüüpi fgetc muutujas 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 segas selle koodi autor || ja && tingimuses. Vaatame võimalikke väärtusi write_time ja change_time:

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

Kui tingimus asendada write_time && change_time käitumine näeb välja korrektne:

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

Fragment 2

V547 Avaldis on alati tõsi. Tõenäoliselt tuleks siin 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;
  ....
}

Tundub, et siin on ka operaatorid segamini aetud || ja &&, või == ja !=: muutuja ei saa samaaegselt võtta väärtust 20 ja 9.

Piiramatu stringi kopeerimine

V512 Funktsiooni ‘sprintf’ kutsumine viib ‘fullpath’ puhvri üleujutamiseni. disk.c 1257

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

Funktsiooni täielikku läbivaatamist arvesse võttes on selge, et see kood ei tekita probleeme. Kuid tulevikus võivad need tekkida: üks ettevaatamatu muudatus ja me saame puhvri üleujutamise — sprintf pole pole on piiratud, seega võivad teede kokkupanemisel meie massiivi piiridest välja minna. Soovitame seda kutset jälgida snprintf(fullpath, PATH_MAX, ….).

Üksikasjalik tingimus

V560 Osa tingimuslikust väljendist 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 < 4 && add > 0)
  {
    ....
  }
}

Kontrollimine add > 0 siin pole mõtet: muutuja on alati suurem kui null, kuna read % 4 tagastab jäägi jagamisest, kuid see ei ole kunagi võrdne 4-ga.

xrdp

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

• xrdp — protokolli rakendus. Jagatakse Apache 2.0 litsentsi alusel.
• xorgxrdp — Xorg draiverite komplekt, mida kasutatakse koos xrdp-ga. Litsents — X11 (nagu MIT, kuid keelab reklaamides kasutamise)

Projekti arendamine põhineb rdesktopi ja FreeRDP tulemustel. Alguses oli graafikaga töötamiseks vajalik kasutada eraldi VNC serverit või spetsiaalset X11 serverit RDP toe jaoks — X11rdp, kuid xorgxrdp ilmumisega kadus vajadus nende järele.

Selles artiklis me xorgxrdp-d ei käsitle.

Projekt xrdp, nagu eelmine, on väga väike ja sisaldab umbes 80 000 rida.

Veel trükivigu

V525 Kood sisaldab sarnaste plokkide kogumit. Kontrollige elemente 'r', 'g', 'r' ridades 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 saadud librfxcodec raamatukogust, mis rakendab jpeg2000 koodeki RemoteFX tööks. Siit näib, et graafikandmed on segamini läinud - 'sinise' aja asemel salvestatakse 'punane'. Selline viga on tõenäoliselt tekkinud copy-paste'i tõttu.

Sama probleem tabas ka sarnast funktsiooni rfx_encode_format_argb, mille teatas meile ka analüsaator:

V525 Kood sisaldab sarnaste plokkide kogumit. Kontrollige elemente 'a', 'r', 'g', 'r' ridades 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. Indeksi väärtus 'i - 8' võib ulatuda kuni 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];
    ....
  }
  ....
}

Nende kahe faili massiivi deklareerimine ja määramine on mitteühtlane — suurus erineb ühe võrra. Kuid vigu ei esine — failis evdev-map.c on määratud õige suurus, seega pole piiridest väljumist. Seega on see lihtsalt viga, mille lihtne parandada.

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 lugemine tüübist unsigned short muutujasse tüübist int. Kontroll siin ei ole vajalik, kuna loeme muutuja mittesisemise tüübist ja määrame tulemuse suurema suurusega muutujasse, seega ei saa muutuja võtta negatiivset väärtust.

Tühjad kontrollid

V560 Osa tingimuslikust väljendusest 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: error");
      return 1;
  }
  ....
}

Kasutatav võrdsus kontroll siin ei ole mõttekas, kuna meil on juba alguses võrdlemine. On täiesti tõenäoline, et see on trükiviga ja arendaja tahtis kasutada operaatorit || vale argumentide filtreerimiseks.

Kokkuvõte

Kontrollimisel ei leitud tõsiseid vigu, kuid mitmeid puudusi tuli siiski esile. Need projektid on kasutusel paljuski süsteemides, kuigi nende maht on väike. Väikeses projekti ei pea tingimata olema palju vigu, seega ei tasu analüsaatori tööd hinnata ainult väikeste projektide põhjal. Rohkem teavet leiate artiklist «Tunded, mida numbrid kinnitasid«.

Võite alla laadida PVS-Studio prooviversiooni meie lehelt veebilehel.

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

Allikas: habr.com