to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Проверка rdesktop и xrdp с помощью анал…

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

12 Апр 2019

Это второй обзор из цикла статей о проверке открытых программ для работы с протоколом RDP. В ней мы рассмотрим клиент rdesktop и сервер xrdp.

0624_Rdesktop_xRDP_ru/image1.png

В качестве инструмента для выявления ошибок использовался 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:

0624_Rdesktop_xRDP_ru/image2.png

Недостижимый код

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 тысяч строк.

0624_Rdesktop_xRDP_ru/image3.png

Еще опечатки

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 у нас на сайте.

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо