Dette er den andre anmeldelsen i en serie artikler om testing av åpen kildekode-programmer for arbeid med RDP-protokollen. I den vil vi se på rdesktop-klienten og xrdp-serveren.
Brukes som et verktøy for å identifisere feil . Это статический анализатор кода для языков C, C++, C# и Java, доступный на платформах Windows, Linux и macOS.
Artikkelen presenterer bare de feilene som virket interessante for meg. Prosjektene er imidlertid små, så det ble få feil :).
Note. En tidligere artikkel om FreeRDP-prosjektverifisering kan bli funnet .
r skrivebord
— свободная реализация клиента RDP для UNIX-based систем. Его также можно использовать и под Windows, если собирать проект под Cygwin. Лицензирован под GPLv3.
Denne klienten er veldig populær - den brukes som standard i ReactOS, og du kan også finne tredjeparts grafiske grensesnitt for den. Han er imidlertid ganske gammel: hans første utgivelse fant sted 4. april 2001 - i skrivende stund er han 17 år gammel.
Som jeg nevnte tidligere, er prosjektet veldig lite. Den inneholder omtrent 30 tusen linjer med kode, noe som er litt merkelig med tanke på alderen. Til sammenligning inneholder FreeRDP 320 tusen linjer. Her er resultatet av Cloc-programmet:
Uoppnåelig kode
Utilgjengelig kode oppdaget. Det er mulig at det er en feil. 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);
}Feilen møter oss umiddelbart i funksjonen main: vi ser at koden kommer etter operatøren retur — dette fragmentet utfører minnerensing. Feilen utgjør imidlertid ingen trussel: alt tildelt minne vil bli slettet av operativsystemet etter at programmet avsluttes.
Ingen feilhåndtering
Array underrun er mulig. Verdien av 'n'-indeksen kan 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);
}
....
}Kodebiten i dette tilfellet leser fra filen inn i en buffer til filen slutter. Det er imidlertid ingen feilhåndtering her: hvis noe går galt, da lese vil returnere -1, og da vil matrisen bli overkjørt produksjon.
Bruker EOF i char type
EOF bør ikke sammenlignes med en verdi av typen "char". '(c = fgetc(fp))' bør være av 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 feil håndtering av å nå slutten av filen: if fgetc returnerer et tegn hvis kode er 0xFF, vil det bli tolket som slutten av filen (EOF).
EOF det er en konstant, vanligvis definert som -1. For eksempel, i CP1251-kodingen, har den siste bokstaven i det russiske alfabetet koden 0xFF, som tilsvarer tallet -1 hvis vi snakker om en variabel som chariot. Det viser seg at symbolet 0xFF, liker EOF (-1) tolkes som slutten av filen. For å unngå slike feil er resultatet av funksjonen fgetc bør lagres i en variabel som int.
Stavefeil
Fragment 1
Uttrykket 'skrivetid' er alltid usant. 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; // <=
....
}Kanskje forfatteren av denne koden tok feil || и && i stand. La oss vurdere mulige alternativer for verdier skrive_tid и change_time:
- Begge variablene er lik 0: i dette tilfellet vil vi havne i en gren ellers: variabel mod_tid vil alltid være 0 uavhengig av den påfølgende tilstanden.
- En av variablene er 0: mod_tid vil være lik 0 (forutsatt at den andre variabelen har en ikke-negativ verdi), fordi MIN vil velge det minste av de to alternativene.
- Begge variablene er ikke lik 0: velg minimumsverdien.
Ved erstatning av tilstanden med skrive_tid && endre_tid atferden vil se riktig ut:
- En eller begge variablene er ikke lik 0: velg en verdi som ikke er null.
- Begge variablene er ikke lik 0: velg minimumsverdien.
Fragment 2
Uttrykk er alltid sant. Sannsynligvis bør '&&'-operatoren brukes 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;
....
}Tilsynelatende er operatørene blandet her også || и &&Eller == и !=: En variabel kan ikke ha verdien 20 og 9 samtidig.
Ubegrenset linjekopiering
Et anrop av 'sprintf'-funksjonen vil føre til overløp av 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å funksjonen i sin helhet, vil det bli klart at denne koden ikke forårsaker problemer. Imidlertid kan de oppstå i fremtiden: en uforsiktig endring og vi vil få et bufferoverløp - sprint er ikke begrenset av noe, så når vi sammenkobler stier kan vi gå utover grensene til arrayet. Det anbefales å legge merke til denne oppfordringen snprintf(fullbane, PATH_MAX, ….).
Redundant tilstand
En del av betinget uttrykk er alltid sant: legg til > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка legg til > 0 det er ikke nødvendig her: variabelen vil alltid være større enn null, fordi les % 4 vil returnere resten av divisjonen, men den vil aldri være lik 4.
xrdp
— implementering av en RDP-server med åpen kildekode. Prosjektet er delt inn i 2 deler:
- xrdp - protokollimplementering. Distribuert under Apache 2.0-lisensen.
- xorgxrdp - Et sett med Xorg-drivere for bruk med xrdp. Lisens - X11 (som MIT, men forbyr bruk i reklame)
Utviklingen av prosjektet er basert på resultatene fra rdesktop og FreeRDP. I utgangspunktet, for å jobbe med grafikk, måtte du bruke en egen VNC-server, eller en spesiell X11-server med RDP-støtte - X11rdp, men med bruken av xorgxrdp forsvant behovet for dem.
I denne artikkelen vil vi ikke dekke xorgxrdp.
Xrdp-prosjektet, som det forrige, er veldig lite og inneholder omtrent 80 tusen linjer.
Flere skrivefeil
Koden inneholder samlingen av lignende blokker. Sjekk elementene 'r', 'g', 'r' i linjene 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 koden ble hentet fra librfxcodec-biblioteket, som implementerer jpeg2000-kodeken for RemoteFX. Her er tilsynelatende de grafiske datakanalene blandet sammen - i stedet for den "blå" fargen, er "rød" registrert. Denne feilen dukket mest sannsynlig opp som et resultat av copy-paste.
Det samme problemet oppsto i en lignende funksjon rfx_encode_format_argb, som analysatoren også fortalte oss:
Koden inneholder samlingen av lignende blokker. Sjekk elementene 'a', 'r', 'g', 'r' i linjene 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 overrun er mulig. Verdien av 'i — 8'-indeksen kan 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];
....
}
....
}Deklarasjonen og definisjonen av matrisen i disse to filene er inkompatible - størrelsen avviker med 1. Det oppstår imidlertid ingen feil - riktig størrelse er spesifisert i evdev-map.c-filen, så det er ingen out of bounds. Så dette er bare en feil som lett kan fikses.
Feil sammenligning
En del av betinget uttrykk er alltid 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))
{
....
}
....
}Funksjonen leser en typevariabel usignert kort inn i en variabel som int. Kontroll er ikke nødvendig her fordi vi leser en variabel uten fortegn og tilordner resultatet til en større variabel, så variabelen kan ikke ta en negativ verdi.
Unødvendige kontroller
En del av betinget uttrykk er alltid sant: (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;
}
....
}Ulikhetskontroller gir ikke mening her siden vi allerede har en sammenligning i begynnelsen. Det er sannsynlig at dette er en skrivefeil og utvikleren ønsket å bruke operatøren || for å filtrere ut ugyldige argumenter.
Konklusjon
Under tilsynet ble det ikke avdekket alvorlige feil, men det ble funnet mange mangler. Imidlertid brukes disse designene i mange systemer, selv om de er små i omfang. Et lite prosjekt har ikke nødvendigvis mange feil, så du bør ikke bedømme analysatorens ytelse kun på små prosjekter. Du kan lese mer om dette i artikkelen "".
Du kan laste ned en prøveversjon av PVS-Studio fra oss .
Hvis du vil dele denne artikkelen med et engelsktalende publikum, vennligst bruk oversettelseslenken: Sergey Larin.
Kilde: www.habr.com
