To druga recenzja z serii artykułów na temat testowania programów open source pod kątem pracy z protokołem RDP. Przyjrzymy się w nim klientowi rdesktop i serwerowi xrdp.
Używany jako narzędzie do identyfikacji błędów . Jest to statyczny analizator kodu dla języków C, C++, C# i Java, dostępny na platformach Windows, Linux i macOS.
W artykule przedstawiono jedynie te błędy, które wydały mi się interesujące. Projekty są jednak małe, więc błędów było niewiele :).
Operacja. Poprzedni artykuł na temat weryfikacji projektu FreeRDP można znaleźć .
rpulpit
— bezpłatna implementacja klienta RDP dla systemów opartych na systemie UNIX. Można go również używać w systemie Windows, jeśli zbudujesz projekt w Cygwin. Licencja na licencji GPLv3.
Klient ten jest bardzo popularny - jest domyślnie używany w ReactOS i można dla niego znaleźć także graficzne interfejsy graficzne innych firm. Jest jednak dość stary: jego pierwsze wydanie odbyło się 4 kwietnia 2001 roku - w chwili pisania tego tekstu ma 17 lat.
Jak wspomniałem wcześniej, projekt jest dość mały. Zawiera około 30 tysięcy linii kodu, co jest nieco dziwne, biorąc pod uwagę jego wiek. Dla porównania FreeRDP zawiera 320 tys. linii. Oto wynik działania programu Cloc:
Nieosiągalny kod
Wykryto niedostępny kod. Możliwe, że wystąpił błąd. 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);
}Błąd spotyka nas natychmiast w funkcji główny: widzimy kod pojawiający się po operatorze powrót — ten fragment wykonuje czyszczenie pamięci. Błąd nie stanowi jednak zagrożenia: cała przydzielona pamięć zostanie wyczyszczona przez system operacyjny po wyjściu programu.
Brak obsługi błędów
Możliwe jest niedopełnienie układu. Wartość indeksu „n” może osiągnąć -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);
}
....
}W tym przypadku fragment kodu wczytuje plik do bufora aż do zakończenia pliku. Nie ma tu jednak żadnej obsługi błędów: jeśli coś pójdzie nie tak, to wtedy czytać zwróci -1, a wtedy tablica zostanie przepełniona wydajność.
Używanie EOF w typie char
EOF nie należy porównywać z wartością typu „char”. Wartość „(c = fgetc(fp))” powinna być 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++;
}
....
}Tutaj widzimy niepoprawną obsługę dotarcia do końca pliku: if fgetc zwraca znak o kodzie 0xFF, będzie to interpretowane jako koniec pliku (EOF).
EOF jest to stała, zwykle definiowana jako -1. Przykładowo w kodowaniu CP1251 ostatnia litera alfabetu rosyjskiego ma kod 0xFF, co odpowiada cyfrze -1 jeśli mówimy o zmiennej typu zwęglać. Okazuje się, że symbol 0xFF, np EOF (-1) jest interpretowane jako koniec pliku. Aby uniknąć takich błędów, wynikiem funkcji jest fgetc należy zapisać w zmiennej np int.
Literówki
Fragment 1
Wyrażenie „czas_zapisu” jest zawsze fałszywe. dysk.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; // <=
....
}Być może autor tego kodu się mylił || и && w stanie. Rozważmy możliwe opcje wartości czas_zapisu и zmienić czas:
- Obie zmienne są równe 0: w tym przypadku znajdziemy się w gałęzi więcej: zmienny mod_time zawsze będzie wynosić 0, niezależnie od kolejnego warunku.
- Jedną ze zmiennych jest 0: mod_time będzie równa 0 (pod warunkiem, że druga zmienna ma wartość nieujemną), ponieważ MIN wybierze mniejszą z dwóch opcji.
- Obie zmienne nie są równe 0: wybierz wartość minimalną.
Podczas zastępowania warunku przez czas_zapisu && czas_zmiany zachowanie będzie wyglądało poprawnie:
- Jedna lub obie zmienne nie są równe 0: wybierz wartość różną od zera.
- Obie zmienne nie są równe 0: wybierz wartość minimalną.
Fragment 2
Wyrażenie jest zawsze prawdziwe. Prawdopodobnie należy tutaj użyć operatora „&&”. dysk.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;
....
}Najwyraźniej operatorzy też tu się pomylili || и &&Lub == и !=: Zmienna nie może mieć jednocześnie wartości 20 i 9.
Nieograniczone kopiowanie linii
Wywołanie funkcji „sprintf” spowoduje przepełnienie bufora „fullpath”. dysk.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);
....
}Kiedy spojrzysz na funkcję w całości, stanie się jasne, że ten kod nie powoduje problemów. Mogą się one jednak pojawić w przyszłości: jedna nieostrożna zmiana i otrzymamy przepełnienie bufora - sprint nie jest niczym ograniczona, zatem konkatenując ścieżki możemy wyjść poza granice tablicy. Zaleca się odnotowanie tego połączenia snprintf(pełna ścieżka, PATH_MAX, ….).
Stan zbędny
Część wyrażenia warunkowego jest zawsze prawdziwa: add > 0. Scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка dodaj > 0 nie ma tu takiej potrzeby: zmienna będzie zawsze większa od zera, ponieważ przeczytaj % 4 zwróci resztę dzielenia, ale nigdy nie będzie równa 4.
xrdp
— wdrożenie serwera RDP z otwartym kodem źródłowym. Projekt podzielony jest na 2 części:
- xrdp - implementacja protokołu. Dystrybuowany na licencji Apache 2.0.
- xorgxrdp - Zestaw sterowników Xorg do użytku z xrdp. Licencja - X11 (jak MIT, ale zabrania wykorzystania w reklamie)
Rozwój projektu opiera się na wynikach rdesktop i FreeRDP. Początkowo do pracy z grafiką trzeba było używać osobnego serwera VNC lub specjalnego serwera X11 z obsługą RDP - X11rdp, jednak wraz z pojawieniem się xorgxrdp ich potrzeba zniknęła.
W tym artykule nie będziemy omawiać xorgxrdp.
Projekt xrdp, podobnie jak poprzedni, jest bardzo mały i zawiera około 80 tysięcy linii.
Więcej literówek
Kod zawiera zbiór podobnych bloków. Sprawdź pozycje „r”, „g”, „r” w liniach 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++;
}
....
}
....
}Ten kod został pobrany z biblioteki librfxcodec, która implementuje kodek jpeg2000 dla RemoteFX. Tutaj najwyraźniej kanały danych graficznych są pomieszane - zamiast „niebieskiego” koloru rejestrowany jest „czerwony”. Ten błąd najprawdopodobniej pojawił się w wyniku kopiowania i wklejania.
Ten sam problem wystąpił w podobnej funkcji rfx_encode_format_argb, o czym analizator powiedział nam również:
Kod zawiera zbiór podobnych bloków. Sprawdź pozycje „a”, „r”, „g”, „r” w liniach 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++;
}Deklaracja tablicy
Możliwe jest przekroczenie tablicy. Wartość indeksu „i — 8” może osiągnąć 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];
....
}
....
}Deklaracja i definicja tablicy w tych dwóch plikach są niekompatybilne - rozmiar różni się o 1. Nie pojawiają się jednak żadne błędy - prawidłowy rozmiar jest podany w pliku evdev-map.c, więc nie ma żadnych przekroczeń. Jest to więc tylko błąd, który można łatwo naprawić.
Niepoprawne porównanie
Część wyrażenia warunkowego jest zawsze fałszywa: (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))
{
....
}
....
}Funkcja odczytuje zmienną typu bez znaku krótkie w zmienną np int. Sprawdzanie nie jest tutaj potrzebne, ponieważ czytamy zmienną bez znaku i przypisujemy wynik do większej zmiennej, więc zmienna nie może przyjąć wartości ujemnej.
Niepotrzebne kontrole
Część wyrażenia warunkowego jest zawsze prawdziwa: (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;
}
....
}Sprawdzanie nierówności nie ma tutaj sensu, ponieważ porównanie mamy już na początku. Prawdopodobnie jest to literówka i programista chciał użyć operatora || aby odfiltrować nieprawidłowe argumenty.
wniosek
Podczas kontroli nie stwierdzono żadnych poważnych błędów, stwierdzono natomiast wiele uchybień. Jednak projekty te są stosowane w wielu systemach, choć o niewielkim zakresie. Mały projekt niekoniecznie musi zawierać wiele błędów, dlatego nie należy oceniać wydajności analizatora tylko na małych projektach. Więcej na ten temat przeczytasz w artykule „".
Możesz pobrać od nas wersję próbną PVS-Studio .
Jeśli chcesz udostępnić ten artykuł anglojęzycznej publiczności, skorzystaj z linku do tłumaczenia: Sergey Larin.
Źródło: www.habr.com