Kontrolante rdesktop kaj xrdp per la analizilo PVS-Studio

Ĉi tiu estas la dua revizio en serio de artikoloj pri testado de malfermkodaj programoj por labori kun la RDP-protokolo. En ĝi ni rigardos la rdesktop-klienton kaj la xrdp-servilon.

Uzita kiel ilo por identigi erarojn PVS Studio. Это статический анализатор кода для языков C, C++, C# и Java, доступный на платформах Windows, Linux и macOS.

La artikolo prezentas nur tiujn erarojn, kiuj ŝajnis al mi interesaj. Tamen la projektoj estas malgrandaj, do estis malmultaj eraroj :).

Примечание. Antaŭa artikolo pri FreeRDP-projekta konfirmo troveblas tie.

rdesktop

rdesktop — свободная реализация клиента RDP для UNIX-based систем. Его также можно использовать и под Windows, если собирать проект под Cygwin. Лицензирован под GPLv3.

Ĉi tiu kliento estas tre populara - ĝi estas uzata defaŭlte en ReactOS, kaj vi ankaŭ povas trovi triajn grafikajn front-finojn por ĝi. Tamen li estas sufiĉe maljuna: lia unua eldono okazis la 4-an de aprilo 2001 - en la momento de la skribado, li estas 17-jara.

Kiel mi rimarkis antaŭe, la projekto estas sufiĉe malgranda. Ĝi enhavas proksimume 30 mil liniojn de kodo, kio estas iom stranga konsiderante ĝia aĝo. Por komparo, FreeRDP enhavas 320 mil liniojn. Jen la eligo de la programo Cloc:

Neatingebla kodo

V779 Nehavebla kodo detektita. Eblas, ke eraro ĉeestas. 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);
}

La eraro renkontas nin tuj en la funkcio ĉefa: ni vidas la kodon venanta post la operatoro reveno — ĉi tiu fragmento faras memorpurigadon. Tamen, la eraro ne prezentas minacon: la tuta asignita memoro estos forigita de la operaciumo post la eliro de la programo.

Neniu erartraktado

V557 Array underrun eblas. La valoro de 'n' indekso povus atingi -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);
  }
  ....
}

La koda fragmento ĉi-kaze legas el la dosiero en bufron ĝis la dosiero finiĝas. Tamen ĉi tie ne estas erartraktado: se io misfunkcias, do legi revenos -1, kaj tiam la tabelo estos superfluita produktado.

Uzante EOF en char tipo

V739 EOF ne devus esti komparita kun valoro de la 'char' tipo. La '(c = fgetc(fp))' devus esti de la tipo '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++;
  }
  ....
}

Ĉi tie ni vidas malĝustan uzadon atingi la finon de la dosiero: if fgetc redonas signon kies kodo estas 0xFF, ĝi estos interpretita kiel la fino de la dosiero (EOF).

EOF ĝi estas konstanto, kutime difinita kiel -1. Ekzemple, en la kodigo CP1251, la lasta litero de la rusa alfabeto havas la kodon 0xFF, kiu respondas al la numero -1 se ni parolas pri variablo kiel char. Rezultas, ke la simbolo 0xFF, kiel EOF (-1) estas interpretita kiel la fino de la dosiero. Por eviti tiajn erarojn, la rezulto de la funkcio estas fgetc devus esti stokita en variablo kiel int.

Tajperaroj

Fragmento 1

V547 Esprimo 'write_time' ĉiam estas malvera. disko.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; // <=
  ....
}

Eble la aŭtoro de ĉi tiu kodo eraris ĝin || и && en kondiĉo. Ni konsideru eblajn eblojn por valoroj skrib_tempo и ŝanĝo_tempo:

• Ambaŭ variabloj estas egalaj al 0: ĉi-kaze ni finiĝos en branĉo alia: varia mod_tempo ĉiam estos 0 sendepende de la posta kondiĉo.
• Unu el la variabloj estas 0: mod_tempo estos egala al 0 (kondiĉe ke la alia variablo havas nenegativan valoron), ĉar MIN elektos la pli malgrandan el la du opcioj.
• Ambaŭ variabloj ne egalas al 0: elektu la minimuman valoron.

Kiam oni anstataŭigas la kondiĉon per skrib_tempo && ŝanĝi_tempo la konduto aspektos ĝusta:

• Unu aŭ ambaŭ variabloj ne egalas al 0: elektu ne-nulan valoron.
• Ambaŭ variabloj ne egalas al 0: elektu la minimuman valoron.

Fragmento 2

V547 Esprimo ĉiam estas vera. Verŝajne la operatoro '&&' estu uzata ĉi tie. 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;
  ....
}

Ŝajne ankaŭ la telefonistoj estas miksitaj ĉi tie || и &&aŭ == и !=: variablo ne povas havi la valoron 20 kaj 9 samtempe.

Senlima linio kopiado

