Проверка проекта LibrePCB с помощью PVS-Studio внутри Docker контейнера

    PVS-Studio and Docker Container

    Это классическая статья о том, как наша команда проверила открытый проект LibrePCB с помощью статического анализатора кода PVS-Studio. Однако статья интересна тем, что проверка осуществлялась внутри Docker контейнера. Если вы использует контейнеры, то надеемся, что статья продемонстрирует ещё один простой способ встроить анализатор в процесс разработки.

    LibrePCB


    LibrePCB — это свободное ПО для проектирования электронных схем и печатных плат. Код программы написан на языке C++, а для построения графического интерфейса используется Qt5. Недавно состоялся первый официальный релиз этого приложения, ознаменовавший собою стабилизацию собственного формата файлов (*.lp, *.lplib). Бинарные пакеты подготовлены для Linux, macOS и Windows.

    LibrePCB


    LibrePCB — это маленький проект, содержащий всего около 300 000 непустых строк кода на языке C и C++. При этом 25% непустых строк — это комментарии. Кстати, это большой процент для комментариев. Скорее всего, это связано с тем, что в проекте много маленьких файлов, существенную часть которых занимают заголовочные комментарии с информаций о проекте и лицензии. Код на сайте GitHub: LibrePCB.

    Проект показался нам интересным, и мы решили проверить его. А вот результаты проверки оказались уже не такими интересными. Да, нашлись настоящие ошибки. Но не было чего-то особенного, о чём непременно надо рассказать читателям наших статей. Возможно, мы бы не стали писать статью и ограничились отправкой найденных ошибок разработчикам проекта. Однако интересным моментом стало то, что проект был проверен внутри Docker образа, и поэтому мы решили, что написать статью всё же стоит.

    Docker


    Docker — программное обеспечение для автоматизации развёртывания и управления приложениями в среде виртуализации на уровне операционной системы. Оно позволяет «упаковать» приложение со всем его окружением и зависимостями в контейнер. Хотя этой технологии около пяти лет и многие компании давно внедрили Docker в инфраструктуры своих проектов, в мире open source это было не очень заметно до недавнего времени.

    Наша компания очень плотно работает с open source проектами, проверяя исходный код с помощью собственного статического анализатора PVS-Studio. На данный момент проверено более 300 проектов. Самым сложным в этой деятельности всегда была компиляция чужих проектов, но внедрение Docker-контейнеров сильно упростило этот процесс.

    Первый опыт проверки open source проекта в Docker был с Azure Service Fabric. Там разработчики сделали монтирование каталога исходных файлов к контейнеру и интеграция анализатора ограничилась редактированием одного из скриптов, выполняющихся в контейнере:

    diff --git a/src/build.sh b/src/build.sh
    index 290c57d..2a286dc 100755
    --- a/src/build.sh
    +++ b/src/build.sh
    @@ -193,6 +193,9 @@ BuildDir()
         
         cd ${ProjBinRoot}/build.${DirName}
     
    +    pvs-studio-analyzer analyze --cfg /src/PVS-Studio.cfg \
    +      -o ./service-fabric-pvs.log -j4
    +
         if [ "false" = ${SkipBuild} ]; then
             if (( $NumProc <= 0 )); then
                 NumProc=$(($(getconf _NPROCESSORS_ONLN)+0))

    Отличие проекта LibrePCB заключается в том, что они сразу предоставили Dockerfile для сборки образа и проекта в нём. Это оказалось ещё более удобным для нас. Вот часть Docker-файла, которая нам интересна:

    FROM ubuntu:14.04
    
    # install packages
    RUN DEBIAN_FRONTEND=noninteractive \
         apt-get -q update \
      && apt-get -qy upgrade \
      && apt-get -qy install git g++ qt5-default qttools5-dev-tools qt5-doc \
         qtcreator libglu1-mesa-dev dia \
      && apt-get clean
    
    # checkout librepcb
    RUN git clone --recursive https://..../LibrePCB.git /opt/LibrePCB \
      && cd /opt/LibrePCB
    
    ....
    
    # build and install librepcb
    RUN /opt/LibrePCB/dev/docker/make_librepcb.sh
    
    ....

    Компиляцию и установку проекта при сборке образа мы делать не будем. Таким образом, нами был собран образ, в котором автор проекта гарантирует успешную сборку проекта.

    После запуска контейнера был установлен анализатор и выполнены следующие команды для сборки и анализа проекта:

    cd /opt/LibrePCB
    mkdir build && cd build
    qmake -r ../librepcb.pro
    pvs-studio-analyzer trace -- make -j2
    pvs-studio-analyzer analyze -l /mnt/Share/PVS-Studio.lic -r /opt/LibrePCB \
      -o /opt/LibrePCB/LibrePCB.log -v -j4
    cp -R -L -a /opt/LibrePCB /mnt/Share

    Кстати, все действия выполнялись в Windows 10. Очень радует, что разработчики всех популярных операционных систем тоже развиваются в этом направлении. К сожалению, контейнеры с Windows не так удобны. Особенно из-за невозможности также легко устанавливать софт.

    Найденные ошибки


    Теперь классический раздел, содержащий описание найденных нами ошибок с помощью статического анализатора кода PVS-Studio. Кстати, пользуясь случаяем, хочу напомнить, что в последнее время мы развиваем анализатор в сторону поддержки анализа кода для встраиваемых систем. Вот пара статей, которые некоторые наши читатели могли пропустить:

    1. В PVS-Studio появилась поддержка GNU Arm Embedded Toolchain;
    2. PVS-Studio: поддержка стандартов кодирования MISRA C и MISRA C++.

    Опечатки


    SymbolPreviewGraphicsItem::SymbolPreviewGraphicsItem(
        const IF_GraphicsLayerProvider& layerProvider,
        const QStringList& localeOrder, const Symbol& symbol, const Component* cmp,
        const tl::optional<Uuid>& symbVarUuid,
        const tl::optional<Uuid>& symbVarItemUuid) noexcept
    {
      if (mComponent && symbVarUuid && symbVarItemUuid)
      ....
      if (mComponent && symbVarItemUuid && symbVarItemUuid)      // <=
      ....
    }

    Предупреждение PVS-Studio: V501 CWE-571 There are identical sub-expressions 'symbVarItemUuid' to the left and to the right of the '&&' operator. symbolpreviewgraphicsitem.cpp 74

    Классическая опечатка: два раза подряд проверяется переменная symbVarItemUuid. Выше находится аналогичная проверка, и, глядя на неё, становится ясно, что второй проверяемой переменной должна быть symbVarUuid.

    Следующий фрагмент кода с опечаткой:

    void Clipper::DoMaxima(TEdge *e)
    {
      ....
      if (e->OutIdx >= 0) 
      {
        AddOutPt(e, e->Top);
        e->OutIdx = Unassigned;
      }
      DeleteFromAEL(e);
    
      if (eMaxPair->OutIdx >= 0)
      {
        AddOutPt(eMaxPair, e->Top);         // <=
        eMaxPair->OutIdx = Unassigned;
      }
      DeleteFromAEL(eMaxPair);
      ....
    }

    Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'eMaxPair' variable should be used instead of 'e'. clipper.cpp 2999

    Скорее всего, код писался с помощью Copy-Paste. Из-за недосмотра во втором блоке текста забыли заменить e->Top на eMaxPair->Top.

    Лишние проверки


    static int
    rndr_emphasis(hoedown_buffer *ob, const hoedown_buffer *content,
                  const hoedown_renderer_data *data)
    {
      if (!content || !content->size) return 0;
      HOEDOWN_BUFPUTSL(ob, "<em>");
      if (content) hoedown_buffer_put(ob, content->data, content->size);
      HOEDOWN_BUFPUTSL(ob, "</em>");
      return 1;
    }

    Предупреждение PVS-Studio: V547 CWE-571 Expression 'content' is always true. html.c 162

    Это скорее всё-таки не ошибка, а просто избыточный код. Не нужно повторно проверять указатель content. Если он нулевой, то функция сразу завершает свою работу.

    Аналогичная ситуация:

    void Clipper::DoMaxima(TEdge *e)
    {
      ....
      else if( e->OutIdx >= 0 && eMaxPair->OutIdx >= 0 )
      {
        if (e->OutIdx >= 0) AddLocalMaxPoly(e, eMaxPair, e->Top);
        DeleteFromAEL(e);
        DeleteFromAEL(eMaxPair);
      }
      ....
    }

    Предупреждение PVS-Studio: V547 CWE-571 Expression 'e->OutIdx >= 0' is always true. clipper.cpp 2983

    Повторная проверка (e->OutIdx >= 0) не имеет смысла. Впрочем, возможно это ошибка. Например, можно предположить, что проверяться должна переменная e->Top. Однако это только догадка. Мы не знакомы с кодом проекта и не можем отличить ошибки от избыточного кода :).

    И ещё один случай:

    QString SExpression::toString(int indent) const {
      ....
      if (child.isLineBreak() && nextChildIsLineBreak) {
        if (child.isLineBreak() && (i > 0) &&
          mChildren.at(i - 1).isLineBreak()) {
          // too many line breaks ;)
        } else {
          str += '\n';
        }
      }
      ....
    }

    Предупреждение PVS-Studio: V571 CWE-571 Recurring check. The 'child.isLineBreak()' condition was already verified in line 208. sexpression.cpp 209

    Ошибка в логике


    void FootprintPreviewGraphicsItem::paint(....) noexcept {
      ....
      for (const Circle& circle : mFootprint.getCircles()) {
        layer = mLayerProvider.getLayer(*circle.getLayerName());
        if (!layer) continue;                                                  // <=
        if (layer) {                                                           // <=
          pen = QPen(....);
          painter->setPen(pen);
        } else
          painter->setPen(Qt::NoPen);
        ....
      }
      ....
    }

    Предупреждение PVS-Studio: V547 CWE-571 Expression 'layer' is always true. footprintpreviewgraphicsitem.cpp 177

    Поскольку условие во втором операторе if всегда истинно, то ветка else никогда не выполняется.

    Забытая проверка указателя


    extern int ZEXPORT unzGetGlobalComment (
      unzFile file, char * szComment, uLong uSizeBuf)
    {
      ....
      if (uReadThis>0)
      {
        *szComment='\0';
        if (ZREAD64(s->z_filefunc,s->filestream,szComment,uReadThis)!=uReadThis)
          return UNZ_ERRNO;
      }
    
      if ((szComment != NULL) && (uSizeBuf > s->gi.size_comment))
        *(szComment+s->gi.size_comment)='\0';
      ....
    }

    Предупреждение PVS-Studio: V595 CWE-476 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 2068, 2073. unzip.c 2068

    Если uReadThis>0, то разыменовывается указатель szComment. Это опасно, так как этот указатель может быть нулевым. Такое заключение анализатор делает на основании того, что далее этот указатель проверяется на равенство NULL.

    Неинициализированный член класса


    template <class T>
    class Edge
    {
    public:
      using VertexType = Vector2<T>;
        
      Edge(const VertexType &p1, const VertexType &p2, T w=-1) :
        p1(p1), p2(p2), weight(w) {};                             // <=
    
      Edge(const Edge &e) :
        p1(e.p1), p2(e.p2), weight(e.weight), isBad(false) {};
    
      Edge() :
        p1(0,0), p2(0,0), weight(0), isBad(false) {}
    
      VertexType p1;
      VertexType p2;
      T weight=0;
    
      bool isBad;
    };

    Предупреждение PVS-Studio: V730 CWE-457 Not all members of a class are initialized inside the constructor. Consider inspecting: isBad. edge.h 14

    Все конструкторы, кроме первого, инициализируют поле класса isBad. Скорее всего, в первом конструкторе просто случайно забыли сделать эту инициализацию. В результате, первый конструктор создаёт не до конца инициализированный объект, что может привести к неопределённому поведение программы.

    Есть ещё 11 срабатываний диагностики V730. Однако, поскольку мы не знакомы с кодом, сложно говорить, указывают ли эти предупреждения на реальные проблемы. Думаю, эти предупреждения лучше изучить авторам проекта.

    Утечка памяти


    template <typename ElementType>
    void ProjectLibrary::loadElements(....) {
      ....
      ElementType* element = new ElementType(elementDir, false);  // can throw
      if (elementList.contains(element->getUuid())) {
        throw RuntimeError(
            __FILE__, __LINE__,
            QString(tr("There are multiple library elements with the same "
                       "UUID in the directory \"%1\""))
                .arg(subdirPath.toNative()));
      }
      ....
    }

    Предупреждение PVS-Studio: V773 CWE-401 The exception was thrown without releasing the 'element' pointer. A memory leak is possible. projectlibrary.cpp 245

    Если некий элемент уже присутствует в списке, то будет сгенерировано исключение. Но при этом не уничтожается ранее созданный объект, указатель на который хранится в переменной element.

    Неправильный тип исключения


    bool CmdRemoveSelectedSchematicItems::performExecute() {
      ....
      throw new LogicError(__FILE__, __LINE__);
      ....
    }

    Предупреждение PVS-Studio: V1022 CWE-755 An exception was thrown by pointer. Consider throwing it by value instead. cmdremoveselectedschematicitems.cpp 143

    Анализатор обнаружил исключение, брошенное по указателю. Чаще всего принято бросать исключения по значению, а перехватывать по ссылке. Бросание указателя может привести к тому, что исключение не будет поймано, так как перехватывать его будут по ссылке. Также использование указателя вынуждает перехватывающую сторону вызвать оператор delete для уничтожения созданного объекта, чтобы не возникали утечки памяти.

    В общем, оператор new написан здесь случайно и должен быть удалён. То, что это ошибка, подтверждается тем, что во всех остальных местах написано:

    throw LogicError(__FILE__, __LINE__);

    Опасное использование dynamic_cast


    void GraphicsView::handleMouseWheelEvent(
      QGraphicsSceneWheelEvent* event) noexcept
    {
      if (event->modifiers().testFlag(Qt::ShiftModifier))
      ....
    }
    
    bool GraphicsView::eventFilter(QObject* obj, QEvent* event) {
      ....
      handleMouseWheelEvent(dynamic_cast<QGraphicsSceneWheelEvent*>(event));
      ....
    }

    Предупреждение PVS-Studio: V522 CWE-628 Dereferencing of the null pointer 'event' might take place. The potential null pointer is passed into 'handleMouseWheelEvent' function. Inspect the first argument. Check lines: 143, 252. graphicsview.cpp 143

    Указатель, являющийся результатом работы оператора dynamic_cast, передаётся в функцию handleMouseWheelEvent. В ней этот указатель разыменовывается без предварительной проверки.

    Это опасно, так как оператор dynamic_cast может вернуть nullptr. Получается, что этот код ничем не лучше, чем просто использовать более быстрый static_cast.

    В этот код следует добавить явную проверку указателя перед использованием.

    Также, очень часто встречается код вот такого вида:

    bool GraphicsView::eventFilter(QObject* obj, QEvent* event) {
      ....
      QGraphicsSceneMouseEvent* e =
        dynamic_cast<QGraphicsSceneMouseEvent*>(event);
      Q_ASSERT(e);
      if (e->button() == Qt::MiddleButton)
      ....
    }

    Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'e'. graphicsview.cpp 206

    Указатель проверяется с помощью макроса Q_ASSERT. Вот его описание:
    Prints a warning message containing the source code file name and line number if test is false.

    Q_ASSERT() is useful for testing pre- and post-conditions during development. It does nothing if QT_NO_DEBUG was defined during compilation.

    Q_ASSERT плохой способ проверки указателей перед использованием. Как правило в Release версии QT_NO_DEBUG не определён. Я не знаю, как обстоит дело в проекте LibrePCB. Однако, если QT_NO_DEBUG определён в Release, то это странное и нестандартное решение.

    Если макрос раскрывается в пустоту, то получается, что никакой проверки нет. И тогда непонятно зачем вообще было использовать dynamic_cast. Почему тогда не использовать static_cast?

    В общем, этот код с запахом и стоит провести обзор всех схожих фрагментов кода. И их, кстати, весьма много. Я насчитал 82 аналогичных случая!

    Заключение


    В целом проект LibrePCB показался нам качественным. Тем не менее, мы рекомендуем авторам проекта вооружиться инструментом PVS-Studio и самостоятельно провести Code Review участков кода, на которые указывает анализатор. Мы готовы предоставить им бесплатную лицензию на месяц для полноценного анализа проекта. Более того, они могут использовать бесплатный вариант лицензирования анализатора, так как код проекта является открытым и размещён на сайте GitHub. Про этот вариант лицензирования мы скоро напишем.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov, Svyatoslav Razmyslov. Checking LibrePCB with PVS-Studio Inside a Docker Container.
    PVS-Studio
    966,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Похожие публикации

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

      0

      Немножко не по теме, но все же. Решил на днях попробовать PVS-Studio и столкнулся с такой проблемой: plog-converter и PlogConverter.exe — две совершенно разные тулзы.


      1. Набор форматов в которые можно конвертировать отличается.
      2. Отличаются имена у форматов. Например Txt -> text, xml -> Plog
      3. Внутреннее представление сконвертированных файлов тоже отличается. Например если сконвертировать в csv, то количество столбцов и их имена будут отличаться на разных платформах. Аналогично с xml — содержимое отличается.

      Из-за этих отличий пришлось построить целый забор из костылей в билдскрипте чтобы получать что-то хотя бы более-менее одинаковое на всех платформах.


      Было бы не плохо привести все к какому-то общему знаменателю чтобы упростить интеграцию.

        0
        Спасибо за замечание. Подумаем над улучшениями.
          +1
          Здравствуйте, Filippok,

          Расскажите, пожалуйста, о своём сценарии использования анализатора в проектах.

          Вы правы, что это похожие утилиты с пересекающимися возможностями. Но обычно на какой-нибудь платформе используется соответствующей конвертер. Также есть возможность собрать отчёты с разных систем и передать их любому удобному конвертеру, он их объединит и преобразует в выбранный формат.
            0

            Да кейс довольно простой:


            1. Кастомный генератор ninja файлов. Генерит дополнительно шаги для препроцесса и анализа.
            2. Gitlab(self-hosted) CI
            3. Хотелось бы просто выплевывать результаты анализа в аутпут гитлаба
              0
              А в каком формате Вы хотели выводить результат в output? Возможно, Вы искали какой-то специальный формат, а у нас он не реализован, поэтому Вы перепробовали все конвертеры.
                0

                Любой который можно распарсить. xml — отлично подходит. Собственно, так и сделал: -t Plog на windows, -t xml на остальных платформах. Единственное, ноды отличаются. Было бы не плохо как-то унифицировать все это.

                  0

                  А вообще, хорошо было бы иметь отдельную тулзу которой можно скормить format string и путь к логу, а она б уже выдавала результаты. Что-то типа


                  $ plog-output foo.plog -f "{ERRNO} {FILE}:{LINE} -> {MESSAGE}"
                    0
                    Понял, спасибо.

                    Plog так-то тоже xml, просто старый, сложный и со схемой. В xml вошли более востребованные данные. При этом xml является подмножетвом plog'a. Эти форматы поддержаны для SonarQube и там у нас общий парсер. Поэтому и Ваш парсер должен сразу поддерживать оба формата. Ещё Вы можете посмотреть исходники конвертеров на GitHub и сделать форк со своим форматом.
            0
            Посмотрите на FreeCAD. Он огромный, на С++, и, кажется, с немалым числом мест для вашего анализатора…

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

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