Це другий огляд із циклу статей про перевірку відкритих програм для роботи з протоколом RDP. У ній ми розглянемо клієнт rdesktop та сервер xrdp.
Як інструмент виявлення помилок використовувався . Це статичний аналізатор коду мов C, C++, C# і Java, доступний на платформах Windows, Linux і macOS.
У статті представлені лише ті помилки, які видалися мені цікавими. Втім, проекти малі, тому й помилок було мало :).
Примітка. Попередню статтю про перевірку проекту FreeRDP можна знайти .
rdesktop
- Вільна реалізація клієнта RDP для UNIX-based систем. Його також можна використовувати під 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);
}Помилка нас зустрічає відразу ж у функції основний: ми бачимо код після оператора повертати - Цей фрагмент здійснює очищення пам'яті. Проте помилка не становить загрози: вся виділена пам'ять вичиститься операційною системою після завершення роботи програми.
Відсутність обробки помилок
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);
}
....
}Фрагмент коду в цьому випадку здійснює читання з файлу в буфер, поки файл не закінчиться. Однак, обробка помилок тут відсутня: якщо щось піде не так, то зчитування поверне -1, і тоді станеться вихід за межі масиву вихід.
Використання EOF у типі char
EOF не може бути compared with value of 'char' типу. The '(c = fgetc(fp))' should be of the '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++;
}
....
}Тут ми бачимо некоректну обробку досягнення кінця файлу: якщо fget тощо поверне символ, код якого дорівнює 0xFF, він буде сприйнятий як кінець файлу (EOF).
EOF це константа, визначена зазвичай як -1. Наприклад, у кодуванні CP1251 остання літера російського алфавіту має код 0xFF, що відповідає числу -1, якщо ми говоримо про змінну типу бак. Виходить, що символ 0xFF, як і EOF (-1) сприймається як кінець файлу. Щоб уникнути подібних помилок результат роботи функції fget тощо слід зберігати в змінній типу Int.
помилки
Фрагмент 1
Expression 'write_time' is always 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 (за умови, що інша змінна має невід'ємне значення), т. до. MIN вибере найменший із двох варіантів.
- Обидві змінні не дорівнюють 0: вибираємо мінімальне значення.
При заміні умови на write_time && change_time поведінка виглядатиме коректною:
- Одна або обидві змінні не дорівнюють 0: вибираємо ненульове значення.
- Обидві змінні не дорівнюють 0: вибираємо мінімальне значення.
Фрагмент 2
Expression is always true. Probably '&&' operator повинен бути використаний тут. 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.
Необмежене копіювання рядка
За допомогою 'sprintf' функція буде керувати overflow of buffer '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);
....
}При розгляді функції цілком зрозуміло, що цей код не викликає проблем. Однак вони можуть виникнути в майбутньому: одна необережна зміна, і ми отримаємо переповнення буфера. спринт нічим не обмежений, тому при конкатенації шляхів ми можемо вийти за межі масиву. Рекомендується помітити цей виклик на snprintf(fullpath, PATH_MAX, ….).
Надмірна умова
Apart 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 < 4 && 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 тисяч рядків.
Ще друкарські помилки
Code contains the collection of similar blocks. Check items 'r', 'g', 'r' in lines 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, Про що нам теж повідомив аналізатор:
Code contains the collection of similar blocks. Check items 'a', 'r', 'g', 'r' in lines 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 overrun is possible. The value of 'i - 8' index could reach 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 вказано правильний розмір, тому виходу за межі немає. Так що це просто недолік, який просто виправити.
Некоректне порівняння
Apart conditional expression is always false: (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. Перевірка тут не потрібна, тому що ми зчитуємо змінну беззнакового типу і привласнюємо результат змінної більшого розміру, тому змінна не може набути негативного значення.
Непотрібні перевірки
Apart of conditional expression is always true: (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.
Джерело: habr.com