# FreeCAD and undefined behavior in C\+\+ code: meditation for developers

Examining the project code with the help of a static analyzer, sometimes we wonder how the error appeared and why no one noticed it\. Would you like to see such an example? If so, welcome to read the article\!

![1072_FreeCAD/image1.png](https://import.viva64.com/docx/blog/1072_FreeCAD/image1.png)

## The unnatural

You're reading a classic article where I check an open\-source project with the help of the PVS\-Studio static analyzer\. We regularly post such articles to popularize the static code analysis methodology\. Moreover, we strive to improve the quality of checked projects since eventually they contain less bugs\.

But I should warn you: this is an unnatural way of using the code analyzer\! The analyzer is useful when used regularly and helps detect errors at the early stage\. Fixing an error during the development is much cheaper than fixing it at the testing stage or when the user reports about it\. This is described in detail in the article: "[Introduce static analysis in the process, don't just search for bugs with it](https://habr.com/en/articles/440610/)"\.

When writing an article about project check, the authors have another task\. They should look through the list of warnings and select some fascinating bugs that show the benefits of static analysis\. Such type of articles is useful for learning purposes but have nothing to do with enhancing the development process\.

To make it clear, I'd make an analogy between analyzers and compilers\.

The analyzer and the compiler warnings are basically the same thing\. However, specialized analyzers perform more code checks and use different [technologies](https://pvs-studio.com/en/blog/posts/0908/) to detect complex error patterns\.

It seems unwise to develop a project with compiler warnings disabled\. If you enable warnings every couple month only, this won't be effective:

1. You will quickly drown in many warnings and get tired of reading them\. As a result, you may mistakenly assume that some useful warnings are false positives\. 
1. You have already fixed some errors when debugging\. However, you could have fixed them at the compile time if you had enabled warnings\.
1. You won't understand what to do with some warnings\. For example, they point to [redundant code](https://pvs-studio.com/en/blog/posts/cpp/0968/) instead of errors\. It's unwise to do a large\-scale refactoring to fix all such errors\. It's also wrong to leave everything as it is and review these code snippets during the next code analysis\. So, it's better not to get to this case and gradually refactor the code instead\.

The same thing applies to warnings of static code analyzers\. It's better not to run such tools from time to time but embed them in the development process\.

I guess our regular readers are surprised by so much theory — sorry for that\. A new generation of programmers grows up, and they are ready to step on the same rake\. Therefore, it's useful to retell such stories over and over again\.

## The FreeCAD project

[FreeCAD](https://www.freecad.org/) is a general\-purpose parametric 3D computer\-aided design \(CAD\) modeler\. This open\-source software application is based on C, C\+\+, and Python\. PVS\-Studio doesn't support Python at the moment, so I will only check the C and C\+\+ parts\. Here is the [link](https://github.com/FreeCAD/FreeCAD) to the project code\.

It's not the first time we've written an article about the FreeCAD project\. We wrote the previous one in 2015\. It's been 8 years — quite a long time ago\. Seems like a lot might have changed in the project by now, and there's no need to read the previous article, but just in case, [here](https://pvs-studio.com/en/blog/posts/cpp/0322/) it is\.

Let's take a look at the very special bug I mentioned earlier\.

## Typo and undefined behavior

```cpp
QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (!vpp) {
    return vpp->getQGVPage();
  }
  return nullptr;
}
```

The PVS\-Studio warning: [V522](https://pvs-studio.com/en/docs/warnings/v522/) \[CWE\-476, CERT\-EXP34\-C\] Dereferencing of the null pointer 'vpp' might take place\. QGIView\.cpp 592

The author of the code made a typo while writing conditions:

* If the pointer is non\-null, the function will do nothing and will return _nullptr_\.
* If the pointer is null, dereferencing will occur, and after that, [undefined behavior](https://pvs-studio.com/en/blog/posts/cpp/0306/) will appear\.

It's plain and clear\. However, I've found this case intriguing for several reasons\.

**The first reason for meditation\.** In my previous article, I described an almost identical error\! Here is this article: "[Simple, yet easy\-to\-miss errors in code](https://pvs-studio.com/en/blog/posts/cpp/1068/)"\. And here is the code:

```cpp
if (!SomeGlobalPointer)
{
  delete[] SomeGlobalPointer;
}
```

Well, we've just wondered how developers make such errors, and here we go again\. Starting to read the bug report for FreeCAD, I suddenly stumble across an almost identical bug\. Guess I catch some kind of vibe :\)\.

P\.S\. Actually, there's nothing unusual here\. What impressed me is that this case shows the [Baader–Meinhof phenomenon](https://en.wikipedia.org/wiki/Frequency_illusion) at work\. It occurs when something you know or just learned suddenly seems to appear everywhere\.

**The second reason for meditation\.** I wonder how such an error even appeared\! The thing is, there's an identical function nearby\. And it contains a correct condition\! See it for yourself:

```cpp
QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (!vpp) {
    return vpp->getQGVPage();
  }
  return nullptr;
}

QGSPage* QGIView::getQGSPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (vpp) {
    return vpp->getQGSPage();
  }
  return nullptr;
}
```

I don't know how that happened\.

It's very likely that a developer copy\-pastes the second function\. But something doesn't add up\.\.\. It's strange that a developer has fixed the error in the second function, but left it in the first one\. I don't believe in that scenario\.

After thinking about it, I came up with the following hypothesis\. Maybe both functions had an error\. Then, someone noticed that the _getQGSPage_ function wasn't working correctly, and then fixed it\.

However, this idea was not proven\. First, the change history didn't contain any place where both functions were incorrect\. The incorrect and correct function appear in the [commit](https://github.com/FreeCAD/FreeCAD/commit/6677cd4d8f8c79d4f1a48681d604286408390f7b?diff=split) at the same time\. A developer refactored the code to reduce the number of duplicate code\.

Second, these functions are often used equally and occasionally combined\. It's strange to notice a bug in the behavior of one function and not notice it in another\.

Mystery\!

**The third reason for meditation\.** We don't know how an error like this will manifest itself\. If the pointer is null, dereferencing it will cause undefined behavior\. The condition helps the compiler in recognizing that the pointer is null\. I wonder what it will do and what code it will generate\.

I know it makes no sense to predict how the compiler will behave and how undefined behavior will manifest itself\. Any assumptions will be wrong\. By the way, here's a good article on this topic: "[Falsehoods programmers believe about undefined behavior](https://pvs-studio.com/en/blog/posts/cpp/1024/)"\.

Nevertheless, I wondered if I could write similar code that would run as correct code due to undefined behavior\. Or at least the code that won't crash\. This could partly explain why the bug hasn't been noticed yet\.

To be honest, I wasn't very good at it\. The code was very unstable\. The slightest change to the code greatly affected its behavior\. 

However, it may also happen that the code won't crush\. Here's an example of such [code](https://godbolt.org/z/9s9EKfaYK)\.

If the code is compiled in unoptimized mode \(\-O0\), it will be terminated:

_Program terminated with signal: SIGSEGV_

If we build it with optimization \(\-O2\), it outputs garbage:

_1033569176_

But it's not crashing\. Anyway, I enjoyed experimenting with it\. If you wish, you can continue to experiment and share your observations\.

## Another typos

**A classic typo: calling the _hasText _function for the second time**

```cpp
void SheetTableView::contextMenuEvent(QContextMenuEvent*)
{
  const QMimeData* mimeData = QApplication::clipboard()->mimeData();
  ....
  actionPaste->setEnabled(
    mimeData && (mimeData->hasText() || mimeData->hasText()));
  ....
}
```

The PVS\-Studio warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) \[CWE\-570\] There are identical sub\-expressions 'mimeData\-\>hasText\(\)' to the left and to the right of the '\|\|' operator\. SheetTableView\.cpp 1115

The code double\-checks that the object can return plain text\. I can't say for sure how to write the code correctly\. I would guess that the _hasHtml _function should be called instead of one of the _hasText_ functions\.

**A typo in the last line**

```cpp
void Document::handleChildren3D(ViewProvider* viewProvider, bool deleting)
{
  ....
  std::vector<App::DocumentObject*> children = viewProvider->claimChildren3D();

  SoGroup* childGroup = viewProvider->getChildRoot();
  SoGroup* frontGroup = viewProvider->getFrontRoot();
  SoGroup* backGroup = viewProvider->getFrontRoot();

  if (deleting ||
      childGroup->getNumChildren() != static_cast<int>(children.size())) {
  ....
}
```

The analyzer indicates an anomaly in the code with two warnings at the same time:

* [V525](https://pvs-studio.com/en/docs/warnings/v525/) \[CWE\-682\] The code contains the collection of similar blocks\. Check items 'getChildRoot', 'getFrontRoot', 'getFrontRoot' in lines 2404, 2405, 2406\. Document\.cpp 2404
* [V656](https://pvs-studio.com/en/docs/warnings/v656/) \[CWE\-665\] Variables 'frontGroup', 'backGroup' are initialized through the call to the same function\. It's probably an error or un\-optimized code\. Consider inspecting the 'viewProvider\-\>getFrontRoot\(\)' expression\. Check lines: 2405, 2406\. Document\.cpp 2406

I think we are facing a demonstration of [the last line effect](https://pvs-studio.com/en/blog/posts/cpp/0260/): the error is most likely to be made at the end of code lines/blocks of the same type\.

The code contains three lines that are very similar to each other\. Most likely, a developer copy\-pasted this code fragment and accidentally duplicated the second line:

```cpp
SoGroup* frontGroup = viewProvider->getFrontRoot();
```

The developers need to replace "front" with "back" in the copied line\. There should have been two such replacements in the line, but only one was made\.

```cpp
SoGroup* backGroup = viewProvider->getFrontRoot(); // incorrect
SoGroup* backGroup = viewProvider->getBackRoot();  // correct
```

These typos are hard to find during the code reviews, and static analysis can become a very good assistant\.

**Freddy's time**

![1072_FreeCAD/image2.png](https://import.viva64.com/docx/blog/1072_FreeCAD/image2.png)

What's Freddy got to do with it? This is a reference to the article: "[Zero, one, two, Freddy's coming for you](https://pvs-studio.com/en/blog/posts/cpp/0713/)"\. Typos are attracted to code when variable names contain 0, 1, or 2\. That's exactly such a case\.

```cpp
TopoDS_Edge GeometryUtils::asCircle(TopoDS_Edge occEdge, bool& arc) { .... }

bool GeometryMatcher::compareBSplines(TopoDS_Edge &edge1, TopoDS_Edge &edge2)
{
  ....
  BRepAdaptor_Curve adapt1(edge1);
  BRepAdaptor_Curve adapt2(edge2);
  bool isArc1(false);
  bool isArc2(false);
  TopoDS_Edge circleEdge1;
  TopoDS_Edge circleEdge2;
  try {
    circleEdge1 = GeometryUtils::asCircle(edge1, isArc1);
    circleEdge2 = GeometryUtils::asCircle(edge2, isArc1);
  }
  catch (Base::RuntimeError&) {
    Base::Console().Error(....);
    return false;
  }
  if (!isArc1 && !isArc2) {
    return compareCircles(circleEdge1, circleEdge2);
  }
  if (isArc1 && isArc2) {
    return compareCircleArcs(circleEdge1, circleEdge2);
  }

  return false;
}
```

The PVS\-Studio warning: [V537](https://pvs-studio.com/en/docs/warnings/v537/) \[CWE\-682\] Consider reviewing the correctness of 'isArc1' item's usage\. GeometryMatcher\.cpp 242

It's a large piece of code\. But that's OK, we'll discuss it bit by bit\. Let's start with the _asCircle_ function\.

```cpp
TopoDS_Edge GeometryUtils::asCircle(TopoDS_Edge occEdge, bool& arc) { .... }
```

Note that the function receives the second argument by reference to return the edge type information through it\.

Now look at the following code:

```cpp
bool isArc1(false);
bool isArc2(false);
....
circleEdge1 = GeometryUtils::asCircle(edge1, isArc1);
circleEdge2 = GeometryUtils::asCircle(edge2, isArc1);
```

A call to the _asCircle_ function should have initialized 4 variables:

1. _circleEdge1_
1. _isArc1_
1. _circleEdge2_
1. _isArc2_\.

A typo causes reusing of the _isArc1 _variable in the second line\. As a result, the _isArc1 _variable is initialized with the wrong value, and the _isArc2 _variable remains always equal to _false_\.

So, further checks are pointless \(they don't work as intended\):

```cpp
if (!isArc1 && !isArc2) {
....
if (isArc1 && isArc2) {
```

## Null pointers

**Test your attention**

Try to find a bug in this code yourself\. Just for fun\.

```cpp
void QGIViewPart::highlightMoved(QGIHighlight* highlight, QPointF newPos)
{
  std::string highlightName = highlight->getFeatureName();
  App::Document* doc = getViewObject()->getDocument();
  App::DocumentObject* docObj = doc->getObject(highlightName.c_str());
  auto detail = dynamic_cast<DrawViewDetail*>(docObj);
  auto oldAnchor = detail->AnchorPoint.getValue();
  if (detail) {
    Base::Vector3d delta = Rez::appX(DrawUtil::toVector3d(newPos)) /
                           getViewObject()->getScale();
    delta = DrawUtil::invertY(delta);
    detail->AnchorPoint.setValue(oldAnchor + delta);
  }
}
```

I put a picture here, so you don't look at the answer right away\. Our unicorn is waiting for your answer\.

![1072_FreeCAD/image3.png](https://import.viva64.com/docx/blog/1072_FreeCAD/image3.png)

Indeed, the error isn't very complicated, and I think you've found it\. However, such a search is quite boring\. That's why such "monolith\-like" code is hard to check during code reviews\. The point is that the _detail_ pointer is dereferenced until it's checked\.

```cpp
auto oldAnchor = detail->AnchorPoint.getValue();
if (detail) {
```

Sometimes the analyzer reports some issues in two ways\. This is just such a case:

1. [V595](https://pvs-studio.com/en/docs/warnings/v595/) \[CWE\-476, CERT\-EXP12\-C\] The 'detail' pointer was utilized before it was verified against nullptr\. Check lines: 842, 843\. QGIViewPart\.cpp 842\. Actually, the name of the diagnostic rule helps us understand why it's issued here\. If not, here's [a description of the V595 diagnostic rule](https://pvs-studio.com/en/blog/posts/cpp/0353/)\.
1. [V522](https://pvs-studio.com/en/docs/warnings/v522/) \[CWE\-690, CERT\-MEM52\-CPP\] There might be dereferencing of a potential null pointer 'detail'\. QGIViewPart\.cpp 842\. The meaning is as follows: the _dynamic\_cast_ operator may return a null pointer\. Therefore, it's dangerous to dereference it without preliminary check\.

If you want to fix the error, just move the declaration and initialization of the _oldAnchor_ variable inside the condition\. Here is the fixed code:

```cpp
if (detail) {
  auto oldAnchor = detail->AnchorPoint.getValue();
  Base::Vector3d delta = Rez::appX(DrawUtil::toVector3d(newPos)) /
                         getViewObject()->getScale();
  delta = DrawUtil::invertY(delta);
  detail->AnchorPoint.setValue(oldAnchor + delta);
}
```

**Dangerous constructor**

```cpp
TaskTransformedParameters::TaskTransformedParameters(
  ViewProviderTransformed *TransformedView, QWidget *parent)
    : TaskBox(...., TransformedView->menuName, true, parent)    // <=
    , proxy(nullptr)
    , TransformedView(TransformedView)
    , parentTask(nullptr)
    , insideMultiTransform(false)
    , blockUpdate(false)
{
  selectionMode = none;

  if (TransformedView) {                                        // <=
     Gui::Document* doc = TransformedView->getDocument();
     this->attachDocument(doc);
  }
  ....
}
```

The PVS\-Studio warning: [V664](https://pvs-studio.com/en/docs/warnings/v664/) \[CWE\-476, CERT\-EXP34\-C\] The 'TransformedView' pointer is being dereferenced on the initialization list before it is verified against null inside the body of the constructor function\. Check lines: 57, 66\. TaskTransformedParameters\.cpp 57

A _TransformedView_ pointer is passed to the constructor\. It's dereferenced when constructing the base class:

```cpp
: TaskBox(...., TransformedView->menuName, ....)
```

This is dangerous because this pointer may be null\. Of course, the analyzer doesn't react to every dereference of an unchecked pointer as dangerous\. If it does, there will be a lot of false positives \([explanation](https://pvs-studio.com/en/blog/posts/cpp/0488/)\)\. The analyzer infers that the pointer is null, because it finds this check in the construct body:

```cpp
if (TransformedView)
```

**The wrong pointer is checked**

```cpp
void SectionCut::startCutting(bool isInitial)
{
  ....
  App::PropertyLinkList* CutLinkList =
    dynamic_cast<App::PropertyLinkList*>(
      CutCompoundBF->getPropertyByName("Objects"));

  if (!CutCompoundBF) {
    Base::Console().Error((std::string("SectionCut error: ")
                           + std::string(CompoundName)
                           + std::string(" could not be added\n")).c_str());
    return;
  }
  CutLinkList->setValue(ObjectsListLinks);
  ....
}
```

The analyzer again issues two different warnings to notify developers about anomalies:

* [V595](https://pvs-studio.com/en/docs/warnings/v595/) \[CWE\-476, CERT\-EXP12\-C\] The 'CutCompoundBF' pointer was utilized before it was verified against nullptr\. Check lines: 690, 691\. SectionCutting\.cpp 690
* [V1051](https://pvs-studio.com/en/docs/warnings/v1051/) \[CWE\-754\] Consider checking for misprints\. It's possible that the 'CutLinkList' should be checked here\. SectionCutting\.cpp 691

The first warning is implicit\. It points to these lines of code:

```cpp
.... CutCompoundBF->getPropertyByName("Objects"));
if (!CutCompoundBF) {
```

It's strange to dereference a pointer at the beginning and then check it for equality with _nullptr_\.

The second warning immediately shows us that we might check another variable instead of the _CutCompoundBF_ variable\.

Indeed, the _CutLinkList_ variable should be checked in the condition\. Then the anomalies will disappear\. Here is the fixed code:

```cpp
App::PropertyLinkList* CutLinkList =
  dynamic_cast<App::PropertyLinkList*>(
    CutCompoundBF->getPropertyByName("Objects"));

if (!CutLinkList) {
  Base::Console().Error(....);
  return;
}
CutLinkList->setValue(ObjectsListLinks);
```

## Inefficient code blocks

To optimize the program performance, it would be better to use profilers\. Static analyzers can't predict which code will waste the most CPU time\. It depends on the input data, and static analyzers don't know about that\.

Nevertheless, we can micro\-optimize something in the code using a static analyzer\. However, there is no guarantee that these optimizations will provide higher performance of the program\. At least it won't get any worse either\.

Here are some examples of what warning is in terms of micro\-optimizations\.

For example, the FreeCAD project has a lot of redundant string conversions\. Here's one of them:

```cpp
void TaskAttacher::onRefName(const QString& text, unsigned idx)
{
  std::string subElement;
  ....
  std::vector<std::string> refnames = pcAttach->Support.getSubValues();
  ....
  refnames[idx] = subElement.c_str();
  ....
}
```

The PVS\-Studio warning: [V811](https://pvs-studio.com/en/docs/warnings/v811/) Decreased performance\. Excessive type casting: string \-\> char \* \-\> string\. Consider inspecting the 'subElement\.c\_str\(\)' expression\. TaskAttacher\.cpp 672

It's inefficient to copy a _std::string_ from a C string \(null\-terminated string\)\. We have to reread the C string to find out its length and allocate the necessary memory buffer\. Although the _subElement_ contains the length information\.

In general, you can write shorter code that will operate faster:

```cpp
refnames[idx] = subElement;
```

There are still many redundant variables in the project\. They are created and initialized, but are not used anywhere\. This is most likely a side effect of multiple iterations of code changes and refactoring\. Suddenly, the variable has become needless for the developers, but they forget to remove it from the function\.

Here is the example:

```cpp
void TaskFilletParameters::apply()
{
  std::string name = getDressUpView()->getObject()->getNameInDocument();

  ui->filletRadius->apply();

  if(ui->listWidgetReferences->count() == 0)
    Base::Console().Warning(
      tr("Empty fillet created !\n").toStdString().c_str());
}
```

The PVS\-Studio warning: [V808](https://pvs-studio.com/en/docs/warnings/v808/) 'name' object of 'basic\_string' type was created but was not utilized\. TaskFilletParameters\.cpp 186

The _name_ variable is obviously redundant\. However, there is no guarantee that the compiler will delete it at the optimization stage\. Strings are complex classes that allocate memory\. Memory allocation is a side effect that changes the external state\. The compiler has difficulties to understand whether all this complex construct is redundant or not\.

Starting with C\+\+14, the compiler [can](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3664.html#expr.new) perform the optimization and delete a variable\. However, this is an optional optimization\. New compilers perform it, but we should keep in mind that the code complexity plays an important role\.

Anyway, it makes no sense to clutter the program code with unnecessary strings\.

Here is another example:

```cpp
void DlgRevertToBackupConfigImp::showEvent(QShowEvent* event)
{
  ....
  for (const auto& backup : backups) {
    auto filename = backup.filename().string();
    auto modification_date =
      QDateTime::fromSecsSinceEpoch(fs::last_write_time(backup));
    auto item = new QListWidgetItem(QLocale().toString(modification_date));
    item->setData(Qt::UserRole, QString::fromStdString(backup.string()));
    ui->listWidget->addItem(item);
  }
  ....
}
```

The PVS\-Studio warning: [V808](https://pvs-studio.com/en/docs/warnings/v808/) 'filename' object of 'basic\_string' type was created but was not utilized\. DlgRevertToBackupConfigImp\.cpp 79

Here, the _filename_ variable is needlessly copied many times and destroyed in the loop\.

P\.S\. By the way, sometimes when we're reviewing code with such "lost" variables, we suddenly find out that the code contains an error and the variable should have actually been used\. I can't say it's a common situation, but it does happen\.

## Other errors and dangerous code blocks

Indeed, I didn't describe all the errors and dangerous code snippets in this article\. I was glancing through the warnings until I had enough content for an article\. If the project developers or someone else wish to thoroughly check a project, they can run the analysis themselves\. We provide [a free license](https://pvs-studio.com/en/blog/posts/0614/) for the most of open\-source projects\.

As I said at the beginning of the article, it's better to integrate analysis into the development process rather than do one\-off checks\. When you first start using the analyzer, there might be many warnings and it can be daunting\. There's really nothing to worry about\. The solution already exists\. I invite you to read the note "[How to introduce a static code analyzer in a legacy project and not to discourage the team](https://pvs-studio.com/en/blog/posts/0743/)"\.

So, let's break down a few more warnings\.

**Alignment: walking on thin ice**

```cpp
template <int N>
void TRational<N>::ConvertTo (double& rdValue) const
{
  assert(m_kDenom != 0);
  if (m_kNumer == 0)
  {
    rdValue = 0.0;
    return;
  }

  unsigned int auiResult[2];
  ....
  rdValue = *(double*)auiResult;
  ....
}
```

The PVS\-Studio warning: [V1032](https://pvs-studio.com/en/docs/warnings/v1032/) \[CWE\-843, CERT\-EXP36\-C\] The pointer 'auiResult' is cast to a more strictly aligned pointer type\. Wm4TRational\.inl 742

An array of the two elements of the _unsigned_ _int_ type is used as a variable of the _double_ type\. The _unsigned int_ type is aligned to a 4\-byte boundary and the _double_ type is aligned to an 8\-byte boundary\. If the array is not aligned to an 8\-byte boundary, undefined behavior will occur\.

This code violates the [strict aliasing](https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing) rule, which causes undefined behavior\. However, if we're lucky, this code may work\. Since there aren't other variables in front of the array, there is a high probability that it will be 8 bytes aligned\. Therefore, everything will work as the programmer expects\. At least for now\.

However, such delicate operability is easy to break\. Declare another variable before the array, and that's enough\.

```cpp
template <int N>
void TRational<N>::ConvertTo (double& rdValue) const
{
  assert(m_kDenom != 0);
  if (m_kNumer == 0)
  {
    rdValue = 0.0;
    return;
  }

  int fooooo;                // Variable is added 
  unsigned int auiResult[2]; // Alignment is not multiple of 8 bytes
  ....
  rdValue = *(double*)auiResult;
  ....
}
```

If you are interested in the topic, read this scary and educational bedtime story: "[A bug story: data alignment on x86](http://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html)"\.

Okay, how to make it right?

Before C\+\+20, you could use _memcpy_\. Don't be afraid: the compiler perfectly [optimizes](https://godbolt.org/z/1Yj1eshdK) the code\. Starting with C\+\+20, use [std::bit\_cast](https://en.cppreference.com/w/cpp/numeric/bit_cast)\. If you wish to find something useful, you can [subscribe](https://pvs-studio.com/en/subscribe/) to our newsletter\.

**Unsafe array index check**

First, let's look at the _getIndexFromName_ auxiliary function\.

```cpp
int DrawUtil::getIndexFromName(const std::string& geomName)
{
  ....
  if (boost::regex_search(begin, end, what, re, flags)) {
    return int(std::stoi(what.str()));
  } else {
    ErrorMsg << "getIndexFromName: malformed geometry name - " << geomName;
    throw Base::ValueError(ErrorMsg.str());
  }
}
```

The function converts a string fragment into a number \(index\)\. Now let's go ahead through the code that uses this function\.

```cpp
TechDraw::VertexPtr DrawViewPart::getVertex(std::string vertexName) const
{
  const std::vector<TechDraw::VertexPtr>
    allVertex(DrawViewPart::getVertexGeometry());

  size_t iTarget = DrawUtil::getIndexFromName(vertexName);
  if (allVertex.empty()) {
    //should not happen
    throw Base::IndexError("DVP::getVertex - No vertices found.");
  }
  if (iTarget > allVertex.size()) {                                // <=
    //should not happen
    throw Base::IndexError("DVP::getVertex - Vertex not found.");
  }

  return allVertex.at(iTarget);                                    // <=
}
```

The PVS\-Studio warning: [V557](https://pvs-studio.com/en/docs/warnings/v557/) \[CWE\-119, CERT\-ARR30\-C\] Array overrun is possible\. The 'iTarget' index is pointing beyond array bound\. DrawViewPart\.cpp 809

Technically, the index values should always be correct\. However, developers protected the code just in case\. Unfortunately, there is a slight error\. The bound value isn't handled correctly:

```cpp
if (iTarget > allVertex.size()) {
```

Suppose there are 100 elements in the vector\. In this case, the maximum correct index should be equal to the value 99, instead of 100\. This is how a correct check should look like:

```cpp
if (iTarget >= allVertex.size()) {
```

**Risk of infinite loop when reading a file**

```cpp
inline uint32 readUint32 ( istream &is ) {
  static const int buf_len = sizeof ( uint32 ) ;
  unsigned char buf [ buf_len ] ;
  int rsf = 0 ;
  std::streampos original_pos = is.tellg() ;
  while ( rsf < buf_len && !is.eof() ) {
    is.read ( reinterpret_cast< char * >( buf ) + rsf, buf_len - rsf ) ;
    rsf += static_cast< int >( is.gcount () ) ;
  }
  ....
}
```

Before we take a look at the error, let's turn our attention to this line:

```cpp
static const int buf_len = sizeof ( uint32 ) ;
```

The _sizeof\(uint32\)_ string is the compile\-time const\. It's quite possible that the developer wanted to give a constant a neat name\. The developer is also worried that it would be computed every time when entered the function, so they declared it as _static_\.

On the other hand, _static_ may cause additional overhead costs\. The constant can be stored in the data section and computed at the runtime when the function is first entered\.

Much better is to use _constexpr_ \(C\+\+11\):

```cpp
constexpr size_t buf_len = sizeof ( uint32 );
```

Then the constant will have a name, and there won't be unnecessary actions\. The compiler will simply set the constant in the right places\.

Now back to the loop\. 

```cpp
while ( rsf < buf_len && !is.eof() )
```

The PVS\-Studio warning: [V663](https://pvs-studio.com/en/docs/warnings/v663/) \[CWE\-834\] Infinite loop is possible\. The 'cin\.eof\(\)' condition is insufficient to break from the loop\. Consider adding the 'cin\.fail\(\)' function call to the conditional expression\. zipheadio\.h 84

If there is some failure in reading the file, the loop can become infinite\. The end of the file hasn't been reached yet, but nothing is being read\. The [_fail_](https://en.cppreference.com/w/cpp/io/basic_ios/fail) function is used to check this situation\.

The way how the code is fixed depends on what behavior we want\. If the reading option is enough, we can write such a condition to stop the loop:

```cpp
while ( rsf < buf_len && !is.eof() && !is.fail() ) {
```

However, such a record isn't very nice\. Instead, you can use the [_good_](https://en.cppreference.com/w/cpp/io/basic_ios/good) function as an option:

```cpp
while ( rsf < buf_len && is.good() ) {
```

Note that this is just one way to avoid an infinite loop if you can't read the entire file\. That doesn't mean that the code is correctly fixed\.

It all depends on how a program is designed and how it should react to the incapability to read the entire file\. Some program may continue to handle some part of the data, another may generate an error message, and so on\.

If you wish to interact with the input stream and find out that something went wrong while reading, there is a whole set of functions\. You can find a good table on this topic on the [cppreference](https://en.cppreference.com/w/cpp/io/basic_ios/good) website\.

![1072_FreeCAD/image4.png](https://import.viva64.com/docx/blog/1072_FreeCAD/image4.png)

As you can see, there are various ways of how to implement reading from a stream and how to handle the reading errors\.

For the code under consideration, reading only a part of the data doesn't look like a correct scenario\. Therefore, here's how the code can be changed:

```cpp
while (   rsf < buf_len
       && is.read(reinterpret_cast< char * >( buf ) + rsf, buf_len - rsf))
{
  rsf += static_cast< int >( is.gcount () ) ;
}

if (is.fail())
  throw Error("Unable to read file.");
```

## It's time to check your projects

This time I decided not to write a special conclusion\. Here's one thing I'd say — static analysis is useful\. That's all :\)\.

So, let's get to the point\. If you haven't evaluated PVS\-Studio yet, I suggest you [download](https://pvs-studio.com/en/pvs-studio/download/) and install it now\. Try to check your code or the code of your colleagues\. You may find many interesting things\.

![1072_FreeCAD/image5.png](https://import.viva64.com/docx/blog/1072_FreeCAD/image5.png)

Lifehack\. Try the "[10 most interesting warnings](https://pvs-studio.com/en/docs/manual/6532/)" mode to get acquainted with the analyzer\.