To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (an extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Checking LibrePCB with PVS-Studio Insid…

Checking LibrePCB with PVS-Studio Inside a Docker Container

Dec 18 2018

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

LibrePCB

LibrePCB 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/image2.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.

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

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:

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:

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:

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 diagnostic message: 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 typo: the symbVarItemUuid variable is checked twice. A similar check above it suggests that the second check should include the variable symbVarUuid.

Another example:

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

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 diagnostic message: 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:

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

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

Logical error

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

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

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 diagnostic message: 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 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

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 diagnostic message: 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

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

PVS-Studio diagnostic message: 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:

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 diagnostic message: 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:

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

PVS-Studio diagnostic message: 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.

Popular related articles
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept