How to become an author
Search
Write a publication
Pull to refresh
307.17
Rating
PVS-Studio
Static Code Analysis for C, C++, C# and Java

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

Reading time11 min
Views22K
PVS-Studio corporate blogC++*

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

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.

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
Tags:
Hubs:
+33
Comments29
PVS-Studio
Static Code Analysis for C, C++, C# and Java
WebsiteFacebookTwitterMediumTelegram
119
Karma
0
Rating
Святослав @SvyatoslavMC

Product Manager

Send message

Articles

Information

Website
pvs-studio.com
Registered
Founded
2008
Employees
31–50 employees

Your account

Sections

Information

Services

Support
© 2006–2024, Habr