Checking the LibrePCB project with PVS-Studio inside the Docker container

This is a classic article about how our team checked the open LibrePCB project with the help of the PVS-Studio static code analyzer. However, the article is interesting because the check was performed inside the Docker container. If you use containers, we hope that the article will demonstrate another simple way to integrate the analyzer into the development process.

LibrePCB

LibrePCB is free software for designing electronic circuits and printed circuit boards. The program code is written in C ++, and Qt5 is used to build the graphical interface. Recently, the first official release of this application, marked the stabilization of its own file format (* .lp, * .lplib). Binary packages are prepared for Linux, macOS and Windows.



LibrePCB is a small project containing only about 300,000 non-empty lines of C and C ++ code. At the same time, 25% of non-empty lines are comments. By the way, this is a big percentage for comments. Most likely, this is due to the fact that there are many small files in the project, a substantial part of which is occupied by the header comments from the project information and license. The code on the site GitHub: LibrePCB .
')
The project seemed interesting to us, and we decided to check it out. But the test results were not so interesting. Yes, there were real mistakes. But there was not anything special about which you should definitely tell the readers of our articles. Perhaps we would not write an article and limit ourselves to sending the found errors to the project developers. However, the interesting point was that the project was tested inside the Docker image, and therefore we decided that it was still worth writing an article.

Docker

Docker is an automation software for deploying and managing applications in a virtualization environment at the operating system level. It allows you to "pack" the application with all its environments and dependencies in the container. Although this technology is about five years old and many companies have long introduced Docker into their project infrastructures, in the open source world this was not very noticeable until recently.

Our company works very closely with open source projects, checking the source code using our own PVS-Studio static analyzer. At the moment, more than 300 projects have been verified. The most difficult thing in this activity has always been the compilation of other people's projects, but the implementation of Docker containers has greatly simplified this process.

The first experience with testing an open source project in Docker was with Azure Service Fabric . There, the developers did mount the source directory to the container and analyzer integration was limited to editing one of the scripts running in the container:

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)) 

The difference between the LibrePCB project is that they immediately provided the Dockerfile to build the image and the project in it. It turned out to be even more convenient for us. Here is the part of the Docker file that we are interested in:

 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 .... 

We will not compile and install the project when building the image. Thus, we have assembled an image in which the author of the project guarantees the successful assembly of the project.

After launching the container, the analyzer was installed and the following commands were executed for assembling and analyzing the project:

 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 

By the way, all actions were performed in Windows 10. I am very pleased that the developers of all popular operating systems are also developing in this direction. Unfortunately, Windows containers are not so convenient. Especially because of the inability to install software as well.

Bugs found

Now the classic section containing the description of the errors we found using the PVS-Studio static code analyzer. By the way, using the case, I want to remind you that lately we have been developing the analyzer in the direction of supporting analysis of code for embedded systems. Here are a couple of articles that some of our readers might have missed:

1. GNU Arm Embedded Toolchain has been added to PVS-Studio ;
2. PVS-Studio: support for MISRA C and MISRA C ++ coding standards .

Typos

 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 warning : V501 CWE-571 There are identical sub-expressions of the operator. symbolpreviewgraphicsitem.cpp 74

Classic typo: twice the variable symbVarItemUuid is checked . Above is a similar check, and, looking at it, it becomes clear that the second variable to be checked must be a symbVarUuid .

The following is a typo code snippet:

 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 warning : V778 CWE-682 Two code fragments were found. Perhaps this is a typo and 'eMaxPair' variable should not be used instead of 'e'. clipper.cpp 2999

Most likely, the code was written using Copy-Paste. Due to an oversight in the second block of text, they forgot to replace e-> Top with eMaxPair-> Top .

Extra checks

 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 warning : V547 CWE-571 Expression 'content' is always true. html.c 162

This is probably not an error, but simply a redundant code. No need to re-check the content pointer. If it is zero, the function immediately terminates its work.

Similar situation:

 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 warning : V547 CWE-571 Expression 'e-> OutIdx> = 0' is always true. clipper.cpp 2983

Rechecking (e-> OutIdx> = 0) does not make sense. However, this may be a mistake. For example, it can be assumed that the variable e-> Top should be checked. However, this is only a guess. We are not familiar with the project code and cannot distinguish errors from redundant code :).

