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 do not see the email in your inbox, please check if it is filtered to one of the following folders:

  • Promotion
  • Updates
  • Spam

Webinar: Evaluation - 05.12

>
>
>
Qt Creator* in search of Qt Creator bugs

Qt Creator* in search of Qt Creator bugs

Feb 01 2024

Strange things happen: for almost a year, PVS-Studio has a plugin for Qt Creator. However, we haven't yet published a good old article with the IDE check. We've made amends and invite you to see how the recently revamped development environment lives.

1099_qtcreator12_check/image1.png

Briefly about Qt Creator and article

Although Qt Creator is a pretty well-known IDE, it's not so well-known to skip over its brief description. Qt Creator is a cross-platform development environment for C++ applications that use the Qt framework. It provides a user-friendly interface for writing, debugging, and deploying apps. Also, Qt Creator has tools for working with the code editor, version control system, and other features. Qt Creator supports different operating systems: Windows, macOS, and Linux. Given that Qt has recently bought a company-developer of a static analyzer and integrated the tool into its IDE, it's very interesting to take a look at Qt Creator and see how good its internal analysis is :)

Why did I point out a "revamp" in the article annotation? It's simple: starting with the 5 version, developers drastically redesigned the IDE to meet modern standards. As a developer, I enjoy these changes. In many cases, Qt Creator is no worse, and in some cases, it's even better than Visual Studio and VS Code (it's my point of view; if you want to talk about it, you're welcome to comments). Of course, there is something we can scold the developers for (such us the API support for third-party developers) but perhaps not today and not here.

  • To simplify the readability, I change the code that's beside the point to .... (ellipsis) and the code issues using the // <= comments.
  • Sometimes code blocks look so-so in the article, so I have to play with formatting without changing the code logic (only where the formatting is unimportant to the narrative).
  • If you wish to check out the original, each code fragment has a permalink to the developers' repository.
  • I've checked the latest available release version (12.0.0) from GitHub. The link goes to the commit, because tags were often missing.
  • The article mentions the bugs that I have found to be odd (yes, it's only a matter of taste). If you'd like to look at the rest of errors, you can always download the analyzer and check the project on your own. And maybe check your own code too. ;-).

Preparing and running analysis

Have I already mentioned that there is a PVS-Studio extension for Qt Creator, right? That's why, I have used an asterisk with Qt Creator in the title. So, today, we're going to check the code of Qt Creator in Qt Creator using the PVS-Studio plugin for Qt Creator. What "ingredients" do we need? The recipe is pretty simple:

  • Download the "fresh" PVS-Studio distribution kit (7.28 at the moment of writing this article).
  • Install the extension using the guide.
  • Get a license at the link and enter it in the settings.
  • Download the newest Qt Creator source code from GitHub.
  • Open the project, build it, and run the analysis.
  • Disable unnecessary diagnostic rules (e.g. V1042) and filter out warnings for the third-party code.
  • PROFIT! Let's "taste" the errors found.

If you follow the recipe, you'll "cook" the following:

1099_qtcreator12_check/image2.png

Bugs are coming

Memory leak

Let's start with the most absorbing things—memory leaks. To those who's off topic, I'll briefly explain the issue's crux. Qt has a non-trivial object ownership system, and to understand that there is no memory leak, you need to keep in mind that there are many ways to both create objects and transfer ownership (hello, setParent and layout's). I'll be honest: there have been plenty of false positives. However, I've found some interesting cases among the warnings. I suggest you take a look at them while we're enhancing our analyzer to reduce false positives. :)

Let's start with a small memory leak in the function for getting paths to the IDE crash reports.

The file: main.cpp:1813

QString crashReportsPath()
{
  std::unique_ptr<Utils::QtcSettings> settings(createUserSettings());
  QFileInfo(settings->fileName()).path() + "/crashpad_reports";
  if (Utils::HostOsInfo::isMacHost())
    return QFileInfo(createUserSettings()->fileName()).path() +
                     "/crashpad_reports";
  else
    return QCoreApplication::applicationDirPath() + '/' +
           RELATIVE_LIBEXEC_PATH + "crashpad_reports";
}

If the reuse of the createUserSettings function confused you, congrats — you have a practiced eye for finding bugs. You're headed in the right direction. For the first time, the pointer obtained from the function is saved to std::unique_ptr and is correctly deleted when leaving the scope. For the second time, in the macOS branch, the pointer is forgotten after getting the path to the file. The analyzer report shows that:

