Единорог заинтересовался KDE

KDE (сокращение от K Desktop Environment) — среда рабочего стола, преимущественно для Linux и других UNIX-подобных систем. Если простым языком, то это та штука, которая отвечает за всё графическое оформление. Среда построена на основе кроссплатформенного инструментария разработки пользовательского интерфейса Qt. Разработкой занимаются несколько сотен программистов со всего мира, преданных идее свободного программного обеспечения. KDE предлагает полный набор приложений пользовательского окружения, который позволяет взаимодействовать с операционной системой в современном графическом интерфейсе. Давайте же посмотрим, что у KDE под капотом.

Следующие пакеты проекта KDE версии 4.14 проверялись с помощью PVS-Studio 5.19 в 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;
    }
  }
  ....
}

Выражение всегда истинно. Причиной может быть опечатка или неправильная логика программиста. Возможно, здесь необходимо использовать оператор '&&'.

Аналогичное подозрительное место:

• 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 );
  ....
}

Приоритет оператора сравнения (==) выше приоритета оператора присваивания (=). Благодаря везению, условие выполняется как задумно, но дальше используется некорректное значение идентификатора 'id', равное 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() ) {
    ....
  }
  ....
}

Разыменование указателя 'incBase' выполняется перед его проверкой.

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

В блоке оператора 'switch' первым оператором не является 'case'. Это приводит к тому, что фрагмент кода никогда не получит управление. В лучшем случае оператор 'break' остался после неполного удаления старого условия, но в худшем случае тут пропущен ещё один 'case'.

V560 A part of conditional expression is always false: validity > 0. imap4.cpp 1843

void IMAP4Protocol::stat (const KUrl & _url)
{
  ....
  ulong validity = 0;
  ....
  if ( validity > 0 && validity != aValidity.toULong() ) {
    ....
  }
  ....
}

Очевидно, что беззнаковая переменная всегда будет больше нуля. Подобные места часто остаются после рефакторинга. А что, если условное выражение писалось таким образом, потому что где-то в эту переменную сохраняется отрицальный результат системной функции? В таком случае проверка уже некорректна.

Аналогичное место:

• V560 A part of conditional expression is always false: validity > 0. imap4.cpp 1858

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;
  ....
}

Данное выражение является потенциально опасным: рекомендуется результат функции realloc сохранять в другой переменной. Функция realloc() производит изменение размера некоторого блока памяти. В случае ошибки указатель на старую область памяти будет утерян.

Да и вообще, такой код плох. Нет провреки того, что вернула функция realloc(). Указатель сразу разыменовывается в следующей строке.

Аналогичные места:

• 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;
}

Одинаковые истинная и ложная ветвь оператора 'if' выглядят крайне подозрительно. Если код всё-таки не содержит опечатку, то его можно упростить, например, так:

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

Похожее место:

• 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);
  }
}

Разыменование указателя 'priv' выполняется перед его проверкой.

Похожие опасные места:

• 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()
  }
  ....
}

Данный фрагмент кода может свидетельствовать о пропуске ключевого слова 'else', либо это просто крайне ненаглядное и сбивающие с толку оформление кода.

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);
  }
  ....
}

Анализатор обнаружил неиспользуемое объединение строковых переменных. Возможно, код должен быть таким:

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

Аналогичные места:

• 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()) {
    ....
  }
  ....
}

Оформление этого фрагмента кода тоже выглядит неоднозначно. Планировалась ли тут конструкция «else if» или же просто забыли удалить '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();
}

Стоило ли писать цикл из-за одной итерации? Или же не хватает условного оператора?

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 );
}

Как и в «KDE PIM Libraries», здесь не рекомендутся использовать один указатель с функцией realloc(), так как указатель на старую область памяти может быть утерян, если память не удастся увеличить.

Аналогичные места:

• 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"));
    ....
  }
  ....
}

Анализатор обнаружил одинаковые условные выражения. По телу условного оператора логично предположить, что условие должно быть таким:

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);
    }
  }
  ....
}

В проектах KDE оказалось много мест, где указатель, пришедший в функцию, сначала используется для инициализиации локальных переменных, а в дальнейшем уже проверяется перед разыменованием.

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();
    }
  }
}

Вложенное условие будет всегда ложным. Можно предположить, что такое условие было актуальным до некоторых правок.

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

Значение переменной 'state' не изменяется в цикле, следовательно, условием остановки является только вызов '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);
}

Анализатор обнаружил несколько мест с повторяющимися условными выраженями. Что-то из этого вполне может быть серьёзной опечаткой.

Ещё такие места:

• 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;
    ....
}

И в этом проекте нашлось место с разыменованием указателя до его проверки.

Ещё место:

• 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;
}

Авторам лучше знать, ошибка здесь или нет, но оператор '&' выглядит подозрительно. Обратите внимание, что выражение "!reftype->baseType()->modifiers()" имеет тип 'bool'.

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

Сравнивать разность беззнаковых чисел с нулём не совсем корректно, потому что отрицательный результат может интерпретироваться как очень большое положительное число. Чтобы в теле условия не получить гигантский индекс, лучше написать условие таким образом:

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

Похожее место:

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

Заключение

Огромная аудитория пользователей и разработчиков продуктов KDE играет большую роль в плане тестирования, но также стоит уделять внимание и различным анализаторам кода. Впрочем, если судить по публикациям в Интернете, для проверки исходных кодов KDE уже используется как минимум Coverity. Именно из-за этого PVS-Studio находит очень мало подозрительных мест.

Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач. Также контроль за качеством кода можно переложить на других, например, воспользовавшись новой услугой: регулярный аудит Си/Си++ кода.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. The Unicorn Getting Interested in KDE.

Автор: SvyatoslavMC

Источник