Testování rdesktop a xrdp pomocí analyzátoru PVS-Studio

Toto je druhá recenze v cyklu článků o kontrole otevřených programů pro práci s protokolem RDP. V ní se podíváme na klienta rdesktop a server xrdp.

Jako nástroj pro odhalování chyb byl použit PVS-Studio. Je to statický analyzátor kódu pro jazyky C, C++, C# a Java, dostupný na platformách Windows, Linux a macOS.

V článku jsou uvedeny pouze ty chyby, které se mi zdály zajímavé. Nicméně, projekty jsou malé, a proto bylo i málo chyb :)).

Poznámka. Předchozí článek o kontrole projektu FreeRDP najdete zde.

rdesktop

rdesktop — volná implementace klienta RDP pro systémy UNIX. Může být také použit na Windows, pokud projekt sestavíte s Cygwin. Licencováno pod GPLv3.

Tento klient má velkou popularitu — je používán jako výchozí v ReactOS, také pro něj lze najít externí grafické front-endy. Přesto je poměrně starý: první vydání bylo 4. dubna 2001 — v době psaní článku je mu 17 let.

Jak jsem již zmínil, projekt je velmi malý. Obsahuje přibližně 30 tisíc řádků kódu, což je trochu zvláštní vzhledem k jeho stáří. Pro srovnání, FreeRDP obsahuje 320 tisíc řádků. Zde je výstup programu Cloc:

Nedosažitelný kód

V779 Byl detekován nedosažitelný kód. Je možné, že je přítomna chyba. 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);
}

Chyba nás vítá ihned ve funkci main: vidíme kód jdoucí za operátorem vrátit — tento fragment provádí čištění paměti. Nicméně, chyba nepředstavuje hrozbu: veškerá alokovaná paměť bude vyčištěna operačním systémem po skončení práce programu.

Nedostatek zpracování chyb

V557 Možný je podtok pole. Hodnota indexu 'n' by mohla dosáhnout -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 kódu v tomto případě provádí čtení ze souboru do bufferu, dokud soubor neskončí. Nicméně, zpracování chyb zde chybí: pokud něco nevyjde, read vrátí -1, a pak dojde k překročení pole output.

Použití EOF v typu char

V739 EOF by nemělo být porovnáváno s hodnotou typu 'char'. Výraz '(c = fgetc(fp))' by měl být typu '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++;
  }
  ....
}

Zde vidíme nesprávné zpracování dosažení konce souboru: pokud fgetc vrátí symbol, jehož kód je 0xFF, bude vnímán jako konec souboru (EOF).

EOF toto je obvykle definováno jako -1. Například v kódování CP1251 má poslední písmeno ruské abecedy kód 0xFF, což odpovídá číslu -1, pokud mluvíme o proměnné typu char. Ukazuje se, že symbol 0xFF, stejně jako EOF (-1) je vnímán jako konec souboru. Abychom se vyhnuli podobným chybám, výsledky práce funkce fgetc by měly být uchovávány v proměnné typu int.

Tiskové chyby

Fragment 1

V547 Výraz „write_time“ je vždy nepravdivý. 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žná autor tohoto kódu zaměnil || a && ve výrazu. Zvážíme možné varianty hodnot write_time a change_time:

• Obě proměnné jsou rovny 0: v tomto případě se dostaneme do bloku else: proměnná mod_time bude vždy rovna 0 bez ohledu na následující podmínku.
• Jedna z proměnných je rovna 0: mod_time bude rovno 0 (za předpokladu, že jiná proměnná má nezápornou hodnotu), protože MIN vybere menší z obou možností.
• Obě proměnné nejsou rovny 0: vybíráme minimální hodnotu.

Pokud změníme podmínku na write_time && change_time se chování bude jevit jako správné:

• Jedna nebo obě proměnné nejsou rovny 0: vybíráme nenulovou hodnotu.
• Obě proměnné nejsou rovny 0: vybíráme minimální hodnotu.

Fragment 2

V547 Výraz je vždy pravdivý. Pravděpodobně by se zde měl použít operátor „&&“. 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;
  ....
}

Zřejmě byly také zaměněny operátory || a &&, nebo == a !=: proměnná nemůže současně nabývat hodnoty 20 a 9.

Neomezené kopírování řetězce