V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] The return value of the 'createUserSettings' function is not saved. A memory leak is possible. main.cpp 422

You may ask me, "Are you sure there is trouble?" Yes, I'm sure. The createUserSettings implementation looks like this:

The file: main.cpp:274

static Utils::QtcSettings *createUserSettings()
{
    return new Utils::QtcSettings(
        QSettings::IniFormat,
        QSettings::UserScope,
        QLatin1String(Core::Constants::IDE_SETTINGSVARIANT_STR),
        QLatin1String(Core::Constants::IDE_CASED_ID));
}

I also found two cases where objects aren't assigned the parent objects, which are responsible for their deletion, while developers just forgot to delete them. The first case is quite simple: developers create an object but forget to delete it here, although everything is fine in other places.

The file: debuggerplugin.cpp:1813

void DebuggerPlugin::attachToProcess(....)
{
  ....
  auto kitChooser = new KitChooser;
  kitChooser->setShowIcons(true);
  kitChooser->populate();
  Kit *kit = kitChooser->currentKit();

  dd->attachToRunningProcess(kit, processInfo, false);
}

In the second case, a model is created in the class constructor without parent. It would be better that only the model is deleted in the destructor, but alas, its implementation is empty. Developers might want to use any smart pointer but forgot about it...

The file: curveeditorview.cpp:43

CurveEditorView::CurveEditorView(
  ExternalDependenciesInterface &externalDepoendencies
)
  : AbstractView{externalDepoendencies}
  , m_block(false)
  , m_model(new CurveEditorModel())
  , m_editor(new CurveEditor(m_model))
{
  ....
}

CurveEditorView::~CurveEditorView() {}

Here are the corresponding warnings:

  • V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'kitChooser' pointer was exited without releasing the memory. A memory leak is possible. debuggerplugin.cpp 1825
  • V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] The 'm_model' pointer was not released in destructor. A memory leak is possible. curveeditorview.cpp 43

Classic of genre

"Next, I suggest you look at a classic of the genre", I'd say, and then show the dereferencing of non-checked pointers, however, there is one thing. We have a project that actively uses C++17 and std::optional, and that's great. But, unfortunately, the errors are called classical because they don't depend on the tools used. Will you try to find these errors by yourself? ;-)

The file: aspects.cpp:2189

void IntegerAspect::addToLayout(Layouting::LayoutItem &parent)
{
  QTC_CHECK(!d->m_spinBox);
  d->m_spinBox = createSubWidget<QSpinBox>();
  d->m_spinBox->setDisplayIntegerBase(d->m_displayIntegerBase);
  d->m_spinBox->setPrefix(d->m_prefix);
  d->m_spinBox->setSuffix(d->m_suffix);
  d->m_spinBox->setSingleStep(d->m_singleStep);
  d->m_spinBox->setSpecialValueText(d->m_specialValueText);
  if (d->m_maximumValue && d->m_maximumValue)
    d->m_spinBox->setRange(
      int(d->m_minimumValue.value() / d->m_displayScaleFactor),
      int(d->m_maximumValue.value() / d->m_displayScaleFactor)
    );

  // Must happen after setRange()
  d->m_spinBox->setValue(int(value() / d->m_displayScaleFactor)); 
  addLabeledItem(parent, d->m_spinBox);
  connect(d->m_spinBox.data(), &QSpinBox::valueChanged,
          this, &IntegerAspect::handleGuiChanged);
}
1099_qtcreator12_check/image3.png

If you found the m_maximumValue reuse instead of m_minimumValue, congrats! It wasn't that easy, was it? I think the Qt Creator developers would agree too, because I found the same code a little further down in the same file:

The file: aspects.cpp:2291

void DoubleAspect::addToLayout(LayoutItem &builder)
{
  QTC_CHECK(!d->m_spinBox);
  d->m_spinBox = createSubWidget<QDoubleSpinBox>();
  d->m_spinBox->setPrefix(d->m_prefix);
  d->m_spinBox->setSuffix(d->m_suffix);
  d->m_spinBox->setSingleStep(d->m_singleStep);
  d->m_spinBox->setSpecialValueText(d->m_specialValueText);
  if (d->m_maximumValue && d->m_maximumValue)
    d->m_spinBox->setRange(d->m_minimumValue.value(),
                           d->m_maximumValue.value());

  bufferToGui(); // Must happen after setRange()!
  addLabeledItem(builder, d->m_spinBox);
  connect(d->m_spinBox.data(), &QDoubleSpinBox::valueChanged,
          this, &DoubleAspect::handleGuiChanged);
}