And one more case:

 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 warning : V571 CWE-571 Recurring check. The 'child.isLineBreak ()' condition was already verified in line 208. sexpression.cpp 209

Error in logic

 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 warning : V547 CWE-571 Expression 'layer' is always true. footprintpreviewgraphicsitem.cpp 177

Since the condition in the second if statement is always true, the else branch is never executed.

Forgotten pointer check

 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 Warning: V595 CWE-476: The 'szComment' Check lines: 2068, 2073. unzip.c 2068

If uReadThis> 0 , then the szComment pointer is dereferenced . This is dangerous, as this pointer may be null. The analyzer makes this conclusion on the basis of the fact that this pointer is further checked for NULL equality.

Uninitialized class member

 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 Warning: V730 CWE-457 Notice . Consider inspecting: isBad. edge.h 14

All constructors except the first initialize the isBad class field . Most likely, in the first constructor, they simply accidentally forgot to do this initialization. As a result, the first constructor creates an incompletely initialized object, which can lead to undefined program behavior.

There are 11 more V730 diagnostic triggers . However, since we are not familiar with the code, it is difficult to say whether these warnings indicate real problems. I think these warnings are best studied by the authors of the project.

Memory leak

 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 warning : V773 CWE-401 A memory leak is possible. projectlibrary.cpp 245

If a certain element is already present in the list, an exception will be generated. But it does not destroy the previously created object, the pointer to which is stored in the variable element .

Wrong type of exception

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

PVS-Studio warning : V1022 CWE-755 An exception was thrown by pointer. Consider throwing it by value instead. cmdremoveselectedschematicitems.cpp 143

The analyzer detected an exception thrown by the pointer. Most often, it is customary to throw exceptions by value, and to intercept by reference. Throwing a pointer can lead to the fact that the exception will not be caught, since it will intercept it by reference. Also, the use of a pointer forces the intercepting side to call the delete operator to destroy the created object so that memory leaks do not occur.

In general, the operator new is written here by chance and should be removed. The fact that this is a mistake is confirmed by the fact that in all other places it is written:

 throw LogicError(__FILE__, __LINE__); 

Dangerous use of 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 warning : V522 CWE-628 Dereferencing of the null pointer 'event' might take place. The potential null pointer is passed into the 'handleMouseWheelEvent' function. Inspect the first argument. Check lines: 143, 252. graphicsview.cpp 143

The pointer, which is the result of the dynamic_cast operator, is passed to the handleMouseWheelEvent function. In it, this pointer is dereferenced without prior verification.

This is dangerous because the dynamic_cast statement can return nullptr . It turns out that this code is no better than just using the faster static_cast .

In this code, you should add an explicit pointer check before use.

Also, the following code is very common:

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

PVS-Studio warning : V522 CWE-690 There might be a pointer of a potential null pointer 'e'. graphicsview.cpp 206

The pointer is checked using the Q_ASSERT macro. Here is its description:

If the test is false. Q_ASSERT () is useful for pre-and post-conditions during development. It doesn’t if QT_NO_DEBUG was defined during compilation.

Q_ASSERT is a bad way to check pointers before use. As a rule, the release version of QT_NO_DEBUG is not defined. I do not know what the situation is with the LibrePCB project. However, if QT_NO_DEBUG is defined in Release, then this is a strange and non-standard solution.

If a macro expands into emptiness, it turns out that there is no verification. And then it is not clear why it was generally used dynamic_cast . Why then not use static_cast ?

In general, this code is with a smell and it is worth reviewing all similar code fragments. And, by the way, there are a lot of them. I counted 82 similar cases!

Conclusion

In general, the LibrePCB project seemed to be of high quality. Nevertheless, we recommend the project authors to arm themselves with the PVS-Studio tool and independently conduct a Code Review of the code sections indicated by the analyzer. We are ready to provide them with a free license for a month to fully analyze the project. Moreover, they can use the free licensing option of the analyzer, since the project code is open and posted on the GitHub website. We will write about this licensing option soon.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov, Svyatoslav Razmyslov. Checking LibrePCB with PVS-Studio Inside a Docker Container .