Хабрахабр

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

PVS-Studio and Docker Container

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

LibrePCB

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

LibrePCB

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

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

Docker

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

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

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

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

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

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

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

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.

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

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

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

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.

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

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

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

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

Заключение

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

Checking LibrePCB with PVS-Studio Inside a Docker Container. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov, Svyatoslav Razmyslov.

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш адрес email не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть