Это классическая статья о том, как наша команда проверила открытый проект LibrePCB с помощью статического анализатора кода PVS-Studio. Однако статья интересна тем, что проверка осуществлялась внутри Docker контейнера. Если вы используете контейнеры, то надеемся, что статья продемонстрирует ещё один простой способ встроить анализатор в процесс разработки.
LibrePCB - это свободное ПО для проектирования электронных схем и печатных плат. Код программы написан на языке C++, а для построения графического интерфейса используется Qt5. Недавно состоялся первый официальный релиз этого приложения, ознаменовавший собою стабилизацию собственного формата файлов (*.lp, *.lplib). Бинарные пакеты подготовлены для Linux, macOS и Windows.
LibrePCB - это маленький проект, содержащий всего около 300 000 непустых строк кода на языке C и C++. При этом 25% непустых строк — это комментарии. Кстати, это большой процент для комментариев. Скорее всего, это связано с тем, что в проекте много маленьких файлов, существенную часть которых занимают заголовочные комментарии с информаций о проекте и лицензии. Код на сайте GitHub: LibrePCB.
Проект показался нам интересным, и мы решили проверить его. А вот результаты проверки оказались уже не такими интересными. Да, нашлись настоящие ошибки. Но не было чего-то особенного, о чём непременно надо рассказать читателям наших статей. Возможно, мы бы не стали писать статью и ограничились отправкой найденных ошибок разработчикам проекта. Однако интересным моментом стало то, что проект был проверен внутри 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. Кстати, пользуясь случаяем, хочу напомнить, что в последнее время мы развиваем анализатор в сторону поддержки анализа кода для встраиваемых систем. Вот пара статей, которые некоторые наши читатели могли пропустить:
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__);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. Про этот вариант лицензирования мы скоро напишем.