By the way, do you often pay attention to what IntelliSense (or its analog) recommends you use? I think that's what caused the error. Before, I often caught myself writing the name of a variable, promptly hitting Enter, and moving on — there was no "Stop" sign for my thought. If it sounds familiar, share your experience in the comments. So we can see how many of us do it too. :)

Oh, of course, here are the warnings:

  • V501 [CWE-571] There are identical sub-expressions to the left and to the right of the '&&' operator: d->m_maximumValue && d->m_maximumValue aspects.cpp 2198
  • V501 [CWE-571] There are identical sub-expressions to the left and to the right of the '&&' operator: d->m_maximumValue && d->m_maximumValue aspects.cpp 2299

Test your focus

I suggest you do the eye exercise again and search for an error in the function for filtering compiler flags. Well, I can't let you go without a clue: "noitcnuf erapmoc eht ni setacilpud rof kooL". Yeah, it's not that easy.

The file: gcctoolchain.cpp:474

// For querying operations such as -dM
static QStringList filteredFlags(const QStringList &allFlags,
                                 bool considerSysroot)
{
  QStringList filtered;
  for (int i = 0; i < allFlags.size(); ++i)
  {
    const QString &a = allFlags.at(i);
    if (a.startsWith("--gcc-toolchain="))
    {
      filtered << a;
    }
    else if (a == "-arch")
    {
      if (++i < allFlags.length() && !filtered.contains(a))
        filtered << a << allFlags.at(i);
    }
    else if (a == "-Xclang")
    {
      filtered << a;
    }
    else if (   (considerSysroot && (a == "--sysroot" || a == "-isysroot"))
             || a == "-D" || a == "-U"
             || a == "-gcc-toolchain"
             || a == "-target"
             || a == "-mllvm"
             || a == "-isystem")
    {
      if (++i < allFlags.length())
        filtered << a << allFlags.at(i);
    }
    else if (  a.startsWith("-m") || a.startsWith("-f") || a.startsWith("-O")
             || a.startsWith("-std=") || a.startsWith("-stdlib=")
             || a.startsWith("-specs=") || a == "-ansi" || a == "-undef"
             || a.startsWith("-D") || a.startsWith("-U")
             || a.startsWith("-stdlib=") || a.startsWith("-B")
             || a.startsWith("--target=")
             || (a.startsWith("-isystem") && a.length() > 8)
             || a == "-Wno-deprecated"
             || a == "-nostdinc" || a == "-nostdinc++")
    {
      filtered << a;
    }
  }

  return filtered;
}

If you can't immediately find an unnecessary check for the "-stdlib" substring, don't be upset because static analyzers were created to help us here. If you can, then congratulations! You have a keen eye.

Alas, the gifts are given at the end of the article, so I've asked an AI to generate a medal for the true detective programmer:

1099_qtcreator12_check/image4.png

I agree, it a bit lacks a programmer's spirit. However, it turned out pretty good. Oh, and I almost forgot about the analyzer warning:

V501 [CWE-570] There are identical sub-expressions 'a.startsWith("-stdlib=")' to the left and to the right of the '||' operator. gcctoolchain.cpp 491.

Unreachable code

Honestly, I really wanted to paste the full code of this function here (which has almost 400 code lines) to make looking for errors more interesting. But I don't want to abuse your mouse wheel, so I brought only curious part.

The file: symbolgroupvalue.cpp:1196

static KnownType knownClassTypeHelper(const std::string &type,
                                      std::string::size_type pos,
                                      std::string::size_type endPos)
{
  ....
  // Remaining non-template types
  switch (endPos - qPos)
  {
    ....
  case 30:
    if (!type.compare(qPos, 30, "QPatternist::SequenceType::Ptr"))
      return KT_QPatternist_SequenceType_Ptr;
    if (!type.compare(qPos, 30, "QXmlStreamNamespaceDeclaration"))
      return KT_QXmlStreamNamespaceDeclaration;
    break;
  case 32:
    break;                                                                // <=
    if (!type.compare(qPos, 32, "QPatternist::Item::Iterator::Ptr"))
      return KT_QPatternist_Item_Iterator_Ptr;
  case 34:
    break;                                                                // <=
    if (!type.compare(qPos, 34, "QPatternist::ItemSequenceCacheCell"))
      return KT_QPatternist_ItemSequenceCacheCell;
  case 37:
    break;                                                                // <=
    if (!type.compare(qPos, 37, "QNetworkHeadersPrivate::RawHeaderPair"))
      return KT_QNetworkHeadersPrivate_RawHeaderPair;
    if (!type.compare(qPos, 37, "QPatternist::AccelTree::BasicNodeData"))
      return KT_QPatternist_AccelTree_BasicNodeData;
    break;
  }

  return KT_Unknown;
}

Please note the 32, 34, and 37 branches of the switch statement above. Honestly, I can't think of any reason for using break; right before the executable code.

I thought that they were just temporary kludges. But no, they added at the same time as this code. According to the history, this code is about 14 years old. If any readers can suggest what exactly is going on here — you're welcome in the comments.

The analyzer points you to this code fragment by issuing the warning:

V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. symbolgroupvalue.cpp 1565

Array overrun (aka modeling errors)

Writing the correct data models is one of the trickiest things for Qt newcomers. And for a good reason. There are a lot of ways to screw this up. There are so many of them that even the Qt Creator developers have been trapped :)

The file: cppquickfixes:4676

QVariant data(int column, int role) const override
{
  if (role == Qt::DisplayRole && column == NameColumn)
    return m_memberInfo->data.memberVariableName;
  if (   role == Qt::CheckStateRole && column > 0
      && column <= static_cast<int>(std::size(ColumnFlag)))
  {
    return m_memberInfo->requestedFlags & ColumnFlag[column] ? Qt::Checked :
                                                               Qt::Unchecked;
  }

  return {};
}

At first glance, you may think that the code is OK... But what if you look into the definition of the ColumnFlag enumeration? I added comments so you don't have to calculate yourself.

The file: cppquickfixes:4661

using Flag = GenerateGetterSetterOp::GenerateFlag;

constexpr static Flag ColumnFlag[] = {
  Flag::Invalid,                    // 0
  Flag::GenerateGetter,             // 1
  Flag::GenerateSetter,             // 2
  Flag::GenerateSignal,             // 3
  Flag::GenerateReset,              // 4
  Flag::GenerateProperty,           // 5
  Flag::GenerateConstantProperty,   // 6
};

I think this hint helps understand what the issue is: you can get the overrun of the ColumnFlag array if you pass column equal to 7 to the function. Let's thank for it the lurking <= in the maximum value check. The correct code looks like this:

if (   role == Qt::CheckStateRole 
    && column > 0
    && column < static_cast<int>(std::size(ColumnFlag)))
{
  ....
}

Do you remember that there are a lot of ways to screw up? In the same model, a check of the following form has also been copied several times:

The file: cppquickfixes:4693

bool setData(int column, const QVariant &data, int role) override
{
  if (column < 1 || column > static_cast<int>(std::size(ColumnFlag)))
    return false;
  if (role != Qt::CheckStateRole)
    return false;
  if (!(m_memberInfo->possibleFlags & ColumnFlag[column]))
    return false;
  ....
}

The error is the same, but now it is with the operator >. I've found all these things with the following warnings:

  • V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'column' index could reach 7. cppquickfixes.cpp 4682
  • V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'column' index could reach 7. cppquickfixes.cpp 4693
  • V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'column' index could reach 7. cppquickfixes.cpp 4697
  • V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'column' index could reach 7. cppquickfixes.cpp 4699
  • V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'column' index could reach 7. cppquickfixes.cpp 4735

If you still think that these are one-off issues, I'll drop the small snippet as a final example, and we'll move on.

The file: defaultannotations.cpp:27

QVariant DefaultAnnotationsModel::data(const QModelIndex &index, int role) const
{
  const auto row = static_cast<size_t>(index.row());
  if (!index.isValid() || m_defaults.size() < row)
    return {};

  auto &item = m_defaults[row];

  switch (role)
  {
    ....
  }

  return {};
}

