Checking the source code of the free graphical editor Krita 4.0
Not so long ago, the release of a new version of the free graphic editor Krita 4.0. It's time to test this project with PVS-Studio.
It is noteworthy that the developers have already used PVS-Studio in the distant 2015 on the Krita 2.9.2 version and successfully fixed the errors with it. However, apparently, the analyzer has not been used since. In all our articles, we say that regular checks are important, because if the developers continued to use PVS-Studio, the errors that I will discuss in this article would simply not be released.
PVS-Studio warning : V714 Variable row is not passed through the loop. kis_algebra_2d.cpp 532
')
In this example, the programmer obviously wanted to multiply each element of the rows container by invM33 , however, this will not happen. At each iteration of the loop, a new variable is created with the name row , which is then simply destroyed. To correct the error, you must create a link to the element stored in the container:
PVS-Studio warning : V547 Expression 'j == 0' is always false. KoColorSpace.cpp 218
The program will never enter the block under the condition j == 0 , since this condition is always false due to the fact that the restriction j> 0 is imposed above in the for loop.
Similar analyzer warnings:
PVS-Studio warning : V560 A part of conditional expression is always true. KoTextLayoutArea.cpp 1622
If you look closely, you will notice that inside this complex condition there is a check of the form (! A || a) :
Such a condition is always true, because of which all this big test becomes meaningless, and the analyzer reports.
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
In this example, the value of the type const char , obtained by de-referencing the src pointer , is written to the n- type variable qint32 , so the range of valid values for the variable n : [-128; 127]. Then the variable n is compared with the number 128, although it is clear that the result of such a test is always false .
Note: The project is built without -funsigned-char .
PVS-Studio warning : V590 Consider inspecting the 'state == (- 3) || state! = 0 'expression. The expression is misprint. psd_pixel_utils.cpp 335
Here they were too smart with the second condition, because of which it became redundant. I will assume that the correct version is as follows:
PVS-Studio warning : V547 Expression is always false. SvgTextEditor.cpp 472
The analyzer detected a condition of the form (a> b && a <b) , which, obviously, is always false. It is difficult to say exactly what the author wanted to write, however, this code is clearly erroneous and needs to be corrected.
PVS-Studio warning : V547 Expression is always true. Probably the '&&' operator should be used here. KoResourceItemChooser.cpp 408
The programmer made a mistake and instead of the operator && wrote the operator || , because of what all his condition became meaningless, because the condition of the form turned out: 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
The case is similar to the previous one: mixed up the logical operator and instead of && wrote || .
PVS-Studio warning : V501 There are identical sub-expressions 'sensor (FUZZY_PER_DAB, true)' operator. kis_pressure_size_option.cpp 43
The analyzer detected a situation where, to the left and right of the operator || there are identical expressions. If you look at the DynamicSensorType enumeration:
it becomes clear that, on the right, most likely they wanted to write FUZZY_PER_STROKE , and not FUZZY_PER_DAB .
Static analyzers do well in finding such errors, but they are easy to overlook in code review, especially when you need to view a large number of changes.
PVS-Studio warning : V583 The '?:' Operator, regardless of its conditional expression, always returns the same value: d-> paragraphStylesDotXmlStyles.values (). KoTextSharedLoadingData.cpp 594
Here, most likely, they copied the then block in the ternary operator and forgot to change the name of the method being called, which is why, regardless of the condition, one value will always be returned.
Judging by the previous method:
in the else block, you need to write paragraphContentDotXmlStyles , instead of paragraphStylesDotXmlStyles .
PVS-Studio warning : V583 The '?:' Operator, regardless of its conditional expression, qFloor (axis). kis_transform_worker.cc 456
Another trigger, very similar to the previous one. Perhaps, in the then block of the first ternary operator, they wanted to write qCeil (axis) , not qFloor (axis) , or else the condition here is superfluous.
PVS-Studio Warning: V656 Variables 'vx', 'vy' are initialized. It's probably not an error or un-optimized code. Check lines: 218, 219. KarbonSimplifyPath.cpp 219
This code looks very suspicious, because, most likely, when writing a formula for vy, they copied the previous line, but forgot to change the x () calls to y () . In case there is no error here, it is better to rewrite the code like this:
PVS-Studio warning : V581 The statement expressions of the "if" statements stand alongside each other are identical. Check lines: 675, 679. KoTableCellStyle.cpp 679
The same test is performed twice. In this method, either copied too much, or something mixed up. If there is no error, then it is worth removing the duplicate code.
PVS-Studio warning : V523 The 'then' statement is equivalent to the 'else' statement. kis_processing_applicator.cpp 227
Most likely, here we copied the code from the then block into the else block and forgot to change the called method. Judging by the project code, in the else branch, they probably wanted to write KisLayerUtils :: updateFrameJobs .
PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 255, 269. KoInlineTextObjectManager.cpp 255
Here the same check is performed twice. Most likely, in the second case, it was necessary 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
BatchMode is an OpenFlag enumeration element with a value of 0x2 :
However, in this example, it is treated as if it were a variable. The result is that part of the condition is always true.
In this case, in the code above, BatchMode works correctly:
From this we can conclude that they wanted to write something like this:
PVS-Studio warning : V768 The enumeration constant 'State_Active' is used as a variable of a Boolean-type. KisOpenPane.cpp 104
In this case, apparently, they mixed up the operators and instead of | inside the mask used the operator && . I think that the corrected version should look like this:
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
The value is assigned some value inside the condition, however, then this value is immediately overwritten. Most likely, there is some kind of mistake.
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
The variable uf.f is assigned two different times in a row. This is suspicious, and it is possible that they wanted to assign a value to some other variable.
PVS-Studio warning : V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. SvgStyleWriter.cpp 82
Here, you may have forgotten the keyword else . Even if there is no error, you should correct the formatting of the code so as not to embarrass the analyzer and other programmers.
Similar warning:
PVS-Studio warning : V595 The 'l' pointer was used before it was verified against nullptr. Check lines: 428, 429. kis_node_manager.cpp 428
Here, the pointer l is first dereferenced and only then 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
The analyzer considers such use of the sb pointer to be unsafe after checking it for nullptr . Indeed, in case the pointer is zero (and this is allowed, since such a condition is written above), then when calling sb-> isHidden () a dereferencing of the zero pointer may occur. As a correction, you can add a check for nullptr and in the second condition, or else somehow handle this situation.
Similar analyzer warning:
PVS-Studio warning : V522 Dereferencing of the null pointer 'slot' might take place. kis_spriter_export.cpp 568
In this example, a null slot pointer is guaranteed to dereference, which in turn leads to undefined program behavior.
PVS-Studio warning : V773 The function was exited without releasing the 'svgSymbol' pointer. A memory leak is possible. SvgParser.cpp 681
In this example, when exiting the method, they forgot to free up the memory allocated for svgSymbol . This is a memory leak. There are many similar leaks in the project, but they are of the same type, so I will not dwell on them.
Similar analyzer warnings:
PVS-Studio warning : V668 against S S S S S S S S S. The exception will be generated in the case of memory allocation error. CharacterGeneral.cpp 153
This column in our articles, it seems, has already become permanent. It makes no sense to check the pointer to nullptr if the memory has been allocated using the new operator. If memory cannot be allocated, the new operator throws an exception std :: bad_alloc () , and does not return nullptr . To fix this code, you can add an exception handler, or use new (std :: nothrow) .
Similar analyzer warnings:
PVS-Studio warning : V728 Alert can be simplified. The '||' operator juxgler 'and' nodeJuggler '. kis_node_manager.cpp 809
This check can be simplified:
Similar analyzer warning:
PVS-Studio warning : V501 operator:! Iterator.atEnd () &&! Iterator.atEnd () KoTextDebug.cpp 867
It is worth checking the condition of the for loop for errors. If there are no errors, it is worth removing the duplicate check.
Similar analyzer warning:
PVS-Studio warning : V799 The 'cmd' variable is not used after it has been allocated. Consider checking the use of this variable. kis_all_filter_test.cpp 154
Here they allocated and freed up memory for the cmd object, but never used it.
PVS-Studio warning : V732 Unary minus operator doesn’t modify a bool type value. Consider using the '!' operator. kis_equalizer_slider.cpp 75
In this example, the variable isRightmost is of type bool . Using the unary minus, the variable is implicitly converted to the type int and the resulting number is passed to the adjusted () method. Such code complicates understanding. Explicit is better than implicit, so I think it's worth rewriting this fragment like this:
Similar analyzer warnings:
In conclusion, I would like to contact the developers of Krita and offer them to resume using our analyzer free of charge.
Since the last time developers used PVS-Studio, we had versions for Linux and MacOS (I checked this project in Linux myself), and the analysis also improved significantly.
In addition, I invite everyone to download and try PVS-Studio on the code of their project.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Egor Bredikhin. Check out the Krita 4.0 Free Source Code Graphics Editor
Introduction
It is noteworthy that the developers have already used PVS-Studio in the distant 2015 on the Krita 2.9.2 version and successfully fixed the errors with it. However, apparently, the analyzer has not been used since. In all our articles, we say that regular checks are important, because if the developers continued to use PVS-Studio, the errors that I will discuss in this article would simply not be released.
Useless range-based for
PVS-Studio warning : V714 Variable row is not passed through 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, the programmer obviously wanted to multiply each element of the rows container by invM33 , however, this will not happen. At each iteration of the loop, a new variable is created with the name row , which is then simply destroyed. To correct the error, you must create a link to the element stored in the container:
for (auto &row : rows) { row *= invM33; } Incorrect conditions
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] = .... } } } .... } The program will never enter the block under the condition j == 0 , since this condition is always false due to the fact that the restriction j> 0 is imposed above in the for loop.
Similar analyzer warnings:
- V547 Expression 'x <0' is always false. kis_selection_filters.cpp 334
- V547 Expression 'y <0' is always false. kis_selection_filters.cpp 342
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 inside this complex condition there is a check of the form (! A || a) :
d->documentLayout->changeTracker()->displayChanges() || !d->documentLayout->changeTracker()->displayChanges() Such a condition is always true, because of which all this big test becomes meaningless, and the analyzer reports.
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, the value of the type const char , obtained by de-referencing the src pointer , is written to the n- type variable qint32 , so the range of valid values for the variable n : [-128; 127]. Then the variable n is compared with the number 128, although it is clear that the result of such a test is always false .
Note: The project is built without -funsigned-char .
PVS-Studio warning : V590 Consider inspecting the 'state == (- 3) || state! = 0 'expression. The expression is 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 they were too smart with the second condition, because of which it became redundant. I will assume that the correct version is 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 detected a condition of the form (a> b && a <b) , which, obviously, is always false. It is difficult to say exactly what the author wanted to write, however, this code is clearly erroneous and needs to be corrected.
Ochepyatki
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(....); } .... } The programmer made a mistake and instead of the operator && wrote the operator || , because of what all his condition became meaningless, because the condition of the form turned out: 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 is similar to the previous one: mixed up the logical operator and instead of && wrote || .
PVS-Studio warning : V501 There are identical sub-expressions 'sensor (FUZZY_PER_DAB, true)' 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 detected a situation where, to the left and right of the operator || there are identical expressions. If you look at the DynamicSensorType enumeration:
enum DynamicSensorType { FUZZY_PER_DAB, FUZZY_PER_STROKE, SPEED, FADE, .... UNKNOWN = 255 }; it becomes clear that, on the right, most likely they wanted to write FUZZY_PER_STROKE , and not FUZZY_PER_DAB .
Static analyzers do well in finding such errors, but they are easy to overlook in code review, especially when you need to view a large number of changes.
Copy-Paste errors
PVS-Studio warning : V583 The '?:' Operator, regardless of its conditional expression, always returns the same value: d-> paragraphStylesDotXmlStyles.values (). KoTextSharedLoadingData.cpp 594
QList<KoParagraphStyle *> KoTextSharedLoadingData::paragraphStyles(bool stylesDotXml) const { return stylesDotXml ? d->paragraphStylesDotXmlStyles.values() : d->paragraphStylesDotXmlStyles.values(); // <= } Here, most likely, they copied the then block in the ternary operator and forgot to change the name of the method being called, which is why, 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, you need to write paragraphContentDotXmlStyles , instead of paragraphStylesDotXmlStyles .
PVS-Studio warning : V583 The '?:' Operator, regardless of its conditional expression, 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 trigger, very similar to the previous one. Perhaps, in the then block of the first ternary operator, they wanted to write qCeil (axis) , not qFloor (axis) , or else the condition here is superfluous.
PVS-Studio Warning: V656 Variables 'vx', 'vy' are initialized. It's probably not 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, because, most likely, when writing a formula for vy, they copied the previous line, but forgot to change the x () calls to y () . In case there is no error here, it is better to rewrite the code like this:
qreal vx = 3 * curve[2].x() - 2 * curve[3].x() - curve[0].x(); qreal vy = vx; PVS-Studio warning : V581 The statement expressions of the "if" statements stand 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 test is performed twice. In this method, either copied too much, or something mixed up. If there is no error, then it is worth removing the duplicate 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); // <= } .... } Most likely, here we copied the code from the then block into the else block and forgot to change the called method. Judging by the project code, in the else branch, they probably wanted to write KisLayerUtils :: updateFrameJobs .
Duplicate condition (error in condition)
PVS-Studio warning : V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 the same check is performed twice. Most likely, in the second case, it was necessary to write something like "sender-title" .
Errors when working with constants from enumeration
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 an OpenFlag enumeration element with a value of 0x2 :
enum OpenFlag { None = 0, Import = 0x1, BatchMode = 0x2, RecoveryFile = 0x4 }; However, in this example, it is treated as if it were a variable. The result is that part of the condition is always true.
In this case, in the code above, BatchMode works correctly:
if (flags & BatchMode) { newdoc->setFileBatchMode(true); } From this we can conclude that they 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, they mixed up the operators and instead of | inside the mask used the operator && . I think that the corrected version should look like this:
void paint(....) const override { QStyledItemDelegate::paint(painter, option, index); if(!(option.state & (int)(QStyle::State_Active | QStyle::State_Enabled))) { .... } } Suspicious reassignments
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 value is assigned some value inside the condition, however, then this value is immediately overwritten. Most likely, there is some kind of mistake.
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 two different times in a row. This is suspicious, and it is possible that they wanted to assign a value to some other variable.
Maybe skipped else
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, you may have forgotten the keyword else . Even if there is no error, you should correct the formatting of the code so as not to embarrass the analyzer and other programmers.
Similar warning:
- V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. transform_stroke_strategy.cpp 166
Problems with null pointers
PVS-Studio warning : V595 The 'l' pointer was used 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 then checked for nullptr .
Similar analyzer warnings:
- V595 'ient ient ient pointer pointer pointer pointer pointer Check lines: 164, 166. kis_gradient_chooser.cc 164
- V595 The 'm_currentShape' pointer was used before it was verified against nullptr. Check lines: 316, 325. ArtisticTextTool.cpp 316
- V595 The 'painter ()' pointer was used before it was verified against nullptr. Check lines: 87, 89. kis_grid_paintop.cpp 87
- V595 The 'm_optionsWidget' pointer was used before it was verified against nullptr. Check lines: 193, 202. kis_tool_move.cc 193
- ....
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 such use of the sb pointer to be unsafe after checking it for nullptr . Indeed, in case the pointer is zero (and this is allowed, since such a condition is written above), then when calling sb-> isHidden () a dereferencing of the zero pointer may occur. As a correction, you can add a check for nullptr and in the second condition, or else somehow handle this situation.
Similar analyzer warning:
- V1004 The 'd-> viewManager' pointer was used unsafely after it was verified against nullptr. Check lines: 338, 365. KisView.cpp 365
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 null slot pointer is guaranteed to dereference, which in turn leads to undefined program behavior.
Memory leaks
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, they forgot to free up the memory allocated for svgSymbol . This is a memory leak. There are many similar leaks in the project, but they are of the same type, so I will not dwell on them.
Similar analyzer warnings:
- V773 The function of the ppmFlow pointer. A memory leak is possible. kis_ppm_import.cpp 249
- V773 The keyShortcut 'pointer. A memory leak is possible. kis_input_manager_p.cpp 443
- V773 The function was exorted without releasing the 'layer Record' pointer. A memory leak is possible. psd_layer_section.cpp 109
- V773 The function of the filterStack 'pointer. A memory leak is possible. FilterEffectResource.cpp 139
- ....
Checks for nullptr after new
PVS-Studio warning : V668 against S S S S S S S S S. 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; // <= .... } } This column in our articles, it seems, has already become permanent. It makes no sense to check the pointer to nullptr if the memory has been allocated using the new operator. If memory cannot be allocated, the new operator throws an exception std :: bad_alloc () , and does not return nullptr . To fix this code, you can add an exception handler, or use new (std :: nothrow) .
Similar analyzer warnings:
- V668 allocated allocated using factory The exception will be generated in the case of memory allocation error. TestKoShapeFactory.cpp 36
- V668 using S S S S allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated The exception will be generated in the case of memory allocation error. ParagraphGeneral.cpp 199
- V668 has been defined as the "new" operator. The exception will be generated in the case of memory allocation error. multi_bspline_create.cpp 460
- The m_currentStrategy 'has been defined using the' new 'operator. The exception will be generated in the case of memory allocation error. ConnectionTool.cpp 333
- ....
Refactoring
PVS-Studio warning : V728 Alert can be simplified. The '||' operator juxgler '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:
- V728 Anonymous check can be simplified. The '||' operator is surrounded by opposite expressions '! m_currentFilterConfigWidget' and 'm_currentFilterConfigWidget'. kis_filter_option.cpp 111
PVS-Studio warning : V501 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 for loop for errors. If there are no errors, it is worth removing the duplicate check.
Similar analyzer warning:
- V501 Operator:! Iterator.atEnd () &&! Iterator.atEnd () KoTextDebug.cpp 909
PVS-Studio warning : V799 The 'cmd' variable is not used after it has been allocated. 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 they allocated and freed up memory for the cmd object, but never used it.
PVS-Studio warning : V732 Unary minus operator doesn’t 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 type bool . Using the unary minus, the variable is implicitly converted to the type int and the resulting number is passed to the adjusted () method. Such code complicates understanding. Explicit is better than implicit, so I think it's worth rewriting 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:
- V732 Unary minus operator value Consider using the '!' operator. kis_equalizer_button.cpp 66
- V732 Unary minus operator value Consider using the '!' operator. kis_duplicateop.cpp 222
Conclusion
In conclusion, I would like to contact the developers of Krita and offer them to resume using our analyzer free of charge.
Since the last time developers used PVS-Studio, we had versions for Linux and MacOS (I checked this project in Linux myself), and the analysis also improved significantly.
In addition, I invite everyone to download and try PVS-Studio on the code of their project.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Egor Bredikhin. Check out the Krita 4.0 Free Source Code Graphics Editor
Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.
Source: https://habr.com/ru/post/354120/
All Articles