Това е вторият преглед от поредица от статии за тестване на програми с отворен код за работа с протокола RDP. В него ще разгледаме rdesktop клиента и xrdp сървъра.
Използва се като инструмент за идентифициране на грешки . Това е анализатор на статичен код за езици C, C++, C# и Java, наличен на платформи Windows, Linux и macOS.
Статията представя само онези грешки, които ми се сториха интересни. Проектите обаче са малки, така че грешките бяха малко :).
Внимание. Можете да намерите предишна статия за проверката на проекта FreeRDP .
rdesktop
— безплатно внедряване на RDP клиент за UNIX-базирани системи. Може да се използва и под Windows, ако изградите проекта под Cygwin. Лицензирано под GPLv3.
Този клиент е много популярен - използва се по подразбиране в ReactOS и можете също да намерите графични интерфейси на трети страни за него. Той обаче е доста стар: първото му освобождаване се състоя на 4 април 2001 г. - към момента на писане той е на 17 години.
Както отбелязах по-рано, проектът е много малък. Съдържа приблизително 30 хиляди реда код, което е малко странно предвид възрастта му. За сравнение FreeRDP съдържа 320 хиляди реда. Ето резултата от програмата Cloc:
Недостъпен код
Открит е недостъпен код. Възможно е да има грешка. 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);
}Грешката ни среща веднага във функцията основен: виждаме кода след оператора връщане — този фрагмент извършва почистване на паметта. Грешката обаче не представлява заплаха: цялата разпределена памет ще бъде изчистена от операционната система, след като програмата излезе.
Няма обработка на грешки
Възможна е недостатъчност на масива. Стойността на индекса '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);
}
....
}Кодовият фрагмент в този случай чете от файла в буфер, докато файлът приключи. Тук обаче няма обработка на грешки: ако нещо се обърка, тогава чета ще върне -1 и след това масивът ще бъде препълнен продукция.
Използване на EOF в тип char
EOF не трябва да се сравнява със стойност от типа "char". „(c = fgetc(fp))“ трябва да е от тип „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++;
}
....
}Тук виждаме неправилно обработване на достигане до края на файла: if fget и т.н връща символ, чийто код е 0xFF, той ще бъде интерпретиран като край на файла (EOF).
EOF това е константа, обикновено дефинирана като -1. Например, в кодирането CP1251, последната буква от руската азбука има код 0xFF, който съответства на числото -1, ако говорим за променлива като овъглявам. Оказва се, че символът 0xFF, като EOF (-1) се интерпретира като край на файла. За да се избегнат подобни грешки, резултатът от функцията е fget и т.н трябва да се съхранява в променлива като 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; // <=
....
}Може би авторът на този код се е объркал || и && в състояние. Нека разгледаме възможните опции за стойности време_на_запис и време_промяна:
- И двете променливи са равни на 0: в този случай ще се окажем в клон още: променлива mod_time винаги ще бъде 0, независимо от последващото условие.
- Една от променливите е 0: mod_time ще бъде равно на 0 (при условие, че другата променлива има неотрицателна стойност), т.к MIN ще избере по-малката от двете опции.
- И двете променливи не са равни на 0: изберете минималната стойност.
При замяна на условието с време за запис && време за промяна поведението ще изглежда правилно:
- Една или и двете променливи не са равни на 0: изберете различна от нула стойност.
- И двете променливи не са равни на 0: изберете минималната стойност.
Фрагмент 2
Изразът винаги е верен. Вероятно операторът '&&' трябва да се използва тук. диск.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 едновременно.
Неограничено копиране на редове
Извикването на функцията 'sprintf' ще доведе до препълване на буфера 'fullpath'. диск.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);
....
}Когато разгледате функцията изцяло, ще стане ясно, че този код не създава проблеми. Те обаче могат да възникнат в бъдеще: една невнимателна промяна и ще получим препълване на буфера - спринт не е ограничен от нищо, така че при конкатениране на пътища можем да излезем извън границите на масива. Препоръчително е да забележите това обаждане на snprintf(пълен път, PATH_MAX, ….).
Излишно състояние
Част от условния израз винаги е вярна: add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка добави > 0 тук няма нужда: променливата винаги ще бъде по-голяма от нула, защото прочетете % 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. Тук очевидно каналите за графични данни са смесени - вместо „синия“ цвят се записва „червен“. Тази грешка най-вероятно се е появила в резултат на копиране и поставяне.
Същият проблем възникна в подобна функция 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))
{
....
}
....
}Функцията чете променлива тип неподписано кратко в променлива като 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: error");
return 1;
}
....
}Проверките за неравенство тук нямат смисъл, тъй като вече имаме сравнение в началото. Вероятно това е печатна грешка и разработчикът е искал да използва оператора || за филтриране на невалидни аргументи.
Заключение
По време на одита не са открити сериозни грешки, но са открити много пропуски. Въпреки това, тези проекти се използват в много системи, макар и с малък обхват. Един малък проект не е задължително да има много грешки, така че не трябва да преценявате ефективността на анализатора само за малки проекти. Повече за това можете да прочетете в статията “".
Можете да изтеглите пробна версия на PVS-Studio от нас .
Ако искате да споделите тази статия с англоезична аудитория, моля, използвайте връзката за превод: Sergey Larin.
Източник: www.habr.com