V557. [CWE-119, CERT-ARR30-C] Array overrun is possible. The 'row' index is pointing beyond array bound. defaultannotations.cpp 33

Choice without choice

Almost all of us have a fav type of errors. For me, it's what's called "choice without choice" (Am I not the only one calling it that, though?) The point is that you can write as complex a condition as you want and still have the same result because of the code duplication in different branches. Qt Creator has delighted me with a couple of these issues.

1099_qtcreator12_check/image5.png

The file: nodemetainfo.cpp:1834

QString NodeMetaInfo::componentFileName() const
{
  if constexpr (!useProjectStorage())
  {
    if (isValid())
    {
      return m_privateData->componentFileName();
    }
  }
  else
  {
    if (isValid())
    {
      return m_privateData->componentFileName();
    }
  }

  return {};
}

Honestly, I don't know what the original concept of the code was, but I like the fact that the compiler deletes the irrelevant branch during the project build. Although it seems like nothing is wrong here, it's worth checking out the warning.

V523 [CWE-691] The 'then' statement is equivalent to the 'else' statement. nodemetainfo.cpp 1840

You can't tell for sure if it's an error or not, and this is really frustrating. Even nearby comments don't usually help. For example, as it's shown here:

The file: debuggerkitaspect.cpp:358

// We have an executable path.
FilePath fileName = FilePath::fromUserInput(binary);
if (item.command() == fileName)
{
  // And it's is the path of this item.
  level = std::min(item.matchTarget(tcAbi), DebuggerItem::MatchesSomewhat);
}
else
{
  // This item does not match by filename, and is an unlikely candidate.
  // However, consider using it as fallback if the tool chain fits.
  level = std::min(item.matchTarget(tcAbi), DebuggerItem::MatchesSomewhat);
}

V523 [CWE-691] The 'then' statement is equivalent to the 'else' statement. debuggerkitaspect.cpp 362

Here, the errors are really difficult to notice at all. That's why they can live in the code for a very long time. Such errors often occur due to banal copy-pasting or "local" refactoring.

Unfortunately, it's uneasy to fight with such errors. Errors can easily slip through code reviews even for experienced programmers. Do you not trust me, though? Well, your loss. My colleague just recently wrote a note about a little error that even several programmers didn't see at once. It's better to delegate the search for such errors to an analyzer. At least its roboeyes aren't blurred. :)

By the way, I'd like to know if you have any favorite or hated error patterns. Is that a trifle? Or is it a bug that needs hours to be caught? Share these patterns in the comments — it will be fun to read and discuss. :)

Operator precedence

Not a super cool bug, but I liked the context in which I found it. Look: there is an error in the precedence (of operations) in the code for checking the precedence (of something). There is even a ready-made error message. All the work was done for me. :)

The file: connectionmodel.cpp:2032

if (int firstError = checkOrder() != -1)
{
  setInvalid(tr("Invalid order at %1").arg(firstError), firstError);
  return;
}

Okay, not all of it. I still should tell you what's wrong here since the code doesn't look bad.

The trouble is in the condition: the 'A = B == C' expression is actually evaluated as 'A = (B == C)''. Especially here, the checkOrder function value is evaluated, and it returns the position where the error occurs. The value is compared to -1, and the result (0 or 1) is written to the variable. Shall I say that, in such a case, the error message will record a useless 1 instead of the normal position? Let's look at the warning:

V593 [CWE-783, CERT-EXP00-C] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. connectionmodel.cpp 2032

Since we have a C++17 project here, why not use the if statement with the initializer and rewrite the code in the following way:

if (int firstError = checkOrder(); firstError != -1)
{
  setInvalid(tr("Invalid order at %1").arg(firstError), firstError);
  return;
}

Now, you certainly can't mix up the operation precedence.

Ownerless expression

I suggest we should take a little break and look at a very simple error. Although it's a simple error, it has been living in the project code for more than 4 years. The point is that developers lost the return instruction or assignment to the boundingRect variable.

The file: quickitemnodeinstance.cpp:576

QRectF QuickItemNodeInstance::boundingRectWithStepChilds(
  QQuickItem *parentItem) const
{
  QRectF boundingRect = parentItem->boundingRect();

  boundingRect = boundingRect.united(QRectF(QPointF(0, 0), size()));

  for (QQuickItem *childItem : parentItem->childItems())
  {
    ....
  }

  if (boundingRect.isEmpty())
    QRectF{0, 0, 640, 480};    // <=

  return boundingRect;
}

