To get a trial key
fill out the form below
Team License (a basic version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

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

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

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

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.

>
>
>
The Unicorn Getting Interested in KDE

The Unicorn Getting Interested in KDE

Sep 29 2014

KDE (abbreviation for K Desktop Environment) is a desktop environment primarily for Linux and other UNIX-like operating systems. To put it simple, it's the thing which is responsible for the entire graphic design. The environment is based on the cross-platform user interface development toolkit Qt. The development is done by several hundreds of programmers throughout the world devoted to the idea of free software. KDE offers a complete set of user environment applications that allows one to interact with the operating system within the framework of a modern graphic interface. So let's see what KDE has under the hood.

0283_KDE/image1.png

We checked the following packages of the KDE project of version 4.14 by PVS-Studio 5.19 in OpenSUSE Factory:

  • KDE PIM Libraries
  • KDE Base Libraries
  • KDE Base Apps
  • KDE Development

KDE PIM Libraries

V547 Expression is always true. Probably the '&&' operator should be used here. incidenceformatter.cpp 2684

enum PartStat {
  ....
  Accepted,
  Tentative,
  ....
};

static QString formatICalInvitationHelper(....)
{
  ....
  a = findDelegatedFromMyAttendee( inc );
  if ( a ) {
    if ( a->status() != Attendee::Accepted ||      // <=
         a->status() != Attendee::Tentative ) {    // <=
      html += responseButtons( inc, rsvpReq, rsvpRec, helper );
      break;
    }
  }
  ....
}

The expression is always true. It may be caused by a typo or programmer's incorrect logic. The '&&' operator should probably be used here.

Another similar fragment:

  • V547 Expression is always true. Probably the '&&' operator should be used here. incidenceformatter.cpp 3293

V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. kio_ldap.cpp 535

void LDAPProtocol::del( const KUrl &_url, bool )
{
  ....
  if ( (id = mOp.del( usrc.dn() ) == -1) ) {
    LDAPErr();
    return;
  }
  ret = mOp.waitForResult( id, -1 );
  ....
}

The priority of the comparison operator (==) is higher than that of the assignment operator (=). Just thanks to pure luck, the condition is executed as expected, but after that, an incorrect value of the 'id' identifier is used which is 0.

V595 The 'incBase' pointer was utilized before it was verified against nullptr. Check lines: 2487, 2491. incidenceformatter.cpp 2487

static QString formatICalInvitationHelper(....)
{
  ....
  incBase->shiftTimes( mCalendar->timeSpec(), ....);

  Incidence *existingIncidence = 0;
  if ( incBase && helper->calendar() ) {
    ....
  }
  ....
}

The 'incBase' pointer is dereferenced before being checked.

V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. listjob.cpp 131

void ListJob::doStart()
{
  Q_D( ListJob );

  switch ( d->option ) {
    break;                          // <=
  case IncludeUnsubscribed:
    d->command = "LIST";
    break;
  case IncludeFolderRoleFlags:
    d->command = "XLIST";
    break;
  case NoOption:
  default:
    d->command = "LSUB";
  }
  ....
}

The first operator in the 'switch' operator's block is other than 'case'. Because of that, this code fragment will never get control. At best, it's just that the 'break' operator could have been left of some old condition removed incompletely; but at worst, there is one more 'case' missing here.

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'lexBuf.strs' is lost. Consider assigning realloc() to a temporary pointer. vcc.y 638

static void lexAppendc(int c)
{
  lexBuf.strs = (char *) realloc(lexBuf.strs, (size_t) .... + 1);  
  lexBuf.strs[lexBuf.strsLen] = c;
  ....
}

This expression is potentially dangerous: it is recommended that the realloc function's result be saved into a different variable. The realloc() function changes the size of some memory block. If it fails to do so, the pointer to the old memory block will be lost.

Well, the overall quality of this code is very low. There's no check for what the realloc() function returns; the pointer is dereferenced at once in the next line.

Other similar fragments:

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'mods' is lost. Consider assigning realloc() to a temporary pointer. ldapoperation.cpp 534
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'mods[i]->mod_vals.modv_bvals' is lost. Consider assigning realloc() to a temporary pointer. ldapoperation.cpp 579
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'ctrls' is lost. Consider assigning realloc() to a temporary pointer. ldapoperation.cpp 624
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'fp->s' is lost. Consider assigning realloc() to a temporary pointer. vobject.c 1055
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'lexBuf.strs' is lost. Consider assigning realloc() to a temporary pointer. vcc.y 635
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'lexBuf.strs' is lost. Consider assigning realloc() to a temporary pointer. vcc.y 643
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'bytes' is lost. Consider assigning realloc() to a temporary pointer. vcc.y 928
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'fp->s' is lost. Consider assigning realloc() to a temporary pointer. vobject.c 1050

KDE Base Libraries

V523 The 'then' statement is equivalent to the 'else' statement. kconfig_compiler.cpp 1051

QString newItem( const QString &type, ....)
{
  QString t = "new "+cfg.inherits+"::Item" + ....;
  if ( type == "Enum" ) t += ", values" + name;
  if ( !defaultValue.isEmpty() ) {
    t += ", ";
    if ( type == "String" ) t += defaultValue;        // <=
    else t+= defaultValue;                            // <=
  }
  t += " );";

  return t;
}

It's too suspicious that the 'if' operator has identical true and false branches. Unless the code has a typo, it can be simplified like this:

if ( !defaultValue.isEmpty() )
    t += ", " + defaultValue;

Another similar fragment:

  • V523 The 'then' statement is equivalent to the 'else' statement. installation.cpp 589

V595 The 'priv->slider' pointer was utilized before it was verified against nullptr. Check lines: 786, 792. knuminput.cpp 786

void KDoubleNumInput::spinBoxChanged(double val)
{
  ....
  const double slidemin = priv->slider->minimum();      // <=
  const double slidemax = priv->slider->maximum();      // <=
  ....
  if (priv->slider) {                                   // <=
    priv->slider->blockSignals(true);
    priv->slider->setValue(qRound(slidemin + rel * (....)));
    priv->slider->blockSignals(false);
  }
}

The 'priv' pointer is dereferenced before being checked.

Other similar dangerous fragments:

  • V595 The 'm_instance' pointer was utilized before it was verified against nullptr. Check lines: 364, 376. ksystemtimezone.cpp 364
  • V595 The 'job' pointer was utilized before it was verified against nullptr. Check lines: 778, 783. knewfilemenu.cpp 778

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. karchive.cpp 187

*bool KArchive::close()
{
  ....
  // if d->saveFile is not null then it is equal to d->dev.
  if ( d->saveFile ) {
    closeSucceeded = d->saveFile->finalize();
    delete d->saveFile;
    d->saveFile = 0;
  } if ( d->deviceOwned ) {                                 // <=
    delete d->dev; // we created it ourselves in open()
  }
  ....
}

This code may indicate a missing keyword 'else', or this is just extremely incomprehensible and confusing code formatting.

V655 The strings was concatenated but are not utilized. Consider inspecting the expression. entrydetailsdialog.cpp 225

void EntryDetails::updateButtons()
{
  ....
  foreach (....) {
    QString text = info.name;
    if (!info.distributionType.trimmed().isEmpty()) {
        text + " (" + info.distributionType.trimmed() + ")";// <=
    }
    QAction* installAction =
      installMenu->addAction(KIcon("dialog-ok"), text);
    installAction->setData(info.id);
  }
  ....
}

The analyzer has detected an unused union of string variables. The code was probably meant to look as follows:

text += " (" + info.distributionType.trimmed() + ")";

Other similar fragments:

  • V655 The strings was concatenated but are not utilized. Consider inspecting the expression. itemsgridviewdelegate.cpp 365
  • V655 The strings was concatenated but are not utilized. Consider inspecting the expression. itemsviewdelegate.cpp 159

V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. entrydetailsdialog.cpp 149

void EntryDetails::entryChanged(const KNS3::EntryInternal& entry)
{
  ....
  if(m_entry.previewUrl(EntryInternal::PreviewSmall1).isEmpty()){
    ui->previewBig->setVisible(false);
  } else                                // <=

  if (!m_entry.previewUrl((....)type).isEmpty()) {
    ....
  }
  ....
}

The formatting of this code fragment is ambiguous too. Was it the "else if" construct meant to be used here or the programmer simply forgot to delete 'else'?

V612 An unconditional 'return' within a loop. bufferfragment_p.h 94

BufferFragment split(char c, unsigned int* start) 
{
  while (*start < len) {
    int end = indexOf(c, *start);
    if (end == -1) end = len;
    BufferFragment line(d + (*start), end - (*start));
    *start = end + 1;
    return line;
  }
  return BufferFragment();
}

Was it necessary to write a loop for one iteration only? Or maybe a conditional operator is missing?

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'd' is lost. Consider assigning realloc() to a temporary pointer. netwm.cpp 596

template <class Z>
void NETRArray<Z>::reset() {
    sz = 0;
    capacity = 2;
    d = (Z*) realloc(d, sizeof(Z)*capacity);
    memset( (void*) d, 0, sizeof(Z)*capacity );
}

Like in "KDE PIM Libraries", it is not recommended to use one pointer with the realloc() function as the pointer to the old memory block may be lost if memory fails to be increased.

Other similar fragments:

  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'handlers' is lost. Consider assigning realloc() to a temporary pointer. kxerrorhandler.cpp 94
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'buffer' is lost. Consider assigning realloc() to a temporary pointer. netwm.cpp 528
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'd' is lost. Consider assigning realloc() to a temporary pointer. netwm.cpp 608
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'ptr' is lost. Consider assigning realloc() to a temporary pointer. kdesu_stub.c 119
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'addr.generic' is lost. Consider assigning realloc() to a temporary pointer. k3socketaddress.cpp 372

KDE Base Apps

V501 There are identical sub-expressions 'mimeData->hasFormat(QLatin1String("application/x-kde-ark-dndextract-service"))' to the left and to the right of the '&&' operator. iconview.cpp 2357

void IconView::dropEvent(QGraphicsSceneDragDropEvent *event)
{
  ....
  if (mimeData->hasFormat(QLatin1String(
       "application/x-kde-ark-dndextract-service")) &&      // <=
      mimeData->hasFormat(QLatin1String(
       "application/x-kde-ark-dndextract-service")))        // <=
  {
    const QString remoteDBusClient = mimeData->data(
      QLatin1String("application/x-kde-ark-dndextract-service"));
    const QString remoteDBusPath = mimeData->data(
      QLatin1String("application/x-kde-ark-dndextract-path"));
    ....
  }
  ....
}

The analyzer has detected two identical conditional expressions. The conditional operator's body suggests that the condition should have looked like this:

if (mimeData->hasFormat(QLatin1String(
       "application/x-kde-ark-dndextract-service")) &&     // <=
      mimeData->hasFormat(QLatin1String(
       "application/x-kde-ark-dndextract-path ")))         // <=
{
  ....
}

V595 The 'm_view' pointer was utilized before it was verified against nullptr. Check lines: 797, 801. kitemlistcontroller.cpp 797

bool KItemListController::mouseDoubleClickEvent(....)
{
  const QPointF pos = transform.map(event->pos());
  const int index = m_view->itemAt(pos);

  // Expand item if desired - See Bug 295573
  if (m_mouseDoubleClickAction != ActivateItemOnly) {
    if (m_view && m_model && ....) {
      const bool expanded = m_model->isExpanded(index);
      m_model->setExpanded(index, !expanded);
    }
  }
  ....
}

There are plenty of fragments in KDE projects where a pointer received by a function is first used to initialize local variables and only then is checked before derefrencing.

V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 410, 412. kebsearchline.cpp 410

void
KViewSearchLine::slotColumnsRemoved(const QModelIndex &,
                                    int first, int last)
{
  if(d->treeView)
    updateSearch();
  else
  {
    if(d->listView->modelColumn() >= first &&
       d->listView->modelColumn() <= last)
    {
      if(d->listView->modelColumn()>last)   // <=
        kFatal()<<"...."<<endl;
      updateSearch();
    }
  }
}

The nested condition will always be false. I think this condition used to make sense until some edits were made.

V654 The condition 'state != 1' of loop is always true. passwd.cpp 255

int PasswdProcess::ConversePasswd(....)
{
  ....
  state = 0;
  while (state != 1)
  {
    line = readLine();
    if (line.isNull())
    {
      // No more input... OK
      return 0;
    }
    if (isPrompt(line, "password"))
    {
      // Uh oh, another prompt. Not good!
      kill(m_Pid, SIGKILL);
      waitForChild();
      return PasswordNotGood;
    }
    m_Error += line + '\n'; // Collect error message
  }
  ....
}

The value of the 'state' variable is not changed in the loop; therefore, the termination condition is only represented by the call of 'return'.

KDE Development

V501 There are identical sub-expressions 'file == rhs.file' to the left and to the right of the '&&' operator. pp-macro.cpp 44

bool pp_macro::operator==(const pp_macro& rhs) const {
  if(completeHash() != rhs.completeHash())
    return false;
  
  return name == rhs.name && file == rhs.file &&      // <=
         file == rhs.file &&                          // <=
         sourceLine == rhs.sourceLine &&
         defined == rhs.defined &&
         hidden == rhs.hidden &&
         function_like == rhs.function_like &&
         variadics == rhs.variadics &&
         fixed == rhs.fixed &&
         defineOnOverride == rhs.defineOnOverride &&
         listsEqual(rhs);
}

The analyzer has detected a number of fragments with duplicated conditional expressions. Something of these may be serious typos.

Other fragments of this kind;

  • V501 There are identical sub-expressions 'tokenKind == Token_not_eq' to the left and to the right of the '||' operator. builtinoperators.cpp 174
  • V501 There are identical sub-expressions '!context->owner()' to the left and to the right of the '||' operator. typeutils.cpp 194

V595 The 'parentJob()->cpp()' pointer was utilized before it was verified against nullptr. Check lines: 437, 438. cppparsejob.cpp 437

void CPPInternalParseJob::run()
{
    ....
    QReadLocker lock(parentJob()->parentPreprocessor() ?
      0: parentJob()->cpp()->language()->parseLock());      // <=
    if(.... || !parentJob()->cpp())                         // <=
      return;
    ....
}

The issue of a pointer dereferenced before a check can be found in this project too.

Another fragment:

  • V595 The 'parentContext()' pointer was utilized before it was verified against nullptr. Check lines: 692, 695. context.cpp 692

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. usedecoratorvisitor.cpp 40

DataAccess::DataAccessFlags typeToDataAccessFlags(....)
{
  DataAccess::DataAccessFlags ret = DataAccess::Read;
  TypePtr< ReferenceType > reftype=type.cast<ReferenceType>();
  if(reftype && reftype->baseType() &&
     !reftype->baseType()->modifiers() &    // <=
     AbstractType::ConstModifier)
    ret |= DataAccess::Write;
  
  return ret;
}

Of course, the authors know better if there is a bug here or not, but the '&' operator looks suspicious. Note that the "!reftype->baseType()->modifiers()" expression is of the 'bool' type.

V555 The expression 'm_pos - backOffset > 0' will work as 'm_pos != backOffset'. pp-stream.cpp 225

unsigned int rpp::Stream::peekLastOutput(uint backOffset) const {
  if(m_pos - backOffset > 0)
    return m_string->at(m_pos - backOffset - 1);
  return 0;
}

Comparing the difference of unsigned numbers with zero is not quite correct because a negative result may be interpreted as a very large positive number. In order not to get a giant index in the condition body, the condition should be rewritten in the following way:

if(m_pos > backOffset)
    return m_string->at(m_pos - backOffset - 1);

Another similar fragment:

  • V555 The expression 'nextOffset - currentOffset > 0' will work as 'nextOffset != currentOffset'. pp-location.cpp 211

Conclusion

The huge audience of KDE products' users and developers does play a very significant role what testing is concerned, but they also should consider using various code analyzers. However, if some posts on the Internet are right, the authors already use at least Coverity to analyze KDE source codes. It's because of that PVS-Studio has found so few suspicious fragments.

Using static analysis regularly will help you save plenty of time to solve more serious tasks.

Popular related articles
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…

Comments (0)

Next comments
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept