Our website uses cookies to enhance your browsing experience.
Accept
to the top
close form

Fill out the form in 2 simple steps below:

Your contact information:

Step 1
Congratulations! This is your promo code!

Desired license type:

Step 2
Team license
Enterprise license
** By clicking this button you agree to our Privacy Policy statement
close form
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
* By clicking this button you agree to our Privacy Policy statement

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

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

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

close form
check circle
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.

>
>
>
FreeCAD and undefined behavior in C++ c…

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

Oct 18 2023
Author:

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

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

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

  • 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.
  • You have already fixed some errors when debugging. However, you could have fixed them at the compile time if you had enabled warnings.
  • You won't understand what to do with some warnings. For example, they point to redundant code 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 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 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 it is.

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

Typo and undefined behavior

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

The PVS-Studio warning: 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 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". And here is the code:

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

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

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.

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

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

The PVS-Studio warning: 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

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 [CWE-682] The code contains the collection of similar blocks. Check items 'getChildRoot', 'getFrontRoot', 'getFrontRoot' in lines 2404, 2405, 2406. Document.cpp 2404
  • 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: 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:

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.

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

What's Freddy got to do with it? This is a reference to the article: "Zero, one, two, Freddy's coming for you". Typos are attracted to code when variable names contain 0, 1, or 2. That's exactly such a case.

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

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:

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:

  • circleEdge1
  • isArc1
  • circleEdge2
  • 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):

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

Null pointers

Test your attention

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

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

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.

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

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

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

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

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

: 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). The analyzer infers that the pointer is null, because it finds this check in the construct body:

if (TransformedView)

The wrong pointer is checked

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 [CWE-476, CERT-EXP12-C] The 'CutCompoundBF' pointer was utilized before it was verified against nullptr. Check lines: 690, 691. SectionCutting.cpp 690
  • 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:

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

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:

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

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:

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

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 '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 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".

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

Alignment: walking on thin ice

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

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

Okay, how to make it right?

Before C++20, you could use memcpy. Don't be afraid: the compiler perfectly optimizes the code. Starting with C++20, use std::bit_cast. If you wish to find something useful, you can subscribe to our newsletter.

Unsafe array index check

First, let's look at the getIndexFromName auxiliary function.

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.

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

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:

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

Risk of infinite loop when reading a file

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:

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

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.

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

The PVS-Studio warning: 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 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:

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

However, such a record isn't very nice. Instead, you can use the good function as an option:

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

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:

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

Lifehack. Try the "10 most interesting warnings" mode to get acquainted with the analyzer.

Popular related articles


Comments (0)

Next comments next comments
close comment form