Sviatoslav Razmyslov

Sep 29 2014

Tags:

#Cpp

The Unicorn Getting Interested in KDE

Sep 29 2014

Author: Sviatoslav Razmyslov

KDE PIM Libraries
KDE Base Libraries
KDE Base Apps
KDE Development
Conclusion

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.

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.

#Cpp