V607 Ownerless expression 'QRectF { 0, 0, 640, 480 }'. quickitemnodeinstance.cpp 592

Incorrect use of pragma directives

Let's start with the code at once, because otherwise it may be unclear what we're talking about:

The file: common.h:15

#ifdef __clang__
#  pragma clang diagnostic push
#  pragma clang diagnostic ignored "-Wc++11-narrowing"
#  include <wdbgexts.h>
#  pragma clang diagnostic pop
#else
#  pragma warning( disable : 4838  )
#  include <wdbgexts.h>
#  pragma warning( default : 4838  )
#endif // __clang__

Here, developers try to suppress the compiler warning about narrowing conversions in a third-party header file. Nothing is wrong with it overall, but the suppression implementation raises some questions. The issue is that using #pragma warning(default : X) doesn't recover the warning state that was previously turned off with #pragma warning(disable: X). Instead, it returns it to the default state, which isn't always correct.

The analyzer properly warns about it to avoid issues:

V665 [CERT-MSC00-C] Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 21, 23. common.h 23

By the way, you can find a very good example of how to step on a rake in the documentation for the V665 diagnostic rule. I'll allow myself to quote it:

Imagine that a file is compiled with the /Wall switch used. The C4061 warning must be generated in this case. If you add the "#pragma warning(default : 4061)" directive, this warning will not be displayed, as it is turned off by default.

The most interesting thing is that, in the case of the Clang compiler, developers have done everything correctly—they've used the push/pop bindings. Let's make a small code fix, so we won't have issues with other compilers during the build:

#ifdef __clang__
#  pragma clang diagnostic push
#  pragma clang diagnostic ignored "-Wc++11-narrowing"
#  include <wdbgexts.h>
#  pragma clang diagnostic pop
#else
#  pragma warning( push )
#  pragma warning( disable : 4838 )
#  include <wdbgexts.h>
#  pragma warning( pop )
#endif // __clang__

Use of uninitialized class fields

It's one of the most frequent mistakes that C++ programmers make from time to time. Even if you read the title of the paragraph, maybe you look at the constructor of the Editor class and think, "What's wrong here...?"

The file: compilerexplorereditor.cpp:773

Editor::Editor(TextEditorActionHandler &actionHandler)
  : m_document(new JsonSettingsDocument(&m_undoStack))
{
  setContext(Core::Context(Constants::CE_EDITOR_ID));
  setWidget(new EditorWidget(m_document, &m_undoStack, actionHandler));

  connect(&m_undoStack, &QUndoStack::canUndoChanged, this,
          [&actionHandler] { actionHandler.updateActions(); });

  connect(&m_undoStack, &QUndoStack::canRedoChanged, this,
          [&actionHandler] { actionHandler.updateActions(); });
}

To be completely honest, I'll add a class declaration:

The file: compilerexplorereditor.h:237

class Editor : public Core::IEditor
{
public:
  Editor(TextEditor::TextEditorActionHandler &actionHandler);
  ~Editor();

  Core::IDocument *document() const override { return m_document.data(); }
  QWidget *toolBar() override;

  QSharedPointer<JsonSettingsDocument> m_document;
  QUndoStack m_undoStack;
  std::unique_ptr<QToolBar> m_toolBar;
};

Have you found it? If you have, shouldn't you challenge yourself in the quiz by Sergei Kushnirenko? I've heard that it has even more tricky questions... But shh, it's a secret. :)

If the error is beyond your grasp, I'll explain it. According to the C++ standard, class members in a member initializer list should be listed in the order that they're defined in the class. It turns out that the m_undoStack field will be initialized after it participates in the initialization of the m_document object, which can easily confuse a third-party developer.

A quick glance through the code shows that now the uninitialized object is not being used ahead of time. But, let's imagine that a new developer will need to limit the size of the m_undoStack history. Why not do it in the JsonSettingsDocument constructor, though?

The file: compilerexplorereditor.cpp:118

JsonSettingsDocument::JsonSettingsDocument(QUndoStack *undoStack)
    : m_undoStack(undoStack)
{
    m_undoStack->setUndoLimit(50); // Ops... Added for example
    ....
}

