# Проверка rdesktop и xrdp с помощью анализатора PVS\-Studio Это второй обзор из цикла статей о проверке открытых программ для работы с протоколом RDP\. В ней мы рассмотрим клиент rdesktop и сервер xrdp\. ![0624_Rdesktop_xRDP_ru/image1.png](https://import.viva64.com/docx/blog/0624_Rdesktop_xRDP_ru/image1.png) В качестве инструмента для выявления ошибок использовался [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Это статический анализатор кода для языков C, C\+\+, C\# и Java, доступный на платформах Windows, Linux и macOS\. В статье представлены лишь те ошибки, которые показались мне интересными\. Впрочем, проекты маленькие, поэтому и ошибок было мало :\)\. **Примечание**\. Предыдущую статью о проверке проекта FreeRDP можно найти [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0617/)\. ## rdesktop [rdesktop](http://www.rdesktop.org/) — свободная реализация клиента RDP для UNIX\-based систем\. Его также можно использовать и под Windows, если собирать проект под Cygwin\. Лицензирован под GPLv3\. Этот клиент имеет большую популярность — он используется по умолчанию в ReactOS, также для него можно найти сторонние графические front\-end'ы\. Тем не менее, он довольно стар: первый релиз состоялся 4 апреля 2001 года — на момент написания статьи его возраст составляет 17 лет\. Как я уже отметил ранее, проект совсем маленький\. Он содержит примерно 30 тысяч строк кода, что немного странно, учитывая его возраст\. Для сравнения, FreeRDP содержит в себе 320 тысяч строк\. Вот вывод программы Cloc: ![0624_Rdesktop_xRDP_ru/image2.png](https://import.viva64.com/docx/blog/0624_Rdesktop_xRDP_ru/image2.png) ### Недостижимый код [V779](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. rdesktop\.c 1502 ```cpp 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_ \- этот фрагмент осуществляет очистку памяти\. Тем не менее, ошибка не представляет угрозы: вся выделенная память вычистится операционной системой после завершения работы программы\. ### Отсутствие обработки ошибок [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array underrun is possible\. The value of 'n' index could reach \-1\. rdesktop\.c 1872 ```cpp 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] = '\0'; // <= str_handle_lines(output, &rest, linehandler, data); } .... } ``` Фрагмент кода в этом случае осуществляет чтение из файла в буфер до тех пор, пока файл не закончится\. Однако обработка ошибок здесь отсутствует: если что\-то пойдет не так, то _read_ вернет \-1, и тогда произойдет выход за границы массива _output_\. ### Использование EOF в типе char [V739](https://pvs-studio.ru/ru/docs/warnings/v739/) 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 ```cpp 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** [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'write\_time' is always false\. disk\.c 805 ```cpp 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: в этом случае мы попадем в ветку _else_: переменная _mod\_time_ всегда будет равна 0 независимо от последующего условия\. * Одна из переменных равна 0: _mod\_time_ будет равно 0 \(при условии, что другая переменная имеет неотрицательное значение\), т\. к\._ MIN_ выберет наименьший из двух вариантов\. * Обе переменные не равны 0: выбираем минимальное значение\. При замене условия на _write\_time && change\_time_ поведение станет выглядеть корректным: * Одна или обе переменные не равны 0: выбираем ненулевое значение\. * Обе переменные не равны 0: выбираем минимальное значение\. **Фрагмент 2** [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always true\. Probably the '&&' operator should be used here\. disk\.c 1419 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v512/) A call of the 'sprintf' function will lead to overflow of the buffer 'fullpath'\. disk\.c 1257 ```cpp RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Get information for directory entry */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... } ``` При рассмотрении функции полностью станет понятно, что этот код не вызывает проблем\. Однако они могут возникнуть в будущем: одно неосторожное изменение, и мы получим переполнение буфера — _sprintf_ ничем не ограничен, поэтому при конкатенации путей мы можем выйти за границы массива\. Рекомендуется заметить этот вызов на _snprintf\(fullpath, PATH\_MAX, \.\.\.\.\)_\. ### Избыточное условие [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: add \> 0\. scard\.c 507 ```cpp 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 xrdp — реализация RDP сервера с открытым исходным кодом\. Проект разделен на 2 части: * xrdp — реализация протокола\. Распространяется под лицензией Apache 2\.0\. * xorgxrdp — набор драйверов Xorg для использования с xrdp\. Лицензия — X11 \(как MIT, но запрещает использование в рекламе\) Разработка проекта базируется на результатах rdesktop и FreeRDP\. Изначально для работы с графикой приходилось использовать отдельный VNC сервер, либо же специальный сервер X11 с поддержкой RDP \- X11rdp, однако с появлением xorgxrdp нужда в них отпала\. В этой статье мы не будем затрагивать xorgxrdp\. Проект xrdp, как и предыдущий, совсем небольшой и содержит примерно 80 тысяч строк\. ![0624_Rdesktop_xRDP_ru/image3.png](https://import.viva64.com/docx/blog/0624_Rdesktop_xRDP_ru/image3.png) ### Еще опечатки [V525](https://pvs-studio.ru/ru/docs/warnings/v525/) The code contains the collection of similar blocks\. Check items 'r', 'g', 'r' in lines 87, 88, 89\. rfxencode\_rgb\_to\_yuv\.c 87 ```cpp 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_, о чем нам тоже сообщил анализатор: [V525](https://pvs-studio.ru/ru/docs/warnings/v525/) The 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 ```cpp while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; } ``` ### Объявление массива [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The value of 'i \- 8' index could reach 129\. genkeymap\.c 142 ```cpp // 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 указан верный размер, поэтому выхода за границы нет\. Так что это просто недочет, который легко исправить\. ### Некорректное сравнение [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always false: \(cap\_len < 0\)\. xrdp\_caps\.c 616 ```cpp // 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_\. Проверка здесь не нужна, т\. к\. мы считываем переменную беззнакового типа и присваиваем результат переменной большего размера, поэтому переменная не может принять отрицательное значение\. ### Ненужные проверки [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: \(bpp \!\= 16\)\. libxrdp\.c 704 ```cpp 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; } .... } ``` Проверки на неравенство здесь не имеют смысла, т\. к\. у нас уже есть сравнение в начале\. Вполне вероятно, что это опечатка и разработчик хотел использовать оператор _\|\|_ чтобы отфильтровать неверные аргументы\. ## Заключение В ходе проверки не было выявлено серьезных ошибок, но нашлось много недочетов\. Тем не менее, эти проекты используются во многих системах, пусть и малы по своему объему\. В небольшом проекте не обязательно должно быть много ошибок, поэтому не стоит судить о работе анализатора только на малых проектах\. Подробнее об этом можно прочесть в статье "[Ощущения, которые подтвердились числами](https://pvs-studio.ru/ru/blog/posts/0158/)"\. Вы можете скачать пробную версию PVS\-Studio у нас на [сайте](https://pvs-studio.ru/ru/pvs-studio/download/)\.