V512 Zavolání funkce ‚sprintf‘ povede k přetečení vyrovnávací paměti ‚fullpath‘. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Získejte informace o položce adresáře */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Při zkoumání funkce jako celku bude jasné, že tento kód nevyvolává problémy. Mohou se však vyskytnout v budoucnu: jedna neopatrná změna a dojde k přetečení vyrovnávací paměti — sprintf není nijak omezen, proto při spojování cest můžeme překročit hranice pole. Doporučuje se zaznamenat tento výstup na snprintf(fullpath, PATH_MAX, ….).

Nadbytečná podmínka

V560 Část podmínkového výrazu je vždy pravdivá: add > 0. scard.c 507

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

Kontrola add > 0 tady není potřeba: proměnná bude vždy větší než nula, protože read % 4 vrátí zbytek po dělení, a ten nikdy nebude roven 4.

xrdp

xrdp — implementace RDP serveru s otevřeným zdrojovým kódem. Projekt je rozdělen na 2 části:

• xrdp — implementace protokolu. Rozšiřuje se pod licencí Apache 2.0.
• xorgxrdp — sada ovladačů Xorg pro použití s xrdp. Licence — X11 (podobně jako MIT, ale zakazuje použití v reklamě)

Vývoj projektu je založen na výsledcích rdesktop a FreeRDP. Původně pro práci s grafikou bylo nutné použít samostatný VNC server nebo speciální server X11 s podporou RDP — X11rdp, avšak s příchodem xorgxrdp tato potřeba odpadla.

V tomto článku se nebudeme zabývat xorgxrdp.

Projekt xrdp, stejně jako předchozí, je velmi malý a obsahuje přibližně 80 tisíc řádků.

Další překlepy

V525 Kód obsahuje sbírku podobných bloků. Zkontrolujte položky 'r', 'g', 'r' v řádcích 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++;
      }
      ....
  }
  ....
}

Tento kód byl převzat z knihovny librfxcodec, která implementuje kodek jpeg2000 pro práci s RemoteFX. Zde jsou zřejmě zaměněny kanály grafických dat — místo 'modré' barvy je zapsána 'červená'. Taková chyba se pravděpodobně objevila v důsledku copy-paste.

Tato stejná problém se objevila i v podobné funkci rfx_encode_format_argb, o čemž nás také informoval analyzátor:

V525 Kód obsahuje sbírku podobných bloků. Zkontrolujte položky 'a', 'r', 'g', 'r' v řádcích 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++;
}

Deklarace pole

V557 Přetečení pole je možné. Hodnota indexu 'i - 8' by mohla dosáhnout 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];
    ....
  }
  ....
}

Deklarace a definice pole v těchto dvou souborech nejsou kompatibilní — velikost se liší o 1. Nicméně žádné chyby nenastávají — ve souboru evdev-map.c je uvedena správná velikost, takže k přetečení nedochází. Takže je to pouze nedopatření, které lze snadno odstranit.

Nesprávné porovnání

V560 Část podmínkového výrazu je vždy nepravdivá: (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))
  {
    ....
  }
  ....
}

Ve funkci se provádí čtení proměnné typu unsigned short do proměnné typu int. Kontrola zde není potřeba, protože čteme proměnnou bez znaménka a přiřazujeme výsledek větší proměnné, takže proměnná nemůže nabýt záporné hodnoty.

Zbytečné kontroly

V560 Část podmínkového výrazu je vždy pravdivá: (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;
  }
  ....
}

Kontroly na nerovnost zde nemají smysl, protože už máme srovnání na začátku. Je možné, že se jedná o překlep a vývojář chtěl použít operátor || k filtrování neplatných argumentů.

Závěr

Během kontroly nebyly zjištěny závažné chyby, ale byla nalezena řada nedostatků. Přesto se tyto projekty využívají v mnoha systémech, i když jsou malé. V malém projektu nemusí být nutně mnoho chyb, a proto není dobré soudit práci analyzátoru pouze na základě malých projektů. Více o tom lze přečíst v článku „Pocity, které potvrdila čísla«.

Můžete si stáhnout zkušební verzi PVS-Studio u nás na webu.

Pokud chcete sdílet tento článek s anglicky mluvící veřejností, použijte prosím odkaz na překlad: Sergey Larin. Kontrola rdesktop a xrdp s PVS-Studio

Zdroj: habr.com