Това е вторият преглед от цикъла статии за проверка на отворени програми за работа с протокола RDP. В него ще разгледаме клиента rdesktop и сървъра xrdp.
Като инструмент за откриване на грешки беше използван . Това е статичен анализатор на кода за езици C, C++, C# и Java, достъпен на платформите Windows, Linux и macOS.
В статията са представени само тези грешки, които ми се сториха интересни. Въпреки това, проектите са малки, така че и грешките бяха малко :).
Забележка. Предишната статия за проверка на проекта FreeRDP може да бъде намерена .
rdesktop
— свободна реализация на клиента RDP за UNIX-базираните системи. Може да се използва и под Windows, ако проектът бъде компилиран под Cygwin. Лицензиран е под GPLv3.
Този клиент е много популярен — той се използва по подразбиране в ReactOS, също така могат да се намерят и трети графични front-end решения за него. Независимо от това, той е доста стар: първият релиз беше на 4 април 2001 г. — към момента на написването на статията, възрастта му е 17 години.
Както вече споменах, проектът е малък. Съдържа около 30 хиляди реда код, което е малко странно, предвид възрастта му. За сравнение, FreeRDP съдържа 320 хиляди реда. Ето изходът от програмата Cloc:
Недостижим код
Unreachable code detected. It is possible that an error is present. 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);
}Грешката ни посреща веднага в функцията main: виждаме кода след оператора return — този фрагмент изпълнява почистване на паметта. Въпреки това, грешката не представлява заплаха: всяка заделена памет ще бъде почистена от операционната система след завършване на работата на програмата.
Липса на обработка на грешки
Array underrun is possible. The value of ‘n’ index could reach -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);
}
....
}Фрагментът от кода в този случай чете от файл в буфер, докато файлът не свърши. Въпреки това, обработка на грешки отсъства: ако нещо не върви както трябва, read той ще върне -1, и тогава ще се получи извън границите на масива output.
Използване на EOF в тип char
EOF should not be compared with a value of the ‘char’ type. The ‘(c = fgetc(fp))’ should be of the ‘int’ type. 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++;
}
....
}Тук виждаме неправилна обработка при достигане на края на файла: ако fgetc върне символ, чийто код е 0xFF, то той ще бъде възприет като край на файла (EOF).
EOF това е константа, обикновено дефинирана като -1. Например, в кодировката CP1251 последната буква на руския алфавит има код 0xFF, който съответства на числото -1, ако говорим за променлива от тип char. Получава се, че символ 0xFF, подобно на EOF (-1) се възприема като край на файла. За да избегнем подобни грешки, резултатът от функцията fgetc трябва да се съхранява в променлива от тип int.
Типографски грешки
Фрагмент 1
Изразът ‘write_time’ винаги е false. 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;
....
}Може би авторът на този код е объркал || и && в условието. Нека разгледаме възможните стойности на write_time и change_time:
- И двете променливи са равни на 0: в този случай ще попаднем в клона иначе: променливата mod_time винаги ще бъде равна на 0 независимо от последващото условие.
- Една от променливите е равна на 0: mod_time ще бъде равна на 0 (при условие, че другата променлива има ненегативна стойност), тъй като МИН ще избере най-малкия от двата варианта.
- И двете променливи не са равни на 0: избираме минималната стойност.
При замяна на условието на write_time && change_time поведението ще изглежда коректно:
- Една или и двете променливи не са равни на 0: избираме ненулевото значение.
- И двете променливи не са равни на 0: избираме минималната стойност.
Фрагмент 2
Изразът винаги е истинен. Вероятно операторът ‘&&’ трябва да се използва тук. 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;
....
}Явно, тук също са объркани операторите: променливата не може да приеме едновременно стойност 20 и 9. || и &&, или == и !=Неограничено копиране на низ
V512
RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Вземете информация за директорията */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }
При разглеждане на функцията напълно ще стане ясно, че този код не предизвиква проблеми. Въпреки това, те могат да възникнат в бъдеще: едно небрежно изменение, и ще получим преливане на буфера —без ограничения, следователно при конкатениране на пътища можем да излезем извън границите на масива. Препоръчително е да се забележи това извикване на sprintf snprintf(fullpath, PATH_MAX, ….) Излишно условие.
Част от условното изразяване винаги е истина: добавете > 0. scard.c 507
A part of conditional expression is always true: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add 0)
{
....
}
}Проверка add > 0 тук не е нужно: променливата винаги ще е по-голяма от нула, тъй като read % 4 ще върне остатъка от делението, а той никога няма да е равен на 4.
xrdp
— имплементация на RDP сървър с отворен код. Проектът е разделен на 2 части:
- xrdp — имплементация на протокола. Разпространява се под лиценз Apache 2.0.
- xorgxrdp — набор от драйвъри Xorg за използване с xrdp. Лиценз — X11 (като MIT, но забранява използването в реклама)
Разработката на проекта се основава на резултатите от rdesktop и FreeRDP. Поначално за работа с графиката е трябвало да се използва отделен VNC сървър или специален X11 сървър с поддръжка на RDP — X11rdp, но с появата на xorgxrdp нуждата от тях отпадна.
В тази статия няма да обсъждаме xorgxrdp.
Проектът xrdp, подобно на предишния, е сравнително малък и съдържа около 80 хиляди реда.
Още печатни грешки
Кодът съдържа колекция от подобни блокове. Проверете елементите ‘r’, ‘g’, ‘r’ в редове 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++;
}
....
}
....
}Този код е взет от библиотеката librfxcodec, реализираща кодек jpeg2000 за работа с RemoteFX. Тук очевидно, кабелите на графичните данни са обърнати — вместо „синьо“ се записва „червено“. Такава грешка, вероятно, е възникнала в резултат на copy-paste.
Тази съща проблема съществува и в подобна функция rfx_encode_format_argb, за което ни информира и анализаторът:
Кодът съдържа колекция от подобни блокове. Проверете елементите ‘a’, ‘r’, ‘g’, ‘r’ в редове 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++;
}Декларация на масив
Възможно е презареждане на масива. Стойността на индекса ‘i - 8’ може да достигне 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];
....
}
....
}Декларацията и определението на масива в тези два файла не са съвместими - размерът се различава с 1. Въпреки това не възникват грешки - в файла evdev-map.c е посочен правилният размер, затова няма извън пределите. Така че това е просто пропуск, който лесно може да бъде поправен.
Некоректно сравнение
Част от условното изразяване винаги е невярно: (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))
{
....
}
....
}В функцията се извършва четене на променлива от тип unsigned short в променлива от тип int. Проверка тук не е необходима, тъй като четем променлива без знак и присвояваме резултата на променлива с по-голям размер, следователно, променливата не може да вземе отрицателна стойност.
Излишни проверки
Част от условното изражение винаги е вярна: (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: грешка");
return 1;
}
....
}Проверките за неравенство тук нямат смисъл, тъй като вече имаме сравнение в началото. Възможно е да е печатна грешка и разработчикът е искал да използва оператора || за да филтрира неверни аргументи.
Заключение
При проверката не бяха открити сериозни грешки, но бяха намерени много недостатъци. Въпреки това, тези проекти се използват в много системи, макар и малки по обем. В малък проект не е задължително да има много грешки, така че не бива да оценявате работата на анализатора само на базата на малки проекти. Повече за това можете да прочетете в статията ««.
Можете да изтеглите пробна версия на PVS-Studio от нас на .
Ако искате да споделите тази статия с англоезичната аудитория, моля, използвайте линка към превода: Сергей Ларин.
Източник: habr.com