All in all, it's not a bad idea. It's sad that such code causes undefined behavior. Even checking the pointer before dereferencing won't save you; it's valid. Just the object it points to hasn't been initialized yet. That's why it's better to fix the code. Especially since it's not difficult at all. There are several options, and the easiest one is to change the fields order in the class.

class Editor : public Core::IEditor
{
public:
  Editor(TextEditor::TextEditorActionHandler &actionHandler);
  ~Editor();

  Core::IDocument *document() const override { return m_document.data(); }
  QWidget *toolBar() override;

  QUndoStack m_undoStack;                           // <=
  QSharedPointer<JsonSettingsDocument> m_document;  // <=
  std::unique_ptr<QToolBar> m_toolBar;
};

Here's a corresponding warning:

V670 [CWE-457, CERT-EXP53-CPP] The uninitialized class member 'm_undoStack' is used to initialize the 'm_document' member. Remember that members are initialized in the order of their declarations inside a class. compilerexplorereditor.cpp 774

Exception thrown by pointer

The analyzer detected an exception thrown by a pointer. Developers usually throw exceptions by value, and intercept by reference. Throwing a pointer may lead to a case when an exception won't be caught. And it also makes the intercepting side to call the operator delete to destroy the created exception object.

The file: celliterator.cpp:53

CellIterator &CellIterator::operator-=(int n)
{
  ....
  if (m_pos - n < 0)
    throw new std::runtime_error("-= n too big!");
}

I ran my eyes over the project code and didn't find any places where exceptions were caught by pointer. Most likely, this was an error that would sooner or later cause the program to crash. Alas.

Throwing an exception by a pointer isn't an error in itself, but we're better off avoiding it, especially in the overused code (the class name hints at its reusability). And the analyzer will be a good helper with such errors:

V1022 [CWE-755] An exception was thrown by pointer. Consider throwing it by value instead. celliterator.cpp 59

Deleting array via pointer to void

At the end, I suggest you look at a stunner—technically, it's a bug, but here it doesn't hurt the code. This is a good example of how you can suddenly add a little bit of undefined behavior (UB) to your program.

The file: containers.cpp:18

static void *readPointerArray(ULONG64 address, unsigned count,
                              const SymbolGroupValueContext &ctx)
{
  const unsigned pointerSize = SymbolGroupValue::pointerSize();
  const ULONG allocSize = pointerSize * count;
  ULONG bytesRead = 0;
  void *data = new unsigned char[allocSize];
  const HRESULT hr = ctx.dataspaces->ReadVirtual(address, data, allocSize,
                                                 &bytesRead);
  if (FAILED(hr) || bytesRead != allocSize)
  {
    delete [] data;                                                        // <=
    return 0;
  }

  return data;
}

"So, where's the error?", you may ask me. We select them via new and delete them via delete[], as stated in the article "Why do arrays have to be deleted via delete[] in C++." What's wrong again?

The issue is in the pointer to void (loss of the real type). From the point of view of the C++ standard, such an array deletion is undefined behavior. Here is quote $8.3.5/3 of the C++20 standard:

In the first alternative (delete object), if the static type of the object to be deleted is different from its dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and the static type shall have a virtual destructor or the behavior is undefined. In the second alternative (delete array) if the dynamic type of the object to be deleted differs from its static type, the behavior is undefined.

Here, we're lucky that memory is allocated to a trivial type, and it doesn't seem to cause major issues. If it was a class with a non-trivial destructor... What am I talking about, though? UB is UB. No one knows what will actually happen.

The analyzer protects against such errors as well:

  • V772 [CWE-758, CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. containers.cpp 26
  • V772 [CWE-758, CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. containers.cpp 439
  • V772 [CWE-758, CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. containers.cpp 767
  • V772 [CWE-758, CERT-MSC15-C] Calling a 'delete' operator for a void pointer will cause undefined behavior. containers.cpp 854

Conclusion

Well, let's stop here. We saw enough bugs, but at the same time, I'd like to say that the project is quite well-written: fresh code, documentation, pretty good API (though not without sins). It's really good, especially for a project with such a long history and its own static analyzer.

By the way, I mentioned the gift... And here it is: for this article, we've made a promocode: QTCREATOR. It allows you to try the PVS-Studio analyzer with the plugin for Qt Creator on your own code. Download the analyzer at the link.



Comments (0)

Next comments next comments
close comment form