Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
Not so long ago, a new version of the Krita 4.0 free graphics editor was released. It's high time to check this project using PVS-Studio.
It's quite remarkable, that developers have already used PVS-Studio in far 2015 for the version Krita 2.9.2 and have successfully fixed the bugs with the help of it. However, it seems like since that time they haven't used the analyzer. In many of our articles, we often say that regular checks are really important, because if the developers had continued using PVS-Studio, the errors, which I'm considering in this article, simply wouldn't have got in the release.
PVS-Studio warning: V714 Variable row is not passed into for each loop by a reference, but its value is changed inside of the loop. kis_algebra_2d.cpp 532
DecomposedMatix::DecomposedMatix(const QTransform &t0)
{
....
if (!qFuzzyCompare(t.m33(), 1.0)) {
const qreal invM33 = 1.0 / t.m33();
for (auto row : rows) { // <=
row *= invM33;
}
}
....
}In this example, a programmer obviously wanted to multiply each element of the container rows by invM33, however, this will not happen. On each iteration of the loop, a new variable with the name row is created and then simply removed. To correct this error, you must create a reference to an element stored in a container:
for (auto &row : rows) {
row *= invM33;
}PVS-Studio warning: V547 Expression 'j == 0' is always false. KoColorSpace.cpp 218
QPolygonF KoColorSpace::estimatedTRCXYY() const
{
....
for (int j = 5; j>0; j--) {
channelValuesF.fill(0.0);
channelValuesF[i] = ((max / 4)*(5 - j));
if (colorModelId().id() != "XYZA") {
fromNormalisedChannelsValue(data, channelValuesF);
convertPixelsTo(....);
xyzColorSpace->normalisedChannelsValue(....);
}
if (j == 0) { // <=
colorantY = channelValuesF[1];
if (d->colorants.size()<2) {
d->colorants.resize(3 * colorChannelCount());
d->colorants[i] = ....
d->colorants[i + 1] = ....
d->colorants[i + 2] = ....
}
}
}
....
}A program will never go in the block under the condition j==0, because this condition is always false due to the fact that earlier in the for cycle there is a limitation j > 0.
Similar analyzer warnings:
PVS-Studio warning: V560 A part of conditional expression is always true. KoTextLayoutArea.cpp 1622
qreal KoTextLayoutArea::addLine(QTextLine &line,
FrameIterator *cursor,
KoTextBlockData &blockData)
{
if (!d->documentLayout->changeTracker()
|| !d->documentLayout->changeTracker()->displayChanges() // <=
|| !d->documentLayout->changeTracker()->...
|| !d->documentLayout->changeTracker()->...
|| !d->documentLayout->changeTracker()->elementById(....)
|| !d->documentLayout->changeTracker()->elementById(....)
|| ....
|| d->documentLayout->changeTracker()->displayChanges()) { // <=
....
}
}If you look closely, you will notice that within this complex condition there is a check of the type (!a || a):
d->documentLayout->changeTracker()->displayChanges() ||
!d->documentLayout->changeTracker()->displayChanges()This condition is always true, that is why this whole big check becomes meaningless, as reported by the analyzer.
PVS-Studio warning: V547 Expression 'n == 128' is always false. compression.cpp 110
PVS-Studio warning: V547 Expression 'n > 128' is always false. compression.cpp 112
quint32 decode_packbits(const char *src,
char* dst,
quint16 packed_len,
quint32 unpacked_len)
{
qint32 n;
....
while (unpack_left > 0 && pack_left > 0)
{
n = *src;
src++;
pack_left--;
if (n == 128) // <=
continue;
else if (n > 128) // <=
n -= 256;
....
}
....
}In this example, a value of the type const char, obtained by dereferencing the src pointer, is written in the variable n of the type qint32, that is why the range of the values for a variable n is as follows: [-128; 127]. Then the variable n is compared with the number 128, though it is clear that the result of such a check is always false.
Note: Project is built without -funsigned-char.
PVS-Studio warning: V590 Consider inspecting the 'state == (- 3) || state != 0' expression. The expression is excessive or contains a misprint. psd_pixel_utils.cpp 335
psd_status
psd_unzip_without_prediction(psd_uchar *src_buf, psd_int src_len,
psd_uchar *dst_buf, psd_int dst_len)
{
do {
state = inflate(&stream, Z_PARTIAL_FLUSH);
if(state == Z_STREAM_END)
break;
if(state == Z_DATA_ERROR || state != Z_OK) // <=
break;
} while (stream.avail_out > 0);
}Here the developers overthought with the second condition, that's why it became redundant. I suppose that the correct version looks as follows:
do {
state = inflate(&stream, Z_PARTIAL_FLUSH);
if(state != Z_OK)
break;
} while (stream.avail_out > 0);PVS-Studio warning: V547 Expression is always false. SvgTextEditor.cpp 472
void SvgTextEditor::setTextWeightDemi()
{
if (m_textEditorWidget.richTextEdit->textCursor()
.charFormat().fontWeight() > QFont::Normal
&& m_textEditorWidget.richTextEdit->textCursor()
.charFormat().fontWeight() < QFont::Normal) { // <=
setTextBold(QFont::Normal);
} else {
setTextBold(QFont::DemiBold);
}
}The analyzer has detected the condition (a > b && a < b), which is, obviously, always false. It's difficult to say what exactly the author wanted to write, but this code is clearly erroneous and needs correction.
PVS-Studio warning: V547 Expression is always true. Probably the '&&' operator should be used here. KoResourceItemChooser.cpp 408
void KoResourceItemChooser::updatePreview(KoResource *resource)
{
....
if (image.format() != QImage::Format_RGB32 || // <=
image.format() != QImage::Format_ARGB32 || // <=
image.format() != QImage::Format_ARGB32_Premultiplied) {
image = image.convertToFormat(....);
}
....
}A programmer made a mistake and instead of writing &&, wrote operator ||, leaving all its condition meaningless, because in the result he got the condition: a != const_1 || a != const_2, which is always true.
PVS-Studio warning: V547 Expression is always true. Probably the '&&' operator should be used here. KoSvgTextShapeMarkupConverter.cpp 1000
QString KoSvgTextShapeMarkupConverter::style(....)
{
....
if (format.underlineStyle() != QTextCharFormat::NoUnderline ||
format.underlineStyle() != QTextCharFormat::SpellCheckUnderline)
{
....
}
....
}The case, similar to the previous one: instead of the && operator wrote ||.
PVS-Studio warning: V501 There are identical sub-expressions 'sensor(FUZZY_PER_DAB, true)' to the left and to the right of the '||' operator. kis_pressure_size_option.cpp 43
void KisPressureSizeOption::lodLimitations(....) const
{
if (sensor(FUZZY_PER_DAB, true) || sensor(FUZZY_PER_DAB, true)) {
l->limitations << KoID("size-fade", i18nc("...."));
}
if (sensor(FADE, true)) {
l->blockers << KoID("...."));
}
}The analyzer has detected a situation where on the left and on the right of the operator || there are same expressions. If you look at the DynamicSensorType enumeration:
enum DynamicSensorType {
FUZZY_PER_DAB,
FUZZY_PER_STROKE,
SPEED,
FADE,
....
UNKNOWN = 255
};it becomes clear that most likely a developer wanted to write FUZZY_PER_STROKE on the right, rather than FUZZY_PER_DAB.
Static analyzers are great at detecting such errors, whereas during code-review it is easy to overlook them, especially when you have to view a big number of changes.
PVS-Studio warning: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: d->paragraphStylesDotXmlStyles.values(). KoTextSharedLoadingData.cpp 594
QList<KoParagraphStyle *>
KoTextSharedLoadingData::paragraphStyles(bool stylesDotXml) const
{
return stylesDotXml ?
d->paragraphStylesDotXmlStyles.values() :
d->paragraphStylesDotXmlStyles.values(); // <=
}Most likely, a programmer copied the block then in the ternary operator and forgot to change the name of the called method, because of which regardless of the condition, one value will always be returned.
Judging by the previous method:
KoParagraphStyle *
KoTextSharedLoadingData::paragraphStyle(const QString &name,
bool stylesDotXml) const
{
return stylesDotXml ?
d->paragraphStylesDotXmlStyles.value(name) :
d->paragraphContentDotXmlStyles.value(name);
}in the else block one has to write paragraphContentDotXmlStyles instead of paragraphStylesDotXmlStyles.
PVS-Studio warning: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: qFloor(axis). kis_transform_worker.cc 456
void mirror_impl(KisPaintDeviceSP dev, qreal axis, bool isHorizontal)
{
....
int leftCenterPoint = qFloor(axis) < axis ? qFloor(axis) :
qFloor(axis); // <=
int leftEnd = qMin(leftCenterPoint, rightEnd);
int rightCenterPoint = qFloor(axis) < axis ? qCeil(axis) :
qFloor(axis);
int rightStart = qMax(rightCenterPoint, leftStart);
....
}Another triggering, very similar to the previous one. Perhaps, in the then block of the first ternary operator a developer wanted to write qCeil(axis), not qFloor(axis), or the condition here is even redundant.
PVS-Studio warning: V656 Variables 'vx', 'vy' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 218, 219. KarbonSimplifyPath.cpp 219
bool KarbonSimplifyPath::isSufficentlyFlat(QPointF curve[4])
{
qreal ux = 3 * curve[1].x() - 2 * curve[0].x() - curve[3].x();
qreal uy = 3 * curve[1].y() - 2 * curve[0].y() - curve[3].y();
qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); // <=
qreal vy = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); // <=
....
}This code looks very suspicious, as, most likely, when writing formula for vy, one copied the previous line, but forgot to change the x() calls to y(). In case if there is no error here, it's better to rewrite the code as follows:
qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x();
qreal vy = vx;PVS-Studio warning: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 675, 679. KoTableCellStyle.cpp 679
void KoTableCellStyle::loadOdfProperties(
KoShapeLoadingContext &context,
KoStyleStack &styleStack)
{
....
if (styleStack.hasProperty(KoXmlNS::style, "print-content"))
{
setPrintContent(styleStack.property(KoXmlNS::style,
"print-content") == "true");
}
if (styleStack.hasProperty(KoXmlNS::style, "repeat-content")) // <=
{
setRepeatContent(styleStack.property(KoXmlNS::style,
"repeat-content") == "true");
}
if (styleStack.hasProperty(KoXmlNS::style, "repeat-content")) // <=
{
setRepeatContent(styleStack.property(KoXmlNS::style,
"repeat-content") == "true");
}
....
}The same check is performed twice. In this method, a developer either copied something extra, or mixed up something. If there is no error, then one has to remove the repeated code.
PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. kis_processing_applicator.cpp 227
void KisProcessingApplicator::applyVisitorAllFrames(....)
{
KisLayerUtils::FrameJobs jobs;
if (m_flags.testFlag(RECURSIVE)) {
KisLayerUtils::updateFrameJobsRecursive(&jobs, m_node); // <=
} else {
KisLayerUtils::updateFrameJobsRecursive(&jobs, m_node); // <=
}
....
}Probably, one copied the code from the block then to the block else and forgot to change the called method. Judging from the project code, a developer probably wanted to write KisLayerUtils::updateFrameJobs in the else branch.
PVS-Studio warning: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 255, 269. KoInlineTextObjectManager.cpp 255
void
KoInlineTextObjectManager::documentInformationUpdated(
const QString &info, const QString &data)
{
if (info == "title") // <=
setProperty(KoInlineObject::Title, data);
else if (info == "description")
setProperty(KoInlineObject::Description, data);
else if (info == "abstract")
setProperty(KoInlineObject::Comments, data);
else if (info == "subject")
setProperty(KoInlineObject::Subject, data);
else if (info == "keyword")
setProperty(KoInlineObject::Keywords, data);
else if (info == "creator")
setProperty(KoInlineObject::AuthorName, data);
else if (info == "initial")
setProperty(KoInlineObject::AuthorInitials, data);
else if (info == "title") // <=
setProperty(KoInlineObject::SenderTitle, data);
else if (info == "email")
setProperty(KoInlineObject::SenderEmail, data);
....
}Here one check is performed twice. Most likely, in the second case, one had to write something like "sender-title".
PVS-Studio warning: V768 The enumeration constant 'BatchMode' is used as a variable of a Boolean-type. KisMainWindow.cpp 811
bool KisMainWindow::openDocument(const QUrl &url, OpenFlags flags)
{
if (!QFile(url.toLocalFile()).exists()) {
if (!flags && BatchMode) { // <=
QMessageBox::critical(0,
i18nc("....", "Krita"),
i18n("....", url.url()));
}
....
}
....
}BatchMode is the member of enumeration OpenFlag with the value of 0x2:
enum OpenFlag {
None = 0,
Import = 0x1,
BatchMode = 0x2,
RecoveryFile = 0x4
};In this example, however, it is handled as if it is a variable. In the result, it turns out that a part of the condition is always true.
While in the above code, BatchMode is handled correctly:
if (flags & BatchMode) {
newdoc->setFileBatchMode(true);
}From this we can conclude that developers wanted to write something like this:
bool KisMainWindow::openDocument(const QUrl &url, OpenFlags flags)
{
if (!QFile(url.toLocalFile()).exists()) {
if (!(flags & BatchMode)) { // <=
QMessageBox::critical(0,
i18nc("....", "Krita"),
i18n("....", url.url()));
}
....
}
....
}PVS-Studio warning: V768 The enumeration constant 'State_Active' is used as a variable of a Boolean-type. KisOpenPane.cpp 104
void paint(....) const override
{
QStyledItemDelegate::paint(painter, option, index);
if(!(option.state & (int)(QStyle::State_Active && // <=
QStyle::State_Enabled))) // <=
{
....
}
}In this case, apparently, the authors of the code mixed up the operators and instead of | inside the mask used the operator &&. I think, the corrected version should be as follows:
void paint(....) const override
{
QStyledItemDelegate::paint(painter, option, index);
if(!(option.state & (int)(QStyle::State_Active |
QStyle::State_Enabled)))
{
....
}
}PVS-Studio warning: V519 The 'value' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 61, 66. kis_draggable_tool_button.cpp 66
int KisDraggableToolButton::continueDrag(const QPoint &pos)
{
....
if (m_orientation == Qt::Horizontal) {
value = diff.x(); // <=
} else {
value = -diff.y(); // <=
}
value = diff.x() - diff.y(); // <=
return value;
}The variable value is assigned a value within the condition, but then immediately the value is overwritten. Most likely, there's some sort of an error.
PVS-Studio warning: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 263, 265. lut.h 265
PVS-Studio warning: V519 The 'uf.f' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 271, 273. lut.h 273
LutKey<float>(float min, float max, float precision) :
m_min(min), m_max(max), m_precision(precision)
{
....
if(m_min > 0 && m_max > 0)
{
uf.f = m_min; // <=
m_tMin_p = uf.i >> m_shift;
uf.f = m_max; // <=
m_tMax_p = uf.i >> m_shift;
m_tMin_n = m_tMax_p;
m_tMax_n = m_tMax_p;
}
else if( m_max < 0)
{
uf.f = m_min; // <=
m_tMax_n = uf.i >> m_shift;
uf.f = m_max; // <=
m_tMin_n = uf.i >> m_shift;
m_tMin_p = m_tMax_n;
m_tMax_p = m_tMax_n;
}
....
}The variable uf.f is assigned with different values twice. It is suspicious and it is quite possible that developers wanted to assign a value to another variable.
PVS-Studio warning: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. SvgStyleWriter.cpp 82
void SvgStyleWriter::saveSvgBasicStyle(KoShape *shape,
SvgSavingContext &context)
{
if (!shape->isVisible(false)) {
....
} if (shape->transparency() > 0.0) { // <=
....
}
}Here, perhaps, one forgot keyword else. Even if there is no error, code formatting is worth fixing not to confuse the analyzer and other programmers.
A similar warning:
PVS-Studio warning: V595 The 'l' pointer was utilized before it was verified against nullptr. Check lines: 428, 429. kis_node_manager.cpp 428
void KisNodeManager::moveNodeAt(....)
{
....
KisLayer *l = qobject_cast<KisLayer*>(parent.data());
KisSelectionMaskSP selMask = l->selectionMask(); // <=
if (m && m->active() && l && l->selectionMask()) // <=
selMask->setActive(false);
....
}Here the pointer l is first dereferenced and only after that checked for nullptr.
Similar analyzer warnings:
PVS-Studio warning: V1004 The 'sb' pointer was used unsafely after it was verified against nullptr. Check lines: 665, 670. KisView.cpp 670
void KisView::slotSavingStatusMessage(const QString &text,
int timeout,
bool isAutoSaving)
{
QStatusBar *sb = statusBar();
if (sb) // <=
sb->showMessage(text, timeout);
KisConfig cfg;
if (sb->isHidden() || // <=
(!isAutoSaving && cfg.forceShowSaveMessages()) ||
(cfg.forceShowAutosaveMessages() && isAutoSaving)) {
viewManager()->showFloatingMessage(text, QIcon());
}
}The analyzer considers unsafe such a usage of a pointer sb after its checking for nullptr. Indeed, if the pointer is null (which is allowed, once such a condition is written above), then when calling sb->isHidden(), null pointer dereference might occur. As a hotfix, one can add a check for nullptr in the second condition as well, or handle this situation differently.
The similar analyzer warning:
PVS-Studio warning: V522 Dereferencing of the null pointer 'slot' might take place. kis_spriter_export.cpp 568
KisImportExportFilter::ConversionStatus KisSpriterExport::convert(
KisDocument *document,
QIODevice *io,
KisPropertiesConfigurationSP /*configuration*/)
{
....
SpriterSlot *slot = 0; // <=
// layer.name format: "base_name bone(bone_name) slot(slot_name)"
if (file.layerName.contains("slot(")) {
int start = file.layerName.indexOf("slot(") + 5;
int end = file.layerName.indexOf(')', start);
slot->name = file.layerName.mid(start, end - start); // <=
slot->defaultAttachmentFlag = .... // <=
}
....
}In this example, a dereference of the null pointer slot will certainly occur, which in turn results in undefined behavior.
PVS-Studio warning: V773 The function was exited without releasing the 'svgSymbol' pointer. A memory leak is possible. SvgParser.cpp 681
bool SvgParser::parseSymbol(const KoXmlElement &e)
{
....
KoSvgSymbol *svgSymbol = new KoSvgSymbol(); // <=
// ensure that the clip path is loaded in local coordinates system
m_context.pushGraphicsContext(e, false);
m_context.currentGC()->matrix = QTransform();
m_context.currentGC()->currentBoundingBox = QRectF(0.0, 0.0,
1.0, 1.0);
QString title = e.firstChildElement("title").toElement().text();
KoShape *symbolShape = parseGroup(e);
m_context.popGraphicsContext();
if (!symbolShape) return false; // <=
....
}In this example, when exiting the method one forgot to release the memory that has been allocated for svgSymbol. This is a memory leak. The project has many such leaks, but they are quite similar, so I won't discuss them a lot.
Similar analyzer warnings:
PVS-Studio warning: V668 There is no sense in testing the 'charStyle' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CharacterGeneral.cpp 153
bool KoPathShape::separate(QList<KoPathShape*> & separatedPaths)
{
....
Q_FOREACH (KoSubpath* subpath, d->subpaths) {
KoPathShape *shape = new KoPathShape();
if (! shape) continue; // <=
....
}
}It seems to me that this topic has become regular in our articles. It is pointless to check a pointer for nullptr if the memory was allocated by the operator new. If it impossible to allocate memory, the new operator throws an exception std::bad_alloc(), it doesn't return nullptr. To fix this code, you can add an exception handler, or use new (std: nothrow).
Similar analyzer warnings:
PVS-Studio warning: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!nodeJuggler' and 'nodeJuggler'. kis_node_manager.cpp 809
if (!nodeJuggler || // <=
(nodeJuggler && // <=
(nodeJuggler->isEnded() ||
!nodeJuggler->canMergeAction(actionName)))) {
....
}This check can be simplified:
if (!nodeJuggler ||
(nodeJuggler->isEnded() ||
!nodeJuggler->canMergeAction(actionName))) {
....
}Similar analyzer warning:
PVS-Studio warning: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: !iterator.atEnd() &&!iterator.atEnd() KoTextDebug.cpp 867
void KoTextDebug::dumpFrame(const QTextFrame *frame, QTextStream &out)
{
....
QTextFrame::iterator iterator = frame->begin();
for (; !iterator.atEnd() && !iterator.atEnd(); ++iterator) { // <=
....
}
....
}It is worth checking the condition of the loop for errors. If there are no errors, it is needed to remove a repeated check.
The similar analyzer warning:
PVS-Studio warning: V799 The 'cmd' variable is not used after memory has been allocated for it. Consider checking the use of this variable. kis_all_filter_test.cpp 154
bool testFilter(KisFilterSP f)
{
....
KisTransaction * cmd =
new KisTransaction(kundo2_noi18n(f->name()), dev); // <=
// Get the predefined configuration from a file
KisFilterConfigurationSP kfc = f->defaultConfiguration();
QFile file(QString(FILES_DATA_DIR) +
QDir::separator() + f->id() + ".cfg");
if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) {
//dbgKrita << "creating new file for " << f->id();
file.open(QIODevice::WriteOnly | QIODevice::Text);
QTextStream out(&file);
out.setCodec("UTF-8");
out << kfc->toXML();
} else {
QString s;
QTextStream in(&file);
in.setCodec("UTF-8");
s = in.readAll();
//dbgKrita << "Read for " << f->id() << "\n" << s;
kfc->fromXML(s);
}
dbgKrita << f->id();// << "\n" << kfc->toXML() << "\n";
f->process(dev, QRect(QPoint(0,0), qimage.size()), kfc);
QPoint errpoint;
delete cmd; // <=
....
}Here the memory was allocated and released for the object cmd, but it wasn't used even once.
PVS-Studio warning: V732 Unary minus operator does not modify a bool type value. Consider using the '!' operator. kis_equalizer_slider.cpp 75
QRect KisEqualizerSlider::Private::boundingRect() const
{
QRect bounds = q->rect().adjusted(0, 0, -isRightmost, -1);
return bounds;
}In this example, the variable isRightmost is of the type bool. Using the unary minus, the variable is implicitly converted into the type int and the resulting number is passed in the method adjusted(). Such code makes the understanding more difficult. Explicit is better than implicit, so I think I'd rewrite this fragment like this:
QRect KisEqualizerSlider::Private::boundingRect() const
{
QRect bounds = q->rect().adjusted(0, 0, isRightmost ? -1 : 0, -1);
return bounds;
}Similar analyzer warnings:
In conclusion, I would like to appeal to the developers of Krita and offer them to resume free using of our analyzer.
Since the last time when the developers used PVS-Studio, we have released the versions for Linux and macOS (I tested this project in Linux myself), and the analysis was significantly improved.
Besides, I suggest downloading and trying out PVS-Studio on the code of your project.
0
0
0
0