Вебинар: C# разработка и статический анализ: в чем практическая польза? - 18.11
KDE (сокращение от K Desktop Environment) - среда рабочего стола, преимущественно для Linux и других UNIX-подобных систем. Если простым языком, то это та штука, которая отвечает за всё графическое оформление. Среда построена на основе кроссплатформенного инструментария разработки пользовательского интерфейса Qt. Разработкой занимаются несколько сотен программистов со всего мира, преданных идее свободного программного обеспечения. KDE предлагает полный набор приложений пользовательского окружения, который позволяет взаимодействовать с операционной системой в современном графическом интерфейсе. Давайте же посмотрим, что у KDE под капотом.
Следующие пакеты проекта KDE версии 4.14 проверялись с помощью PVS-Studio 5.19 в OpenSUSE Factory:
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;
}
}
....
}
Выражение всегда истинно. Причиной может быть опечатка или неправильная логика программиста. Возможно, здесь необходимо использовать оператор '&&'.
Аналогичное подозрительное место:
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(). Указатель сразу разыменовывается в следующей строке.
Аналогичные места:
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;
Похожее место:
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' выполняется перед его проверкой.
Похожие опасные места:
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() + ")";
Аналогичные места:
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(), так как указатель на старую область памяти может быть утерян, если память не удастся увеличить.
Аналогичные места:
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'.
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);
}
Анализатор обнаружил несколько мест с повторяющимися условными выраженями. Что-то из этого вполне может быть серьёзной опечаткой.
Ещё такие места:
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;
....
}
И в этом проекте нашлось место с разыменованием указателя до его проверки.
Ещё место:
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);
Похожее место:
Огромная аудитория пользователей и разработчиков продуктов KDE играет большую роль в плане тестирования, но также стоит уделять внимание и различным анализаторам кода. Впрочем, если судить по публикациям в Интернете, для проверки исходных кодов KDE уже используется как минимум Coverity. Именно из-за этого PVS-Studio находит очень мало подозрительных мест.
Используя статический анализ регулярно, можно сэкономить массу времени на решение более полезных задач. Также контроль за качеством кода можно переложить на других, например, воспользовавшись новой услугой: регулярный аудит Си/Си++ кода.
0