V512 Voko de la funkcio 'sprintf' kondukos al superfluo de la bufro 'fullpath'. disk.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);
  ....
}

Kiam vi rigardos la funkcion plene, evidentiĝos, ke ĉi tiu kodo ne kaŭzas problemojn. Tamen, ili povas estiĝi estonte: unu senzorga ŝanĝo kaj ni ricevos bufran superfluon - spurto ne estas limigita de io ajn, do kiam kunkatenante vojojn ni povas iri preter la limoj de la tabelo. Oni rekomendas rimarki ĉi tiun vokon snprintf (plena vojo, PATH_MAX, ....).

Redunda kondiĉo

V560 Parto de kondiĉa esprimo estas ĉiam vera: aldonu > 0. scard.c 507

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

inspektado aldonu > 0 ne necesas ĉi tie: la variablo ĉiam estos pli granda ol nulo, ĉar legi % 4 redonos la reston de la divido, sed ĝi neniam estos egala al 4.

xrdp

xrdp — efektivigo de RDP-servilo kun malferma fontkodo. La projekto estas dividita en 2 partojn:

• xrdp - protokola efektivigo. Distribuite sub la permesilo Apache 2.0.
• xorgxrdp - Aro de Xorg-ŝoforoj por uzo kun xrdp. Licenco - X11 (kiel MIT, sed malpermesas uzon en reklamado)

La evoluo de la projekto baziĝas sur la rezultoj de rdesktop kaj FreeRDP. Komence, por labori kun grafikaĵoj, vi devis uzi apartan VNC-servilon, aŭ specialan X11-servilon kun RDP-subteno - X11rdp, sed kun la apero de xorgxrdp, la bezono de ili malaperis.

En ĉi tiu artikolo ni ne kovros xorgxrdp.

La projekto xrdp, kiel la antaŭa, estas tre malgranda kaj enhavas proksimume 80 mil liniojn.

Pli da tajperaroj

V525 La kodo enhavas la kolekton de similaj blokoj. Kontrolu erojn 'r', 'g', 'r' en linioj 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++;
      }
      ....
  }
  ....
}

Ĉi tiu kodo estis prenita el la biblioteko librfxcodec, kiu efektivigas la jpeg2000-kodekon por RemoteFX. Ĉi tie, ŝajne, la grafikaj datumkanaloj estas miksitaj - anstataŭ la "blua" koloro, "ruĝa" estas registrita. Ĉi tiu eraro plej verŝajne aperis kiel rezulto de kopio-gluo.

La sama problemo okazis en simila funkcio rfx_encode_format_argb, kiun la analizisto ankaŭ diris al ni:

V525 La kodo enhavas la kolekton de similaj blokoj. Kontrolu erojn 'a', 'r', 'g', 'r' en linioj 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++;
}

Array Deklaro

V557 Array transpaso eblas. La valoro de 'i — 8' indekso povus atingi 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];
    ....
  }
  ....
}

La deklaro kaj difino de la tabelo en ĉi tiuj du dosieroj estas malkongruaj - la grandeco diferencas je 1. Tamen, neniuj eraroj okazas - la ĝusta grandeco estas specifita en la dosiero evdev-map.c, do ne ekzistas ekster limoj. Do ĉi tio estas nur cimo, kiu povas esti facile riparita.

Malĝusta komparo

V560 Parto de kondiĉa esprimo ĉiam estas malvera: (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))
  {
    ....
  }
  ....
}

La funkcio legas tipvariablon sennoma mallonga en variablon kiel int. Kontrolado ne necesas ĉi tie ĉar ni legas nesignatan variablon kaj asignas la rezulton al pli granda variablo, do la variablo ne povas preni negativan valoron.

Nenecesaj kontroloj

V560 Parto de kondiĉa esprimo ĉiam estas vera: (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;
  }
  ....
}

Neegalecaj kontroloj ĉi tie ne havas sencon, ĉar ni jam havas komparon komence. Verŝajnas, ke ĉi tio estas tajperaro kaj la programisto volis uzi la funkciigiston || por filtri nevalidajn argumentojn.

konkludo

Dum la revizio, neniuj gravaj eraroj estis identigitaj, sed multaj mankoj estis trovitaj. Tamen, ĉi tiuj dezajnoj estas uzataj en multaj sistemoj, kvankam malgrandaj en amplekso. Eta projekto ne nepre havas multajn erarojn, do vi ne juĝu la agadon de la analizilo nur pri malgrandaj projektoj. Vi povas legi pli pri tio en la artikolo "Sentoj, kiuj estis konfirmitaj per nombroj".

Vi povas elŝuti provversion de PVS-Studio de ni ejo.

Se vi volas dividi ĉi tiun artikolon kun anglalingva publiko, bonvolu uzi la tradukan ligilon: Sergey Larin. Kontrolante rdesktop kaj xrdp kun PVS-Studio

fonto: www.habr.com