Это второй обзор из цикла статей о проверке открытых программ для работы с протоколом RDP. В ней мы рассмотрим клиент rdesktop и сервер xrdp.
В качестве инструмента для выявления ошибок использовался PVS-Studio. Это статический анализатор кода для языков 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:
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.
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:
При замене условия на write_time && change_time поведение станет выглядеть корректным:
Фрагмент 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 — реализация RDP сервера с открытым исходным кодом. Проект разделен на 2 части:
Разработка проекта базируется на результатах 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 у нас на сайте.