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