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

    PVS-Studio and 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 carried out 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


    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 nothing special about which we must tell the readers of our articles. Perhaps we would not write an article and limit ourselves to sending the errors found 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 surroundings and dependencies into a container. Although this technology is about five years old and many companies have long introduced Docker into the infrastructure of their projects, 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 made mounting 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 opportunity, I want to remind you that lately we have been developing the analyzer in the direction of supporting code analysis 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: two times the variable symbVarItemUuid is checked . Above is a similar test, and, looking at it, it becomes clear that the second variable to be checked must be a symbVarUuid .

    The following code snippet is a typo:

    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


    staticintrndr_emphasis(hoedown_buffer *ob, const hoedown_buffer *content,
              const hoedown_renderer_data *data){
  if (!content || !content->size) return0;
  HOEDOWN_BUFPUTSL(ob, "<em>");
  if (content) hoedown_buffer_put(ob, content->data, content->size);
  HOEDOWN_BUFPUTSL(ob, "</em>");
  return1;
}

    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, then the function immediately terminates its work.

    Similar situation:

    void Clipper::DoMaxima(TEdge *e)
{
  ....
  elseif( 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

    Rescan (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 satisfied.

    Forgotten pointer check


    externint 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 dereferenceed . This is dangerous since this pointer may be null. The analyzer makes this conclusion on the basis that this pointer is further checked for NULL equality .

    Uninitialized class member


    template <classT>
classEdge
{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 but the first constructors 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 better studied by the authors of the project.

    Memory leak


    template <typename ElementType>
void ProjectLibrary::loadElements(....) {
  ....
  ElementType* element = new ElementType(elementDir, false);  // can throwif (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 an element is already in the list, an exception will be thrown. 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() {
  ....
  thrownew 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 may 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 that 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 undefined . 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 a void, it turns out that there is no verification. And then it is not clear why it was at all to use 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 analyzer licensing option, 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 .
    Tags:

    Also popular now: