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

Checking the LibrePCB project using PVS-Studio inside the Docker container 3r38080. 3r3669.  
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 are using containers, we hope that the article will demonstrate another simple way to integrate the analyzer into the development process. 3r3669.  
3r311.
3r3669.  
3r3654. LibrePCB
3r3669.  
LibrePCB - this 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. 3r3669.  
3r3669.  
3r38080. 3r3669.  
3r3669.  
LibrePCB is a small project containing only about 30?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 . 3r3669.  
3r3669.  
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 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. 3r3669.  
3r3669.  
3r3654. Docker
3r3669.  
Docker - software to automate the deployment and management of 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 the infrastructure of their projects, in the open source world this was not very noticeable until recently. 3r3669.  
3r3669.  
Our company works very closely with open source projects, checking the source code using our own PVS-Studio static analyzer. Currently verified more than 300 projects 3r3672. . The most difficult thing in this activity has always been the compilation of other people's projects, but the implementation of Docker containers greatly simplified this process. 3r3669.  
3r3669.  
The first experience of checking open source project in Docker was from 3r3358. 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: 3r36666.  
3r3669.  
3r33588. 3r3-3589. diff --git a /src /build.sh b /src /build.sh
index 290c57d2a286dc 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)) 3r33598. 3r3599. 3r3669.  
The difference in the LibrePCB project is that they immediately submitted 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:
 
3r3669.  
3r33588. 3r3-3589. FROM ubuntu: ???r3r3679.
# 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
3r33598. 3r3599. 3r3669.  
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. 3r3669.  
3r3669.  
After launching the container, the analyzer was installed and the following commands were executed for assembling and analyzing the project: 3r36666  
3r3669.  
3r33588. 3r3-3589. 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 3r3599. 3r3669.  
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. 3r3669.  
3r3669.  
3r3654. Found errors
3r3669.  
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 code analysis for embedded systems. Here are a couple of articles that some of our readers might have missed:
 
3r3669.  
3r3146.  
3r3149. GNU Arm Embedded Toolchain
has been added to PVS-Studio. ;
 
3r3154. PVS-Studio: support for the MISRA C and MISRA C ++ coding standards ++
.
 
3r3158. 3r3669.  
3r? 3534. Errata 3r33535. 3r3669.  
3r33588. 3r3-3589. SymbolPreviewIndustry.de. noexcept
{
if (mComponent && symbVarUuid && symbVarItemUuid)
if (mComponent && symbVarItemUuid && symbVarItemUuid) //<=
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning: V501 CWE-571 V V V V operator operator operator operator operator operator operator operator operator operator operator operator operator. symbolpreviewgraphicsitem.cpp 74
 
3r3669.  
3r3189. Classic
typo: variable is checked twice in a row. symbVarItemUuid . Above is a similar check, and, looking at it, it becomes clear that the second variable to be checked should be symbVarUuid . 3r3669.  
3r3669.  
The following code snippet is a typo:
 
3r3669.  
3r33588. 3r3-3589. 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);
} 3r33598. 3r3599. 3r3669.  
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
 
3r3669.  
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 on 3r3642. eMaxPair-> Top [/i] . 3r3669.  
3r3669.  
3r? 3534. Unnecessary checks 3r33535. 3r3669.  
3r33588. 3r3-3589. static int 3r3679. 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;
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning: V547 3r3672. CWE-571 Expression 'content' is always true. html.c 162
 
3r3669.  
This is probably not a mistake, but simply a redundant code. No need to re-check the pointer content . If it is zero, the function immediately terminates its work. 3r3669.  
3r3669.  
Similar situation:
 
3r3669.  
3r33588. 3r3-3589. 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);
}
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning:
V547 3r3672. CWE-571 Expression 'e-> OutIdx> = 0' is always true. clipper.cpp 2983
 
3r3669.  
Re-check 3r3642. (e-> OutIdx> = 0) [/i] does not make sense. However, this may be a mistake. For example, we can assume that the variable should be checked e-> Top . However, this is only a guess. We are not familiar with the project code and cannot distinguish errors from redundant code :). 3r3669.  
3r3669.  
And one more case:
 
3r3669.  
3r33588. 3r3-3589. 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';
}
}
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning:
V571
CWE-571 Recurring check. The 'child.isLineBreak ()' condition was already verified in line 208. sexpression.cpp 209
 
3r3669.  
3r? 3534. Error in the logic 3r33535. 3r3669.  
3r33588. 3r3-3589. 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);
}
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning: V547 3r3672. CWE-571 Expression 'layer' is always true. footprintpreviewgraphicsitem.cpp 177
 
3r3669.  
Since the condition in the second statement is if always true, then branch else never done. 3r3669.  
3r3669.  
3r? 3534. Forgotten pointer check 3r33535. 3r3669.  
3r33588. 3r3-3589. extern int ZEXPORT unzGetGlobalComment (
unzFile file, char * szComment, uLong uSizeBuf)
{
if (uReadThis> 0)
{
* szComment = '  3r33395. V595
CWE-476 The 'szComment' pointer has been verified against nullptr. Check lines: 206? 2073. unzip.c 2068
 
3r3669.  
If uReadThis> 0 then the pointer is derefered szComment . 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 equality NULL . 3r3669.  
3r3669.  
3r? 3534. An uninitialized member of class 3r33535. 3r3669.  
3r33588. 3r3-3589. template
class Edge
{
public:
using VertexType = Vector2
;
Edge (const VertexType & p? const VertexType & p? 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), p2 (?0), weight (0), isBad (false) {}
VertexType p1;
VertexType p2;
T weight = 0;
bool isBad;
}; 3r3-3598. 3r3599. 3r3669.  
PVS-Studio warning: V730 CWE-457 is the initialized inside of the constructor. Consider inspecting: isBad. edge.h 14
 
3r3669.  
All designers, except the first, initialize the field of the class isBad . 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. 3r3669.  
3r3669.  
There are 11 more diagnostics triggers. V730 . 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. 3r3669.  
3r3669.  
3r? 3534. Memory leak 3r33535. 3r3669.  
3r33588. 3r3-3589. 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 ""))
.grg)) );
}
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning: V773 CWE-401 A memory leak is possible. projectlibrary.cpp 245
 
3r3669.  
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 . 3r3669.  
3r3669.  
3r? 3534. Wrong exception type
3r3669.  
3r33588. 3r3-3589. bool CmdRemoveSelectedSchematicItems :: performExecute () {
throw new LogicError (__ FILE__, __LINE__);
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning: V1022 CWE-755 An exception was thrown by pointer. Consider throwing it by value instead. cmdremoveselectedschematicitems.cpp 143
 
3r3669.  
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 the pointer forces the intercepting side to call the operator 3r3642. delete [/i] to destroy the created object so that memory leaks do not occur. 3r3669.  
3r3669.  
In general, the operator new written here by chance and must be deleted. The fact that this is a mistake is confirmed by the fact that in all other places it is written:
 
3r3669.  
3r33588. 3r3-3589. throw LogicError (__ FILE__, __LINE__); 3r3-3598. 3r3599. 3r3669.  
3r? 3534. Dangerous use of dynamic_cast
3r3669.  
3r33588. 3r3-3589. void GraphicsView :: handleMouseWheelEvent (
QGraphicsSceneWheelEvent * event) noexcept
{
if (event-> modifiers (). testFlag (Qt :: ShiftModifier))
}
bool GraphicsView :: eventFilter (QObject * obj, QEvent * event) {
handleMouseWheelEvent (dynamic_cast
(event));
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning: V522 3r3672. 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: 14? 252. graphicsview.cpp 143
 
3r3669.  
The pointer that is the result of the work of the operator 3r3642. dynamic_cast [/i] , is passed to the function handleMouseWheelEvent . In it, this pointer is dereferenced without prior verification. 3r3669.  
3r3669.  
This is dangerous, because the operator dynamic_cast may return 3r3642. nullptr [/i] . It turns out that this code is no better than just using the faster static_cast . 3r3669.  
3r3669.  
In this code, you should add an explicit pointer check before use. 3r3669.  
3r3669.  
Also, a code like this is very common:
 
3r3669.  
3r33588. 3r3-3589. bool GraphicsView :: eventFilter (QObject * obj, QEvent * event) {
QGraphicsSceneMouseEvent * e =
dynamic_cast
(event);
Q_ASSERT (e);
if (e-> button () == Qt :: MiddleButton)
} 3r33598. 3r3599. 3r3669.  
PVS-Studio warning:
V522 3r3672. CWE-690 There might be a null pointer 'e'. graphicsview.cpp 206
 
3r3669.  
The pointer is checked using macro Q_ASSERT . Here is its description:
 
3r3612.  
3r31414.  
3r31616. If the test is false. 3r3669.  
3r3669.  
Q_ASSERT () is useful for pre-and post-conditions during development. It doesn’t if QT_NO_DEBUG was defined during compilation. 3r3669.  
 
3r33625.  
3r3669.  
Q_ASSERT bad way to check pointers before use. Usually in Release version QT_NO_DEBUG not determined. I do not know what the situation is with the LibrePCB project. However, if r3r3642. QT_NO_DEBUG [/i] defined in Release, this is a strange and non-standard solution. 3r3669.  
3r3669.  
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 3r3642. dynamic_cast [/i] . Why then not use static_cast ? 3r3669.  
3r3669.  
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! 3r3669.  
3r3669.  
3r3654. Conclusion 3r33655. 3r3669.  
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. 3r3669.  
3r3669.  
3r3662.
3r3664.
3r3666. 3r3669.  
3r3669.  
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 . 3r38080.
3r3777. ! function (e) {function t (t, n) {if (! (n in e)) {for (var r, a = e.document, i = a.scripts, o = i.length; o-- ;) if (-1! == i[o].src.indexOf (t)) {r = i[o]; break} if (! r) {r = a.createElement ("script"), r.type = "text /jаvascript", r.async =! ? r.defer =! ? r.src = t, r.charset = "UTF-8"; var d = function () {var e = a.getElementsByTagName ("script")[0]; e.parentNode.insertBefore (r, e)}; "[object Opera]" == e.opera? a.addEventListener? a.addEventListener ("DOMContentLoaded", d,! 1): e.attachEvent ("onload", d ): d ()}}} t ("//mediator.mail.ru/script/2820404/"""_mediator") () (); 3r3678.
3r38080.
+ 0 -

Add comment