Это второй обзор из цикла статей о проверке открытых программ для работы с протоколом RDP. В ней мы рассмотрим клиент rdesktop и сервер xrdp.
В качестве инструмента для выявления ошибок использовался PVS-Studio. Это статический анализатор кода для языков C, C++, C# и Java, доступный на платформах Windows, Linux и macOS.
В статье представлены лишь те ошибки, которые показались мне интересными. Впрочем, проекты маленькие, поэтому и ошибок было мало :).
Примечание. Предыдущую статью о проверке проекта FreeRDP можно найти здесь.
rdesktop
rdesktop — свободная реализация клиента RDP для UNIX-based систем. Его также можно использовать и под Windows, если собирать проект под Cygwin. Лицензирован под GPLv3.
Этот клиент имеет большую популярность — он используется по умолчанию в ReactOS, также для него можно найти сторонние графические front-end'ы. Тем не менее, он довольно стар: первый релиз состоялся 4 апреля 2001 года — на момент написания статьи его возраст составляет 17 лет.
Как я уже отметил ранее, проект совсем маленький. Он содержит примерно 30 тысяч строк кода, что немного странно, учитывая его возраст. Для сравнения, FreeRDP содержит в себе 320 тысяч строк. Вот вывод программы Cloc:
Недостижимый код
V779 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 — этот фрагмент осуществляет очистку памяти. Тем не менее, ошибка не представляет угрозы: вся выделенная память вычистится операционной системой после завершения работы программы.
Отсутствие обработки ошибок
V557 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] = '\0'; // <= str_handle_lines(output, &rest, linehandler, data); } .... }
Фрагмент кода в этом случае осуществляет чтение из файла в буфер до тех пор, пока файл не закончится. Однако обработка ошибок здесь отсутствует: если что-то пойдет не так, то read вернет -1, и тогда произойдет выход за границы массива output.
Использование EOF в типе char
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
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 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: в этом случае мы попадем в ветку else: переменная mod_time всегда будет равна 0 независимо от последующего условия.
- Одна из переменных равна 0: mod_time будет равно 0 (при условии, что другая переменная имеет неотрицательное значение), т. к. MIN выберет наименьший из двух вариантов.
- Обе переменные не равны 0: выбираем минимальное значение.
При замене условия на write_time && change_time поведение станет выглядеть корректным:
- Одна или обе переменные не равны 0: выбираем ненулевое значение.
- Обе переменные не равны 0: выбираем минимальное значение.
Фрагмент 2
V547 Expression is always true. Probably the '&&' operator should be used here. 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 A call of the 'sprintf' function will lead to overflow of the 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); .... }
При рассмотрении функции полностью станет понятно, что этот код не вызывает проблем. Однако они могут возникнуть в будущем: одно неосторожное изменение, и мы получим переполнение буфера — sprintf ничем не ограничен, поэтому при конкатенации путей мы можем выйти за границы массива. Рекомендуется заметить этот вызов на snprintf(fullpath, PATH_MAX, ....).
Избыточное условие
V560 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 < 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 тысяч строк.
Еще опечатки
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
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 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
while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; }
Объявление массива
V557 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 указан верный размер, поэтому выхода за границы нет. Так что это просто недочет, который легко исправить.
Некорректное сравнение
V560 A part of 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)) { .... } .... }
В функции происходит чтение переменной типа unsigned short в переменную типа int. Проверка здесь не нужна, т. к. мы считываем переменную беззнакового типа и присваиваем результат переменной большего размера, поэтому переменная не может принять отрицательное значение.
Ненужные проверки
V560 A part 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. Checking rdesktop and xrdp with PVS-Studio