Questa è la seconda recensione di un ciclo di articoli sulla revisione di programmi open source per lavorare con il protocollo RDP. In essa esamineremo il client rdesktop e il server xrdp.
Come strumento per l'identificazione degli errori è stato utilizzato . Questo è un analizzatore statico di codice per i linguaggi C, C++, C# e Java, disponibile sulle piattaforme Windows, Linux e macOS.
Nell'articolo sono presentati solo quegli errori che mi sono sembrati interessanti. D'altra parte, i progetti sono piccoli, quindi ci sono stati pochi errori :).
Nota. L'articolo precedente sulla revisione del progetto FreeRDP può essere trovato .
rdesktop
— è un'implementazione libera del client RDP per sistemi UNIX-based. Può essere utilizzato anche su Windows, se si compila il progetto per Cygwin. È concesso in licenza sotto GPLv3.
Questo client è molto popolare: è usato di default in ReactOS e per esso si possono trovare frontend grafici di terze parti. Tuttavia, è piuttosto vecchio: la prima versione è stata rilasciata il 4 aprile 2001 — al momento della scrittura dell'articolo ha già 17 anni.
Come ho già notato in precedenza, il progetto è davvero piccolo. Contiene circa 30.000 righe di codice, il che è un po' strano, considerando la sua età. Per confronto, FreeRDP contiene 320.000 righe. Ecco l'output del programma Cloc:
Codice non raggiungibile
Codice non raggiungibile rilevato. È possibile che ci sia un errore. 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);
}L'errore ci colpisce subito nella funzione main: vediamo il codice che segue l'operatore return — questo frammento effettua la pulizia della memoria. Tuttavia, l'errore non rappresenta una minaccia: tutta la memoria allocata sarà liberata dal sistema operativo al termine dell'esecuzione del programma.
Mancanza di gestione degli errori
È possibile un underrun dell'array. Il valore dell'indice 'n' potrebbe arrivare a -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);
}
....
}Il frammento di codice in questo caso legge da un file in un buffer fino a quando il file non finisce. Tuttavia, qui manca la gestione degli errori: se qualcosa va storto, read restituirà -1, causando così un overflow dell'array output.
Uso di EOF nel tipo char
EOF non dovrebbe essere confrontato con un valore di tipo 'char'. Il '(c = fgetc(fp))' dovrebbe essere di 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++;
}
....
}Qui vediamo un'elaborazione non corretta del raggiungimento della fine del file: se fgetc restituisce un carattere il cui codice è uguale a 0xFF, verrà interpretato come fine del file (EOF).
EOF è una costante solitamente definita come -1. Ad esempio, nella codifica CP1251 l'ultima lettera dell'alfabeto russo ha codice 0xFF, che corrisponde al numero -1, se parliamo di una variabile di tipo char. Risulta che il carattere 0xFF, così come EOF (-1) è percepito come fine del file. Per evitare tali errori, il risultato del funzionamento della funzione fgetc dovrebbe essere memorizzato in una variabile di tipo int.
Refusi
Frammento 1
L'espressione ‘write_time’ è sempre falsa. 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; // <=
....
}Forse l'autore di questo codice ha confuso || e && nelle condizioni. Consideriamo i possibili valori di write_time e change_time:
- Entrambe le variabili sono uguali a 0: in questo caso andremo nel ramo else: la variabile mod_time sarà sempre uguale a 0 indipendentemente dalla condizione successiva.
- Una delle variabili è uguale a 0: mod_time sarà uguale a 0 (a condizione che l'altra variabile abbia un valore non negativo), poiché MIN sceglierà il valore minore tra i due.
- Entrambe le variabili non sono uguali a 0: scegliamo il valore minimo.
Modificando la condizione in write_time && change_time il comportamento apparirà corretto:
- Una o entrambe le variabili non sono uguali a 0: scegliamo un valore non nullo.
- Entrambe le variabili non sono uguali a 0: scegliamo il valore minimo.
Frammento 2
L'espressione è sempre vera. Probabilmente l'operatore ‘&&’ dovrebbe essere usato qui. 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;
....
}Sembra che anche qui gli operatori siano stati confusi || e &&, oppure == e !=: la variabile non può assumere contemporaneamente il valore 20 e 9.
Copia illimitata della stringa
Una chiamata alla funzione ‘sprintf’ porterà a un overflow del buffer ‘fullpath’. disk.c 1257
RD_NTSTATUS
disk_query_directory(....)
{
....
char *dirname, fullpath[PATH_MAX];
....
/* Ottieni informazioni per l'entrata della directory */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Esaminando completamente la funzione diventa chiaro che questo codice non causa problemi. Tuttavia, potrebbero sorgere in futuro: una modifica imprudente e avremo un overflow del buffer — sprintf non è soggetto a restrizioni, quindi nella concatenazione dei percorsi possiamo superare i limiti dell'array. È consigliato notare questa chiamata su snprintf(fullpath, PATH_MAX, ….).
Condizione ridondante
Una parte dell'espressione condizionale è sempre vera: aggiungi > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add 0)
{
....
}
}Verifica add > 0 qui non serve a nulla: la variabile sarà sempre maggiore di zero, poiché read % 4 restituirà il resto della divisione, e non sarà mai uguale a 4.
xrdp
— implementazione di un server RDP con codice sorgente aperto. Il progetto è suddiviso in 2 parti:
- xrdp — implementazione del protocollo. È distribuito sotto licenza Apache 2.0.
- xorgxrdp — insieme di driver Xorg da utilizzare con xrdp. Licenza — X11 (come MIT, ma vieta l'uso in pubblicità)
Lo sviluppo del progetto si basa sui risultati di rdesktop e FreeRDP. Inizialmente, per lavorare con la grafica era necessario utilizzare un server VNC separato, oppure un server X11 speciale con supporto RDP — X11rdp, ma con l'arrivo di xorgxrdp non è più necessario.
In questo articolo non tratteremo xorgxrdp.
Il progetto xrdp, come il precedente, è piuttosto piccolo e contiene circa 80.000 righe.
Altre imprecisioni
Il codice contiene una raccolta di blocchi simili. Controlla gli elementi ‘r’, ‘g’, ‘r’ nelle righe 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++;
}
....
}
....
}Questo codice è stato preso dalla libreria librfxcodec, che implementa il codec jpeg2000 per lavorare con RemoteFX. Qui, evidentemente, sono stati confusi i canali dei dati grafici: al posto del colore "blu" viene registrato "rosso". Un errore di questo tipo è probabilmente emerso a causa di un copy-paste.
Anche questo problema si è presentato in una funzione simile rfx_encode_format_argb, come ci ha anche informato l'analizzatore:
Il codice contiene una raccolta di blocchi simili. Controlla gli elementi ‘a’, ‘r’, ‘g’, ‘r’ nelle righe 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++;
}Dichiarazione dell'array
L'overflow dell'array è possibile. Il valore dell'indice ‘i - 8’ potrebbe raggiungere 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 dichiarazione e la definizione dell'array in questi due file non sono compatibili: le dimensioni differiscono di 1. Tuttavia, non si verificano errori — nel file evdev-map.c è stata indicata la dimensione corretta, quindi non ci sono sovrapposizioni. Quindi è solo una svista, che è facile correggere.
Confronto non corretto
Una parte dell'espressione condizionale è sempre falsa: (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))
{
....
}
....
}Nella funzione si legge una variabile di tipo unsigned short in una variabile di tipo int. Non è necessaria alcuna verifica qui, poiché leggiamo una variabile di tipo non firmato e assegniamo il risultato a una variabile di dimensioni maggiori, quindi la variabile non può assumere un valore negativo.
Verifiche inutili
Una parte dell'espressione condizionale è sempre 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: errore");
return 1;
}
....
}Le verifiche di disuguaglianza qui non hanno senso, poiché abbiamo già un confronto all'inizio. È probabile che si tratti di un errore di battitura e lo sviluppatore volesse usare l'operatore || per filtrare gli argomenti errati.
Conclusione
Durante la verifica non sono stati riscontrati errori gravi, ma sono emersi molti punti deboli. Tuttavia, questi progetti vengono utilizzati in molti sistemi, anche se di piccole dimensioni. In un progetto di piccole dimensioni non è necessario avere molti errori, quindi non bisogna giudicare il lavoro dell'analizzatore solo su progetti piccoli. Maggiori dettagli si possono trovare nell'articolo "«.
Puoi scaricare la versione di prova di PVS-Studio da noi su .
Se vuoi condividere quest'articolo con un pubblico di lingua inglese, ti prego di utilizzare il link alla traduzione: Sergey Larin.
Fonte: habr.com
