Detta är den andra recensionen i en serie artiklar om att testa program med öppen källkod för att arbeta med RDP-protokollet. I den kommer vi att titta på rdesktop-klienten och xrdp-servern.
Används som ett verktyg för att identifiera fel . Det är en statisk kodanalysator för språken C, C++, C# och Java, tillgänglig på Windows, Linux och macOS-plattformar.
Artikeln presenterar bara de fel som verkade intressanta för mig. Projekten är dock små, så det blev få misstag :).
Notera. En tidigare artikel om FreeRDP-projektverifiering kan hittas .
rdesktop
— en gratis implementering av en RDP-klient för UNIX-baserade system. Det kan även användas under Windows om du bygger projektet under Cygwin. Licensierad under GPLv3.
Den här klienten är väldigt populär - den används som standard i ReactOS, och du kan även hitta grafiska gränssnitt från tredje part för den. Han är dock ganska gammal: hans första release ägde rum den 4 april 2001 - i skrivande stund är han 17 år gammal.
Som jag nämnde tidigare är projektet väldigt litet. Den innehåller ungefär 30 tusen rader kod, vilket är lite konstigt med tanke på dess ålder. Som jämförelse innehåller FreeRDP 320 tusen rader. Här är resultatet av Cloc-programmet:
Otillgänglig kod
Otillgänglig kod upptäckt. Det är möjligt att ett fel föreligger. 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);
}Felet stöter på oss direkt i funktionen huvudsakliga: vi ser koden komma efter operatören avkastning — detta fragment utför minnesrensning. Felet utgör dock inget hot: allt tilldelat minne kommer att rensas av operativsystemet efter att programmet avslutas.
Ingen felhantering
Array underrun är möjlig. Värdet på 'n' index 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);
}
....
}Kodavsnittet i detta fall läser från filen till en buffert tills filen tar slut. Det finns dock ingen felhantering här: om något går fel, då läsa returnerar -1, och sedan kommer arrayen att överskridas produktion.
Använder EOF i char-typ
EOF ska inte jämföras med ett värde av typen "char". '(c = fgetc(fp))' bör vara 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++;
}
....
}Här ser vi felaktig hantering av att nå slutet av filen: if fgetc returnerar ett tecken vars kod är 0xFF, det kommer att tolkas som slutet av filen (EOF).
EOF det är en konstant, vanligtvis definierad som -1. Till exempel, i CP1251-kodningen, har den sista bokstaven i det ryska alfabetet koden 0xFF, vilket motsvarar siffran -1 om vi pratar om en variabel som röding. Det visar sig att symbolen 0xFF, gillar EOF (-1) tolkas som slutet på filen. För att undvika sådana fel är resultatet av funktionen fgetc bör lagras i en variabel som int.
Skrivfel
Fragment 1
Uttrycket 'write_time' är alltid falskt. 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; // <=
....
}Kanske har författaren till den här koden missförstått || и && i skick. Låt oss överväga möjliga alternativ för värden skriv_tid и ändra tid:
- Båda variablerna är lika med 0: i det här fallet kommer vi att hamna i en gren annars: variabel mod_tid kommer alltid att vara 0 oavsett efterföljande tillstånd.
- En av variablerna är 0: mod_tid kommer att vara lika med 0 (förutsatt att den andra variabeln har ett icke-negativt värde), eftersom MIN kommer att välja det minsta av de två alternativen.
- Båda variablerna är inte lika med 0: välj minimivärdet.
När man byter ut villkoret med skriv_tid && ändra_tid beteendet kommer att se korrekt ut:
- En eller båda variablerna är inte lika med 0: välj ett värde som inte är noll.
- Båda variablerna är inte lika med 0: välj minimivärdet.
Fragment 2
Uttryck är alltid sant. Antagligen bör operatorn '&&' användas här. 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;
....
}Tydligen är operatörerna blandade här också || и &&Eller == и !=: En variabel kan inte ha värdet 20 och 9 samtidigt.
Obegränsad radkopiering
Ett anrop av 'sprintf'-funktionen kommer att leda till översvämning av bufferten '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 tittar på funktionen i sin helhet kommer det att bli tydligt att denna kod inte orsakar problem. Men de kan uppstå i framtiden: en slarvig förändring och vi kommer att få ett buffertspill - sprinta är inte begränsat av någonting, så när vi sammanfogar banor kan vi gå bortom gränserna för arrayen. Det rekommenderas att lägga märke till detta samtal snprintf(fullsökväg, PATH_MAX, ….).
Redundant skick
En del av det villkorliga uttrycket är alltid sant: lägg till > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка lägg till > 0 det finns inget behov här: variabeln kommer alltid att vara större än noll, eftersom läs % 4 kommer att returnera resten av divisionen, men det kommer aldrig att vara lika med 4.
xrdp
— Implementering av en RDP-server med öppen källkod. Projektet är uppdelat i 2 delar:
- xrdp - protokollimplementering. Distribueras under Apache 2.0-licensen.
- xorgxrdp - En uppsättning Xorg-drivrutiner för användning med xrdp. Licens - X11 (som MIT, men förbjuder användning i reklam)
Utvecklingen av projektet baseras på resultaten från rdesktop och FreeRDP. Inledningsvis, för att arbeta med grafik, var du tvungen att använda en separat VNC-server, eller en speciell X11-server med RDP-stöd - X11rdp, men med tillkomsten av xorgxrdp försvann behovet av dem.
I den här artikeln kommer vi inte att täcka xorgxrdp.
xrdp-projektet, liksom det tidigare, är väldigt litet och innehåller cirka 80 tusen rader.
Fler stavfel
Koden innehåller samlingen av liknande block. Kontrollera objekten 'r', 'g', 'r' på raderna 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++;
}
....
}
....
}Den här koden togs från biblioteket librfxcodec, som implementerar jpeg2000 codec för RemoteFX. Här är tydligen de grafiska datakanalerna blandade - istället för den "blå" färgen spelas "röd" in. Detta fel uppstod troligen som ett resultat av copy-paste.
Samma problem uppstod i en liknande funktion rfx_encode_format_argb, vilket analysatorn också berättade för oss:
Koden innehåller samlingen av liknande block. Kontrollera objekten 'a', 'r', 'g', 'r' på raderna 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-deklaration
Array-överskridning är möjlig. Värdet på 'i — 8'-index 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];
....
}
....
}Deklarationen och definitionen av arrayen i dessa två filer är inkompatibla - storleken skiljer sig med 1. Det uppstår dock inga fel - den korrekta storleken anges i filen evdev-map.c, så det finns ingen out of bounds. Så detta är bara en bugg som lätt kan fixas.
Felaktig jämförelse
En del av villkorligt uttryck är alltid falskt: (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 typvariabel osignerad kort till en variabel som int. Kontroll behövs inte här eftersom vi läser en variabel utan tecken och tilldelar resultatet till en större variabel, så variabeln kan inte ta ett negativt värde.
Onödiga kontroller
En del av villkorligt uttryck är 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;
}
....
}Ojämlikhetskontroller är inte meningsfulla här eftersom vi redan har en jämförelse i början. Det är troligt att detta är ett stavfel och utvecklaren ville använda operatören || för att filtrera bort ogiltiga argument.
Slutsats
Vid revisionen upptäcktes inga allvarliga fel, men många brister konstaterades. Dessa konstruktioner används dock i många system, även om de är små i omfattning. Ett litet projekt har inte nödvändigtvis många fel, så du bör inte bedöma analysatorns prestanda endast på små projekt. Du kan läsa mer om detta i artikeln "".
Du kan ladda ner en testversion av PVS-Studio från oss .
Om du vill dela den här artikeln med en engelsktalande publik, använd gärna översättningslänken: Sergey Larin.
Källa: will.com