# Checking LibrePCB with PVS\-Studio Inside a Docker Container This is a classic article, where we share our experience of checking an open\-source project called LibrePCB with PVS\-Studio\. What makes it special, though, is the fact that the analysis was done inside a Docker container\. If you use containers, we hope this article will show you another way to easily integrate the analyzer into your development process\. ![0598_LibrePCB/image1.png](https://import.viva64.com/docx/blog/0598_LibrePCB/image1.png) ## LibrePCB [LibrePCB](https://librepcb.org/) is a free EDA application for developers of printed circuit boards\. The source code is written in C\+\+, while the GUI is built using Qt5\. The first official release, where the developers established their own file format \(\*\.lp, \*\.lplib\), took place recently\. Binary packages were prepared for Linux, macOS, and Windows\. ![0598_LibrePCB/image3.png](https://import.viva64.com/docx/blog/0598_LibrePCB/image3.png) LibrePCB is a small project consisting of about 300,000 non\-empty lines of code in C and C\+\+, 25% of which are comments, which is pretty much for comments, I should say\. This probably has to do with the fact that the project is made up of lots of small files, which to a great extent consist of header comments with project and licensing information\. The source code can be downloaded from GitHub: [LibrePCB](https://github.com/librepcb/librepcb)\. The project looked interesting, so we decided to check it\. The results weren't that exciting, though\. True, we found some genuine bugs, but they were nothing special \- nothing that you'd like to bother writing about\. We could have contented ourselves with just reporting the bugs to the project authors, but there was one thing that made this experience special: the check was done inside a Docker image\. That's why we decided to write this article\. ## Docker [Docker](https://www.docker.com/) is a computer program that performs operating\-system\-level virtualization, also known as "containerization"\. Docker is used to run software packages called "containers"\. Containers are isolated from each other and bundle applications with their own tools, libraries, and configuration files\. Although this technology has been around for about five years already and many companies have long had Docker integrated into their projects, it wasn't that prominent in the open\-source world until recently\. Our company is very tightly connected with the open\-source community as we use our own static code analyzer, PVS\-Studio, to check the source code of open\-source projects\. By now, we have [more than 300 checked projects](https://pvs-studio.com/en/blog/inspections/) under our belt\. Compiling programs written by others has always been the most difficult part of this activity, but Docker containers have made this process way easier\. [Azure Service Fabric](https://pvs-studio.com/en/blog/posts/cpp/0567/) was the first open\-source project we checked in Docker\. The developers mounted the directory with the source files in the container, so we just had to edit one of the scripts running inside the container to integrate the analyzer: ```cpp 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 LibrePCB project is different in that they provided a Dockerfile right away to build the image and the project inside it\. This suits us even more\. Here's the part of the Docker file that we are interested in: ```cpp 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 won't compile and install the project when building the image\. So, we built an image inside which the authors guarantee a successful build of the project\. After launching the container, we installed the analyzer and ran the following commands to build and analyze the project: ```cpp 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 of that was done on Windows 10\. It's cool that all popular operating systems are supporting the containerization technology too\. Unfortunately, containers aren't that convenient on Windows \- especially because you can't install software as easily\. ## Bugs found Now, this is a classic section where we comment on bugs found with PVS\-Studio\. While we are at it, I'd like to remind you that we've been working on adding support of embedded systems to our analyzer lately\. Here are a couple of articles that some of you could have missed: 1. [PVS\-Studio Now Supports GNU Arm Embedded Toolchain](https://pvs-studio.com/en/blog/posts/cpp/0588/); 1. [PVS\-Studio: Support of MISRA C and MISRA C\+\+ Coding Standards](https://pvs-studio.com/en/blog/posts/cpp/0596/)\. ### Typos ```cpp SymbolPreviewGraphicsItem::SymbolPreviewGraphicsItem( const IF_GraphicsLayerProvider& layerProvider, const QStringList& localeOrder, const Symbol& symbol, const Component* cmp, const tl::optional& symbVarUuid, const tl::optional& symbVarItemUuid) noexcept { if (mComponent && symbVarUuid && symbVarItemUuid) .... if (mComponent && symbVarItemUuid && symbVarItemUuid) // <= .... } ``` PVS\-Studio diagnostic message: [V501](https://pvs-studio.com/en/docs/warnings/v501/) CWE\-571 There are identical sub\-expressions 'symbVarItemUuid' to the left and to the right of the '&&' operator\. symbolpreviewgraphicsitem\.cpp 74 This is a [classic](https://pvs-studio.com/en/blog/examples/v501/) typo: the _symbVarItemUuid_ variable is checked twice\. A similar check above it suggests that the second check should include the variable _symbVarUuid_\. Another example: ```cpp 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 diagnostic message: [V778](https://pvs-studio.com/en/docs/warnings/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 This code must have been written using copy\-paste\. The developer forgot to change _e\-\>Top_ to _eMaxPair\-\>Top_ in the second block\. ### Redundant checks ```cpp 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, ""); if (content) hoedown_buffer_put(ob, content->data, content->size); HOEDOWN_BUFPUTSL(ob, ""); return 1; } ``` PVS\-Studio diagnostic message: [V547](https://pvs-studio.com/en/docs/warnings/v547/) CWE\-571 Expression 'content' is always true\. html\.c 162 This one looks more like redundant code than a bug\. There's no need to check the _content_ pointer one more time: if it's null, the function will terminate right away\. Another similar case: ```cpp 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 diagnostic message: [V547](https://pvs-studio.com/en/docs/warnings/v547/) CWE\-571 Expression 'e\-\>OutIdx \>\= 0' is always true\. clipper\.cpp 2983 The second _\(e\-\>OutIdx \>\= 0\)_ check is unnecessary\. This could also be an error, though\. For instance, the developer may have intended to check the _e\-\>Top_ variable\. But it's only a guess\. We aren't familiar with the code well enough to reliably distinguish bugs from redundant code :\)\. And the last case here: ```cpp 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 diagnostic message: [V571](https://pvs-studio.com/en/docs/warnings/v571/) CWE\-571 Recurring check\. The 'child\.isLineBreak\(\)' condition was already verified in line 208\. sexpression\.cpp 209 ### Logical error ```cpp 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 diagnostic message: [V547](https://pvs-studio.com/en/docs/warnings/v547/) CWE\-571 Expression 'layer' is always true\. footprintpreviewgraphicsitem\.cpp 177 Since the condition of the second _if_ statement is always true, the _else_ branch will never execute\. ### Missing pointer check ```cpp 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 diagnostic message: [V595](https://pvs-studio.com/en/docs/warnings/v595/) CWE\-476 The 'szComment' pointer was utilized before it was verified against nullptr\. Check lines: 2068, 2073\. unzip\.c 2068 If _uReadThis\>0_, the _szComment_ pointer will be dereferenced\. This is dangerous because the pointer could be null\. The analyzer draws this conclusion based on the later _NULL_ check\. ### Uninitialized class member ```cpp template class Edge { public: using VertexType = Vector2; 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 diagnostic message: [V730](https://pvs-studio.com/en/docs/warnings/v730/) CWE\-457 Not all members of a class are initialized inside the constructor\. Consider inspecting: isBad\. edge\.h 14 Each of the constructors, save the first, initializes the class field _isBad_\. It looks like the developer simply forgot to add the initialization code to the first constructor\. As a result, the first constructor creates an incompletely initialized object, which may end up with undefined behavior\. We got 11 more [V730](https://pvs-studio.com/en/docs/warnings/v730/) warnings\. But since we are not familiar with the code, we can't say for sure if these warnings point at real defects\. We'd better leave it to the authors to decide\. ### Memory leak ```cpp template 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 diagnostic message: [V773](https://pvs-studio.com/en/docs/warnings/v773/) CWE\-401 The exception was thrown without releasing the 'element' pointer\. A memory leak is possible\. projectlibrary\.cpp 245 If an element is already in the list, an exception will be thrown\. However, the previously created object, the pointer to which is stored in the _element_ variable, won't be destroyed\. ### Incorrect exception type ```cpp bool CmdRemoveSelectedSchematicItems::performExecute() { .... throw new LogicError(__FILE__, __LINE__); .... } ``` PVS\-Studio diagnostic message: [V1022](https://pvs-studio.com/en/docs/warnings/v1022/) CWE\-755 An exception was thrown by pointer\. Consider throwing it by value instead\. cmdremoveselectedschematicitems\.cpp 143 The analyzer has detected an exception thrown by pointer\. A common practice is to throw exceptions by value and catch them by reference\. When throwing a pointer, the catcher may fail to catch the exception because it will attempt to catch it by reference\. The catcher would also have to call the _delete_ operator to destroy the created object to avoid memory leaks when throwing a pointer\. So, the _new_ operator was written by mistake and should be deleted\. The conclusion that this is an error is confirmed by the following code found in all the other cases: ```cpp throw LogicError(__FILE__, __LINE__); ``` ### Dangerous use of dynamic\_cast ```cpp void GraphicsView::handleMouseWheelEvent( QGraphicsSceneWheelEvent* event) noexcept { if (event->modifiers().testFlag(Qt::ShiftModifier)) .... } bool GraphicsView::eventFilter(QObject* obj, QEvent* event) { .... handleMouseWheelEvent(dynamic_cast(event)); .... } ``` PVS\-Studio diagnostic message: [V522](https://pvs-studio.com/en/docs/warnings/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 The pointer returned by the _dynamic\_cast_ operator is passed to the function _handleMouseWheelEvent_ and dereferenced in it without any prior check\. This is unsafe because the _dynamic\_cast_ operator could return _nullptr_\. It means this code is no better than the faster _static\_cast_\. The developers should explicitly check the pointer before using it\. The following code pattern is also very common: ```cpp bool GraphicsView::eventFilter(QObject* obj, QEvent* event) { .... QGraphicsSceneMouseEvent* e = dynamic_cast(event); Q_ASSERT(e); if (e->button() == Qt::MiddleButton) .... } ``` PVS\-Studio diagnostic message: [V522](https://pvs-studio.com/en/docs/warnings/v522/) CWE\-690 There might be dereferencing of a potential null pointer 'e'\. graphicsview\.cpp 206 The pointer is checked using the _Q\_ASSERT_ macro\. Let's look at its description: > 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_ is a bad way to check pointers before using them\. _QT\_NO\_DEBUG_ is typically not defined in the Release version\. I don't know if it is defined in the LibrePCB project, but if it is, that would be pretty strange and unconventional\. If the macro is expanded into nothing, it means no check\. Why use _dynamic\_cast_ at all then? Why not use _static\_cast_? So, my point is that this code smells and the rest similar cases need reviewing too\. There's plenty of them, by the way \- **82\!** ## Conclusion Overall, we found LibrePCB pretty high\-quality\. However, we still recommend that the authors deploy PVS\-Studio and review the reported code snippets\. We can help by providing a free license for one month so that they could analyze the project in full\. In addition, they can make use of our free licensing policy since the project is open\-source and stored on GitHub\. We'll soon write about this option of licensing\.