Сергей Ларин

12 Апр 2019

Теги:

#Cpp

Проверка rdesktop и xrdp с помощью анализатора PVS-Studio

12 Апр 2019

Автор: Сергей Ларин

rdesktop
xrdp
Заключение

Это второй обзор из цикла статей о проверке открытых программ для работы с протоколом 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 у нас на сайте.

#Cpp