Dette er den anden anmeldelse i en serie af artikler om test af open source-programmer til at arbejde med RDP-protokollen. I den vil vi se på rdesktop-klienten og xrdp-serveren.
Bruges som et værktøj til at identificere fejl . Det er en statisk kodeanalysator til C, C++, C# og Java sprog, tilgængelig på Windows, Linux og macOS platforme.
Artiklen præsenterer kun de fejl, der forekom interessante for mig. Projekterne er dog små, så der var få fejl :).
Bemærk. En tidligere artikel om FreeRDP-projektverifikation kan findes .
r skrivebord
— en gratis implementering af en RDP-klient til UNIX-baserede systemer. Det kan også bruges under Windows, hvis du bygger projektet under Cygwin. Licenseret under GPLv3.
Denne klient er meget populær - den bruges som standard i ReactOS, og du kan også finde tredjeparts grafiske frontends til den. Han er dog ret gammel: hans første udgivelse fandt sted den 4. april 2001 - i skrivende stund er han 17 år.
Som jeg nævnte tidligere, er projektet ret lille. Den indeholder cirka 30 tusinde linjer kode, hvilket er lidt mærkeligt i betragtning af dens alder. Til sammenligning indeholder FreeRDP 320 tusind linjer. Her er output fra Cloc-programmet:
Uopnåelig kode
Utilgængelig kode fundet. Det er muligt, at der er en fejl. 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);
}Fejlen støder på os med det samme i funktionen main: vi ser koden komme efter operatøren afkast — dette fragment udfører hukommelsesrensning. Fejlen udgør dog ikke en trussel: al allokeret hukommelse vil blive ryddet af operativsystemet, når programmet afsluttes.
Ingen fejlhåndtering
Array-underløb er muligt. Værdien af 'n' indeks kunne nå -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);
}
....
}Kodestykket i dette tilfælde læser fra filen ind i en buffer, indtil filen slutter. Der er dog ingen fejlhåndtering her: hvis noget går galt, så læse vil returnere -1, og derefter vil arrayet blive overskredet output.
Brug af EOF i char type
EOF bør ikke sammenlignes med en værdi af typen 'char'. '(c = fgetc(fp))' skal være af typen '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++;
}
....
}Her ser vi forkert håndtering af at nå slutningen af filen: if fgetc returnerer et tegn, hvis kode er 0xFF, vil det blive fortolket som slutningen af filen (EOF).
EOF det er en konstant, normalt defineret som -1. For eksempel i CP1251-kodningen har det sidste bogstav i det russiske alfabet koden 0xFF, som svarer til tallet -1, hvis vi taler om en variabel som f.eks. char. Det viser sig, at symbolet 0xFF, ligesom EOF (-1) fortolkes som slutningen af filen. For at undgå sådanne fejl er resultatet af funktionen fgetc skal gemmes i en variabel som int.
Slåfejl
Fragment 1
Udtrykket 'write_time' er altid falsk. 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; // <=
....
}Måske har forfatteren til denne kode taget fejl || и && i stand. Lad os overveje mulige muligheder for værdier skrive_tid и change_time:
- Begge variabler er lig med 0: i dette tilfælde ender vi i en gren andet: variabel mod_tid vil altid være 0 uanset den efterfølgende betingelse.
- En af variablerne er 0: mod_tid vil være lig med 0 (forudsat at den anden variabel har en ikke-negativ værdi), fordi MIN vil vælge den mindste af de to muligheder.
- Begge variabler er ikke lig med 0: vælg minimumsværdien.
Ved udskiftning af tilstanden med skrive_tid && ændre_tid adfærden vil se korrekt ud:
- En eller begge variable er ikke lig med 0: vælg en værdi, der ikke er nul.
- Begge variabler er ikke lig med 0: vælg minimumsværdien.
Fragment 2
Udtryk er altid sandt. Sandsynligvis skal '&&'-operatoren bruges her. 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;
....
}Tilsyneladende er operatørerne også blandet her || и &&Eller == и !=: En variabel kan ikke have værdien 20 og 9 på samme tid.
Ubegrænset linjekopiering
Et kald af 'sprintf'-funktionen vil føre til overløb af bufferen '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);
....
}Når du ser på funktionen fuldt ud, vil det blive klart, at denne kode ikke giver problemer. Men de kan opstå i fremtiden: én skødesløs ændring, og vi vil få et bufferoverløb - sprint er ikke begrænset af noget, så når vi sammenkæder stier, kan vi gå ud over arrayets grænser. Det anbefales at lægge mærke til denne opfordring snprintf(fuldsti, PATH_MAX, ….).
Redundant stand
En del af det betingede udtryk er altid sandt: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}inspektion tilføje > 0 det er ikke nødvendigt her: variablen vil altid være større end nul, fordi læs % 4 vil returnere resten af divisionen, men den vil aldrig være lig med 4.
xrdp
— implementering af en RDP-server med åben kildekode. Projektet er opdelt i 2 dele:
- xrdp - protokolimplementering. Distribueret under Apache 2.0-licensen.
- xorgxrdp - Et sæt Xorg-drivere til brug med xrdp. Licens - X11 (som MIT, men forbyder brug i reklamer)
Udviklingen af projektet er baseret på resultaterne af rdesktop og FreeRDP. I første omgang, for at arbejde med grafik, skulle du bruge en separat VNC-server eller en speciel X11-server med RDP-understøttelse - X11rdp, men med fremkomsten af xorgxrdp forsvandt behovet for dem.
I denne artikel vil vi ikke dække xorgxrdp.
xrdp-projektet er ligesom det forrige meget lille og indeholder cirka 80 tusind linjer.
Flere stavefejl
Koden indeholder samlingen af lignende blokke. Tjek punkterne 'r', 'g', 'r' i linje 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++;
}
....
}
....
}Denne kode blev taget fra biblioteket librfxcodec, som implementerer jpeg2000 codec til RemoteFX. Her er tilsyneladende de grafiske datakanaler blandet sammen - i stedet for den "blå" farve optages "rød". Denne fejl opstod højst sandsynligt som et resultat af copy-paste.
Det samme problem opstod i en lignende funktion rfx_encode_format_argb, som analysatoren også fortalte os:
Koden indeholder samlingen af lignende blokke. Tjek punkterne 'a', 'r', 'g', 'r' i linje 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-erklæring
Array-overløb er muligt. Værdien af 'i — 8'-indekset kunne nå 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];
....
}
....
}Deklarationen og definitionen af arrayet i disse to filer er inkompatible - størrelsen adskiller sig med 1. Der opstår dog ingen fejl - den korrekte størrelse er angivet i evdev-map.c filen, så der er ingen out of bounds. Så dette er bare en fejl, der nemt kan rettes.
Forkert sammenligning
En del af det betingede udtryk er altid falsk: (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))
{
....
}
....
}Funktionen læser en typevariabel usigneret kort ind i en variabel som int. Kontrol er ikke nødvendig her, fordi vi læser en variabel uden fortegn og tildeler resultatet til en større variabel, så variablen kan ikke tage en negativ værdi.
Unødvendige kontroller
En del af det betingede udtryk er altid sandt: (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;
}
....
}Ulighedskontrol giver ikke mening her, da vi allerede har en sammenligning i begyndelsen. Det er sandsynligt, at dette er en tastefejl, og udvikleren ønskede at bruge operatøren || for at bortfiltrere ugyldige argumenter.
Konklusion
Under revisionen blev der ikke konstateret alvorlige fejl, men der blev konstateret mange mangler. Disse designs bruges dog i mange systemer, om end små i omfang. Et lille projekt har ikke nødvendigvis mange fejl, så du bør ikke bedømme analysatorens ydeevne kun på små projekter. Det kan du læse mere om i artiklen "".
Du kan downloade en prøveversion af PVS-Studio fra os .
Hvis du vil dele denne artikel med et engelsktalende publikum, så brug venligst oversættelseslinket: Sergey Larin.
Kilde: www.habr.com