Andrey Karpov , Sviatoslav Razmyslov

Dec 18 2018

Tags:

#Cpp #DevOps

Checking LibrePCB with PVS-Studio Inside a Docker Container

Dec 18 2018

Author: Andrey Karpov , Sviatoslav Razmyslov

LibrePCB
Docker
Bugs found
Conclusion

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.

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.

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.

#Cpp #DevOps