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

    Изображение3

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

    Изображение1


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


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

    Изображение2


    Еще опечатки


    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
    PVS-Studio
    243,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 16

      +5
      Единорожек тут выглядит так, будто ему дали пинка под зад и он летит :D
        +1
        На rdesktop уже все забили, и чинить его, наверное, никто не будет.
        Сейчас актуален FreeRDP
        0
        А rrdtool не хотите проверить? Широко известная в узких кругах программа с 20 летней историей.
          0
          Чтобы избежать подобных ошибок результат работы функции fgetc следует хранить в переменной типа int

          Здравствуй, *опа, Новый Год :) Неужели до сих пор находятся личности, которые не смогли осилить fgetc?
            0
            А льзя ли сделать, чтоб конструкция «break; case» не вызывала срабатывания V622?
            switch ( value )
            {
                break; case 1: func1();
                break; case 2: func2();
                break; case 3: func3();
            }
            
              0
              Анализатор тут прав, зачем так писать? Возможно, для кого-то это даже красиво, но не более.
                +1
                Так писать можно не для красоты, а для того, чтоб не попадать в ситуацию «ой я опять забыл break». Или когда в кейсе был написан return, затем код изменился, но break добавить забыли. С тех пор, как я начал так писать в своих личных проектах, ни разу не возникло типичных для switch проблем.
                Зато буквально пару недель назад нашёл несколько проблем во вложенных свитчах при код-ревью одного проекта на работе. В своё время наши парни не захотели писать «break; case», типа выглядит плохо, и вот результат.
                  0
                  Предлагаю вписать в файл конфигурации диагностик (.pvsconfig) комментарий:
                  //-V:break;case:622

                  См. раздел "Подавление ложных предупреждений".
                    0
                    Да я так и сделал, просто хотел уточнить насчёт данного случая. Анализатор ругается на первый break, но ведь реально там ошибки нет.
                      0
                      Почему нет? Быть может должно было быть:

                      switch ( value )
                      {
                                 case 0: func0();
                          break; case 1: func1();
                          break; case 2: func2();
                          break; case 3: func3();
                          break;
                      }
                        0
                        Это просто переформатированный обычный switch. Если перед «case 0» добавить ещё один кейс, то у нас проблемы, потому что скорее всего break никто не добавит. Есть ли хоть какая-нибудь причина не писать break перед первым case?
                        Я сейчас серьезно спрашиваю, интересно разобраться.
                          +1
                          Я сейчас посмотрел ещё раз что делает V622 и понимаю, что эта штука именно для диагностики случая, когда потеряли первый case. Я сначала думал, что тут про любой кейс в свитче. Насколько я понимаю, есть другие диагностические сообщения для свитча, поэтому вопрос снимается, сорри за то, что побеспокоил.
                    +1
                    а как по мне, так фактически это Dead Code, этот джамп (если он не будет удалён компилятором) недостижим… Но я бы игнорировал в случае если это break — выглядит как хорошый оверрайд.
                      0
                      > Но я бы игнорировал в случае если это break — выглядит как хорошый оверрайд.
                      У нас 100500 исключений, чтобы не ругаться на определённые стили. Но всё предвидеть невозможно и не всегда разумно.
                        0
                        Конечно. Просто мне кажется что этот случай вполне можно сделать 100501 :) низкроприоритетная задача для студня на стажировке типа

                Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                Самое читаемое