Относительно недавно состоялся релиз фреймворка Qt 6, и это стало поводом вновь проверить его с помощью PVS-Studio. В статье будут рассмотрены различные интересные ошибки, например, связанные с обработкой дат. Обнаружение всех этих ошибок хорошо демонстрирует пользу, которую может получить проект от использования таких инструментов, как PVS-Studio, особенно если они применяются регулярно.
Это классическая статья о проверке открытого проекта, которая пополнит нашу "доказательную базу" полезности и эффективности использования PVS-Studio для контроля качества кода. Хотя мы уже писали про поверку проекта Qt (в 2011, в 2014 и в 2018), очень полезно сделать это вновь. Так, мы на практике демонстрируем простую, но очень важную мысль: статический анализ должен применяться регулярно!
Наши статьи показывают, что анализатор PVS-Studio умеет находить множество разнообразнейших ошибок. И, как правило, авторы проектов быстро исправляют описанные нами ошибки. Однако всё это не имеет ничего общего с правильной и полезной методикой регулярного применения статических анализаторов. Когда анализатор встроен в процесс разработки, это позволяет быстро находить ошибки в новом или изменённом коде и тем самым их исправление обходится максимально дёшево.
Всё, теория заканчивается. Давайте посмотрим, что интересного нас ждёт в коде. А пока вы будете читать статью, предлагаю скачать PVS-Studio и запросить демонстрационный ключ, чтобы посмотреть, что интересного найдётся в ваших собственных проектах :).
Даты
Кажется, намечается ещё один паттерн кода, где любят собираться ошибки. Он, конечно, не такой масштабный, как функции сравнения или последние строки в однотипных блоках. Речь идёт про фрагменты кода, работающие с датами. Наверное, такой код сложно тестировать, и в итоге эти недотестированные функции будут давать некорректный результат на определённых наборах входных данных. Про пару таких случаев уже было рассказано в статье "Почему PVS-Studio не предлагает автоматические правки кода".
Встретились ошибки обработки дат и в Qt. Давайте с них и начнём.
Фрагмент N1: неправильная интерпретация статуса ошибки
Для начала нам следует посмотреть, как устроена функция, возвращающая номер месяца по его сокращённому названию.
static const char qt_shortMonthNames[][4] = {
"Jan", "Feb", "Mar", "Apr", "May", "Jun",
"Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
};
static int fromShortMonthName(QStringView monthName)
{
for (unsigned int i = 0;
i < sizeof(qt_shortMonthNames) / sizeof(qt_shortMonthNames[0]); ++i)
{
if (monthName == QLatin1String(qt_shortMonthNames[i], 3))
return i + 1;
}
return -1;
}В случае успеха функция возвращает номер месяца (значение от 1 до 12). Если имя месяца некорректно, то функция возвращает отрицательное значение (-1). Обратите внимание, что функция не может вернуть значение 0.
Но там, где только что рассмотренная функция используется, программист как раз рассчитывает, что в качестве статуса ошибки ему будет возвращено нулевое значение. Фрагмент кода, некорректно использующий функцию fromShortMonthName:
QDateTime QDateTime::fromString(QStringView string, Qt::DateFormat format)
{
....
month = fromShortMonthName(parts.at(1));
if (month)
day = parts.at(2).toInt(&ok);
// If failed, try day then month
if (!ok || !month || !day) {
month = fromShortMonthName(parts.at(2));
if (month) {
QStringView dayPart = parts.at(1);
if (dayPart.endsWith(u'.'))
day = dayPart.chopped(1).toInt(&ok);
}
}
....
}Проверки номера месяца на равенство нулю никогда не сработают, и программа продолжит выполняться с некорректным отрицательным номером месяца. Анализатор PVS-Studio видит здесь целый букет несостыковок, о чем сообщает сразу четырьмя сообщениями:
- V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4907
- V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4911
- V547 [CWE-571] Expression 'month' is always true. qdatetime.cpp 4913
- V560 [CWE-570] A part of conditional expression is always false: !month. qdatetime.cpp 4921
Фрагмент N2: ошибка логики обработки даты
Для начала посмотрим на реализацию функции, возвращающей количество секунд.
enum {
....
MSECS_PER_DAY = 86400000,
....
SECS_PER_MIN = 60,
};
int QTime::second() const
{
if (!isValid())
return -1;
return (ds() / 1000)%SECS_PER_MIN;
}Рассмотренная функция может вернуть значение в диапазоне [0..59] или статус ошибки -1.
В одном месте эта функция используется очень странным образом:
static qint64 qt_mktime(QDate *date, QTime *time, ....)
{
....
} else if (yy == 1969 && mm == 12 && dd == 31
&& time->second() == MSECS_PER_DAY - 1) {
// There was, of course, a last second in 1969, at time_t(-1); we won't
// rescue it if it's not in normalised form, and we don't know its DST
// status (unless we did already), but let's not wantonly declare it
// invalid.
} else {
....
}Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: time->second() == MSECS_PER_DAY — 1. qdatetime.cpp 2488
Согласно комментарию, если что-то пошло не так, то лучше ничего не делать. Однако, условие всегда будет ложным, поэтому всегда выполняется else-ветка.
Ошибочно вот это сравнение:
time->second() == MSECS_PER_DAY - 1MSECS_PER_DAY — 1 это 86399999. Функция second, как мы уже знаем, никак не может вернуть такое значение. Таким образом, здесь какая-то логическая ошибка и код заслуживает пристального внимания разработчиков.
Сильная сторона статических анализаторов кода в том, что они проверяют все ситуации, независимо от частоты их выполнения. Таким образом, статический анализ хорошо дополняет юнит-тесты и другие методы контроля качества кода.
Опечатки
Фрагмент N3: неожиданно, мы поговорим о… HTML!
QString QPixelTool::aboutText() const
{
const QList<QScreen *> screens = QGuiApplication::screens();
const QScreen *windowScreen = windowHandle()->screen();
QString result;
QTextStream str(&result);
str << "<html></head><body><h2>Qt Pixeltool</h2><p>Qt " << QT_VERSION_STR
<< "</p><p>Copyright (C) 2017 The Qt Company Ltd.</p><h3>Screens</h3><ul>";
for (const QScreen *screen : screens)
str << "<li>" << (screen == windowScreen ? "* " : " ")
<< screen << "</li>";
str << "<ul></body></html>";
return result;
}Предупреждение PVS-Studio: V735 Possibly an incorrect HTML. The "</ body>" closing tag was encountered, while the "</ ul>" tag was expected. qpixeltool.cpp 707
В PVS-Studio есть диагностики, которые не только проверят сам код, но и выискивают аномалии в строковых константах. Здесь как раз сработала одна из таких диагностик. Это достаточно редкий случай, зато этим он и примечательный.
Дважды используется тег открытия списка. Это явная опечатка. Первый тег, должен открывать список, а второй – закрывать. Правильный код:
str << "</ul></body></html>";Фрагмент N4: повторная проверка в условии
class Node
{
....
bool isGroup() const { return m_nodeType == Group; }
....
};
void DocBookGenerator::generateDocBookSynopsis(const Node *node)
{
....
if (node->isGroup() || node->isGroup()
|| node->isSharedCommentNode() || node->isModule()
|| node->isJsModule() || node->isQmlModule() || node->isPageNode())
return;
....
}Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the '||' operator: node->isGroup() || node->isGroup() docbookgenerator.cpp 2599
Простая опечатка, но её исправление зависит от того, чего на самом деле хотели достичь в этом коде. Если проверка просто дублируется, то её стоит просто удалить. Но возможен и другой сценарий: не проверено какое-то другое нужное условие.
Фрагмент N5: создание лишней локальной переменной
void MainWindow::addToPhraseBook()
{
....
QString selectedPhraseBook;
if (phraseBookList.size() == 1) {
selectedPhraseBook = phraseBookList.at(0);
if (QMessageBox::information(this, tr("Add to phrase book"),
tr("Adding entry to phrasebook %1").arg(selectedPhraseBook),
QMessageBox::Ok | QMessageBox::Cancel, QMessageBox::Ok)
!= QMessageBox::Ok)
return;
} else {
bool okPressed = false;
QString selectedPhraseBook =
QInputDialog::getItem(this, tr("Add to phrase book"),
tr("Select phrase book to add to"),
phraseBookList, 0, false, &okPressed);
if (!okPressed)
return;
}
MessageItem *currentMessage = m_dataModel->messageItem(m_currentIndex);
Phrase *phrase = new Phrase(currentMessage->text(),
currentMessage->translation(),
QString(), nullptr);
phraseBookHash.value(selectedPhraseBook)->append(phrase);
}Кстати, если хотите, можете проверить свою внимательность и самостоятельно поискать ошибку. А чтобы вы сразу случайно не увидели ответ, добавлю сюда единорожку из старой коллекции. Возможно, вы даже его ещё не видели :).
Предупреждение PVS-Studio: V561 [CWE-563] It's probably better to assign value to 'selectedPhraseBook' variable than to declare it anew. Previous declaration: mainwindow.cpp, line 1303. mainwindow.cpp 1313
В обоих ветках условного оператора формируется текст, который должен быть записан в переменную selectedPhraseBook. Имя переменной длинное, и программисту было лень писать его заново. Поэтому он скопировал имя переменной из того места, где она объявляется. При этом он чуть поспешил и вместе с именем скопировал и тип переменной:
QString selectedPhraseBook =В результате в else-блоке появилась ещё одна локальная строковая переменная, которая инициализируется, но не используется. Тем временем переменная с таким же именем во внешнем блоке останется пустой.
Фрагмент N6: приоритет операций
Классический паттерн ошибки, который встречается весьма часто.
bool QQmlImportInstance::resolveType(....)
{
....
if (int icID = containingType.lookupInlineComponentIdByName(typeStr) != -1)
{
*type_return = containingType.lookupInlineComponentById(icID);
} else {
auto icType = createICType();
....
}
....
}Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. qqmlimport.cpp 754
Значение переменной icID всегда будет иметь значение 0 или 1. Это явно не то, что задумывалось. Причина: в начале происходит сравнение с -1, а только затем инициализация переменной icID.
Современный синтаксис C++ позволяет корректно записать это условие следующим образом:
if (int icID = containingType.lookupInlineComponentIdByName(typeStr);
icID != -1)Кстати, очень похожую ошибку мы уже обнаруживали в Qt:
char ch;
while (i < dataLen && ((ch = data.at(i) != '\n') && ch != '\r'))
++i;Но пока на вооружение не будет взят такой анализатор кода, как PVS-Studio, программисты вновь и вновь будут допускать такие ошибки. Никто не совершенен. И да, это тонкий намёк внедрить PVS-Studio :).
Фрагмент N7: коварное деление по модулю
Часто бывает необходимо определить, делится число без остатка на 2 или нет. Правильный вариант — это поделить по модулю два и проверить результат:
if (A % 2 == 1)Но программисты вновь и вновь ошибаются и пишут что-то типа этого:
if (A % 1 == 1)Это естественно неправильно, так как остаток от деления по модулю на один – это всегда ноль. Не обошлось без этой ошибки и в Qt:
bool loadQM(Translator &translator, QIODevice &dev, ConversionData &cd)
{
....
case Tag_Translation: {
int len = read32(m);
if (len % 1) { // <=
cd.appendError(QLatin1String("QM-Format error"));
return false;
}
m += 4;
QString str = QString((const QChar *)m, len/2);
....
}Предупреждение PVS-Studio: V1063 The modulo by 1 operation is meaningless. The result will always be zero. qm.cpp 549
Фрагмент N8: перезаписывание значения
QString Node::qualifyQmlName()
{
QString qualifiedName = m_name;
if (m_name.startsWith(QLatin1String("QML:")))
qualifiedName = m_name.mid(4);
qualifiedName = logicalModuleName() + "::" + m_name;
return qualifiedName;
}Предупреждение PVS-Studio: V519 [CWE-563] The 'qualifiedName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1227, 1228. node.cpp 1228
Как я понимаю, случайно использовали не то имя переменной. Предполагаю, здесь должно быть написано так:
QString qualifiedName = m_name;
if (m_name.startsWith(QLatin1String("QML:")))
qualifiedName = m_name.mid(4);
qualifiedName = logicalModuleName() + "::" + qualifiedName;
return qualifiedName;Фрагмент N9: copy-paste
class Q_CORE_EXPORT QJsonObject
{
....
bool operator<(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }
bool operator<=(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }
....
}Прудпреждение PVS-Studio: V524 It is odd that the body of '<=' function is fully equivalent to the body of '<' function. qjsonobject.h 155
Такие скучные функции, как операторы сравнения, никто не проверяет. На них обычно не пишут тесты. Их не просматривают или делают это очень быстро на code review. А зря. Статический анализ кода здесь как нельзя кстати. Анализатор не устаёт и с удовольствием проверяет даже такой скучный код.
Реализация операторов < и <= совпадают. Это явно неправильно. Скорее всего, код писался методом Copy-Paste, и затем забыли изменить всё что нужно в скопированном коде. Правильно:
bool operator<(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index < other.item.index; }
bool operator<=(const iterator& other) const
{ Q_ASSERT(item.o == other.item.o); return item.index <= other.item.index; }Фрагмент N10: static_cast / dynamic_cast
void QSGSoftwareRenderThread::syncAndRender()
{
....
bool canRender = wd->renderer != nullptr;
if (canRender) {
auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);
if (softwareRenderer)
softwareRenderer->setBackingStore(backingStore);
....
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'softwareRenderer' is always true. qsgsoftwarethreadedrenderloop.cpp 510
В начале рассмотрим вот эту проверку:
bool canRender = wd->renderer != nullptr;
if (canRender) {Благодаря ей можно быть уверенным, что внутри тела условного оператора значение указателя wd->renderer всегда точно ненулевое. Тогда непонятно, что же хотят проверить следующим кодом?
auto softwareRenderer = static_cast<QSGSoftwareRenderer*>(wd->renderer);
if (softwareRenderer)Если указатель wd->renderer ненулевой, то и указатель softwareRenderer точно ненулевой. Есть подозрение, что здесь опечатка, которая состоит в том, что на самом деле следовало использовать dynamic_cast. В этом случае код начинает приобретать смысл. Если преобразование типа невозможно, оператор dynamic_cast возвращает nullptr, и это возвращенное значение, естественно, следует проверять. Впрочем, возможно, я неправильно интерпретировал ситуацию и код нужно исправлять другим способом.
Фрагмент N11: скопировали блок кода и забыли изменить
void *QQuickPath::qt_metacast(const char *_clname)
{
if (!_clname) return nullptr;
if (!strcmp(_clname, qt_meta_stringdata_QQuickPath.stringdata0))
return static_cast<void*>(this);
if (!strcmp(_clname, "QQmlParserStatus"))
return static_cast< QQmlParserStatus*>(this);
if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus")) // <=
return static_cast< QQmlParserStatus*>(this);
if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus")) // <=
return static_cast< QQmlParserStatus*>(this);
return QObject::qt_metacast(_clname);
}Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2719, 2721. moc_qquickpath_p.cpp 2721
Эти две строчки:
if (!strcmp(_clname, "org.qt-project.Qt.QQmlParserStatus"))
return static_cast< QQmlParserStatus*>(this);Были размножены с помощью Copy-Paste. После чего они не были модифицированы и не имеют смысла.
Фрагмент N12: переполнение из-за не там поставленной скобки
int m_offsetFromUtc;
....
void QDateTime::setMSecsSinceEpoch(qint64 msecs)
{
....
if (!add_overflow(msecs, qint64(d->m_offsetFromUtc * 1000), &msecs))
status |= QDateTimePrivate::ValidWhenMask;
....
}Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands of the 'd->m_offsetFromUtc * 1000' operator to the 'qint64' type, not the result. qdatetime.cpp 3922
Программист предвидит ситуацию, что при умножении переменной типа int на 1000 может произойти переполнение. Чтобы этого избежать, он планирует использовать при умножении 64-битный тип qint64. И использует явное приведение типа.
Вот только толку от этого приведения типа никакого нет. В начале всё равно произойдёт переполнение. И только затем выполнится приведение типа. Правильный вариант:
add_overflow(msecs, qint64(d->m_offsetFromUtc) * 1000, &msecs)Фрагмент N13: не полностью инициализированный массив
class QPathEdge
{
....
private:
int m_next[2][2];
....
};
inline QPathEdge::QPathEdge(int a, int b)
: flag(0)
, windingA(0)
, windingB(0)
, first(a)
, second(b)
, angle(0)
, invAngle(0)
{
m_next[0][0] = -1;
m_next[1][0] = -1;
m_next[0][0] = -1;
m_next[1][0] = -1;
}Предупреждения PVS-Studio:
- V1048 [CWE-1164] The 'm_next[0][0]' variable was assigned the same value. qpathclipper_p.h 301
- V1048 [CWE-1164] The 'm_next[1][0]' variable was assigned the same value. qpathclipper_p.h 302
Перед нами неудачная попытка инициализировать массив размером 2x2. Два элемента инициализируются повторно, и два остаются неинициализированными. Правильный вариант:
m_next[0][0] = -1;
m_next[0][1] = -1;
m_next[1][0] = -1;
m_next[1][1] = -1;Я очень люблю такие примеры ошибок, которые встречаются в коде профессиональных разработчиков. Это как раз такой случай. Он показывает, что любой может опечататься, и поэтому статический анализ является вашим другом. Дело в том, что я уже десяток лет веду бой со скептиками, которые уверены, что такие ошибки можно встретить только в лабораторных работах студентов и что они такие ошибки никогда не делают :). Ещё 10 лет назад я написал заметку "Миф второй – профессиональные разработчики не допускают глупых ошибок", и с тех пор, естественно, ничего не изменилось. Люди всё также делают такие ошибки и всё также утверждают, что это не так :).
Ошибки в логике
Фрагмент N14: недостижимый код
void QmlProfilerApplication::tryToConnect()
{
Q_ASSERT(!m_connection->isConnected());
++ m_connectionAttempts;
if (!m_verbose && !(m_connectionAttempts % 5)) {// print every 5 seconds
if (m_verbose) {
if (m_socketFile.isEmpty())
logError(
QString::fromLatin1("Could not connect to %1:%2 for %3 seconds ...")
.arg(m_hostName).arg(m_port).arg(m_connectionAttempts));
else
logError(
QString::fromLatin1("No connection received on %1 for %2 seconds ...")
.arg(m_socketFile).arg(m_connectionAttempts));
}
}
....
}Предупреждение PVS-Studio: V547 [CWE-570] Expression 'm_verbose' is always false. qmlprofilerapplication.cpp 495
Этот код никогда ничего не запишет в лог. Причиной являются противоположные условия:
if (!m_verbose && ....) {
if (m_verbose) {Фрагмент N15: перетирание значения переменной
void QRollEffect::scroll()
{
....
if (currentHeight != totalHeight) {
currentHeight = totalHeight * (elapsed/duration)
+ (2 * totalHeight * (elapsed%duration) + duration)
/ (2 * duration);
// equiv. to int((totalHeight*elapsed) / duration + 0.5)
done = (currentHeight >= totalHeight);
}
done = (currentHeight >= totalHeight) &&
(currentWidth >= totalWidth);
....
}Предупреждение PVS-Studio: V519 [CWE-563] The 'done' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 509, 511. qeffects.cpp 511
Весь условный оператор не имеет смысла, так как значение переменной done всё равно тут же перезаписывается. Возможно, здесь не хватает ключевого слова else.
Фрагмент N16-N20: перетирание значения переменной
Альтернативный вариант перетирания значения переменной.
bool QXmlStreamWriterPrivate::finishStartElement(bool contents)
{
....
if (inEmptyElement) {
....
lastNamespaceDeclaration = tag.namespaceDeclarationsSize; // <=
lastWasStartElement = false;
} else {
write(">");
}
inStartElement = inEmptyElement = false;
lastNamespaceDeclaration = namespaceDeclarations.size(); // <=
return hadSomethingWritten;
}Предупреждение PVS-Studio: V519 [CWE-563] The 'lastNamespaceDeclaration' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3030, 3036. qxmlstream.cpp 3036
Возможно, первая запись значения в переменную lastNamespaceDeclaration является лишней и её можно удалить. А возможно, перед нами серьезная логическая ошибка.
Есть ещё 4 предупреждения, указывающих на такой же паттерн ошибки:
- V519 [CWE-563] The 'last' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 609, 637. qtextengine.cpp 637
- V519 [CWE-563] The 'm_dirty' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1014, 1017. qquickshadereffect.cpp 1017
- V519 [CWE-563] The 'changed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 122, 128. qsgdefaultspritenode.cpp 128
- V519 [CWE-563] The 'eaten' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 299, 301. qdesigner.cpp 301
Фрагмент N21: путаница между нулевым указателем и пустой строкой
// this could become a list of all languages used for each writing
// system, instead of using the single most common language.
static const char languageForWritingSystem[][6] = {
"", // Any
"en", // Latin
"el", // Greek
"ru", // Cyrillic
...... // Нулевых указателей нет. Используются пустые строковые литералы.
"", // Symbol
"sga", // Ogham
"non", // Runic
"man" // N'Ko
};
static void populateFromPattern(....)
{
....
for (int j = 1; j < QFontDatabase::WritingSystemsCount; ++j) {
const FcChar8 *lang = (const FcChar8*) languageForWritingSystem[j];
if (lang) {
....
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'lang' is always true. qfontconfigdatabase.cpp 462
В массиве languageForWritingSystem нет нулевых указателей. Поэтому проверка if(lang) не имеет смысла. Зато в массиве есть пустые строки. Быть может, хотелось сделать проверку именно на пустую строку? Если да, тогда корректный код должен выглядеть так:
if (strlen(lang) != 0) {Или можно ещё проще написать:
if (lang[0] != '\0') {Фрагмент N22: странная проверка
bool QNativeSocketEnginePrivate::createNewSocket(....)
{
....
int socket = qt_safe_socket(domain, type, protocol, O_NONBLOCK);
....
if (socket < 0) {
....
return false;
}
socketDescriptor = socket;
if (socket != -1) {
this->socketProtocol = socketProtocol;
this->socketType = socketType;
}
return true;
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'socket != — 1' is always true. qnativesocketengine_unix.cpp 315
Условие socket != -1 всегда истинно, так как выше происходит выход из функции, если значение переменной socket отрицательное.
Фрагмент N23: так что же всё-таки должна вернуть функция?
bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent)
{
Q_D(QSqlTableModel);
if (parent.isValid() || row < 0 || count <= 0)
return false;
else if (row + count > rowCount())
return false;
else if (!count)
return true;
....
}Предупреждение PVS-Studio: V547 [CWE-570] Expression '!count' is always false. qsqltablemodel.cpp 1110
Для упрощения выделю самое главное:
if (.... || count <= 0)
return false;
....
else if (!count)
return true;Первая проверка говорит нам, что если значение count меньше или равно 0, то это ошибочное состояние и функция должна вернуть false. Однако ниже мы видим точное сравнение этой переменной с нулём, и этот случай уже интерпретируется по-другому: функция должна вернуть true.
Здесь явно что-то не так. Я подозреваю, что на самом деле проверка должна быть не <=, а просто <. Тогда код обретает смысл:
bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent)
{
Q_D(QSqlTableModel);
if (parent.isValid() || row < 0 || count < 0)
return false;
else if (row + count > rowCount())
return false;
else if (!count)
return true;
....
}Фрагмент N24: лишний статус?
В следующем коде переменная identifierWithEscapeChars выглядит просто как лишняя сущность. Или это логическая ошибка? Или код не дописан? К моменту второй проверки эта переменная в любом случае всегда будет равна true.
int Lexer::scanToken()
{
....
bool identifierWithEscapeChars = false;
....
if (!identifierWithEscapeChars) {
identifierWithEscapeChars = true;
....
}
....
if (identifierWithEscapeChars) { // <=
....
}
....
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'identifierWithEscapeChars' is always true. qqmljslexer.cpp 817
Фрагмент N25: что делать с девятью объектами?
bool QFont::fromString(const QString &descrip)
{
....
const int count = l.count();
if (!count || (count > 2 && count < 9) || count == 9 || count > 17 ||
l.first().isEmpty()) {
qWarning("QFont::fromString: Invalid description '%s'",
descrip.isEmpty() ? "(empty)" : descrip.toLatin1().data());
return false;
}
setFamily(l[0].toString());
if (count > 1 && l[1].toDouble() > 0.0)
setPointSizeF(l[1].toDouble());
if (count == 9) { // <=
setStyleHint((StyleHint) l[2].toInt());
setWeight(QFont::Weight(l[3].toInt()));
setItalic(l[4].toInt());
setUnderline(l[5].toInt());
setStrikeOut(l[6].toInt());
setFixedPitch(l[7].toInt());
} else if (count >= 10) {
....
}Предупреждение PVS-Studio: V547 [CWE-570] Expression 'count == 9' is always false. qfont.cpp 2142
Как должна себя вести функция, если переменная count равна 9? С одной стороны, функция должна выдать предупреждение и завершить свою работу. Ведь явно написано:
if (.... || count == 9 || ....) {
qWarning(....);
return false;
}С другой стороны, для 9 объектов предусмотрено выполнение специального кода:
if (count == 9) {
setStyleHint((StyleHint) l[2].toInt());
setWeight(QFont::Weight(l[3].toInt()));
setItalic(l[4].toInt());
....
}Этот код, конечно, никогда не выполняется. Код ждёт, чтобы его пришли и исправили :).
Нулевые указатели
Фрагмент N26-N42: использование указателя до его проверки
class __attribute__((visibility("default"))) QMetaType {
....
const QtPrivate::QMetaTypeInterface *d_ptr = nullptr;
};
QPartialOrdering QMetaType::compare(const void *lhs, const void *rhs) const
{
if (!lhs || !rhs)
return QPartialOrdering::Unordered;
if (d_ptr->flags & QMetaType::IsPointer)
return threeWayCompare(*reinterpret_cast<const void * const *>(lhs),
*reinterpret_cast<const void * const *>(rhs));
if (d_ptr && d_ptr->lessThan) {
if (d_ptr->equals && d_ptr->equals(d_ptr, lhs, rhs))
return QPartialOrdering::Equivalent;
if (d_ptr->lessThan(d_ptr, lhs, rhs))
return QPartialOrdering::Less;
if (d_ptr->lessThan(d_ptr, rhs, lhs))
return QPartialOrdering::Greater;
if (!d_ptr->equals)
return QPartialOrdering::Equivalent;
}
return QPartialOrdering::Unordered;
}Предупреждение PVS-Studio: V595 [CWE-476] The 'd_ptr' pointer was utilized before it was verified against nullptr. Check lines: 710, 713. qmetatype.cpp 710
Ошибка на первый взгляд может быть не заметна. Но на самом деле всё просто. Проследим, как работают с указателем d_ptr:
if (d_ptr->flags & ....)
if (d_ptr && ....)Вначале выполняется условие, в котором указатель разыменовывается. И только из следующей проверки становится ясно, что этот указатель, оказывается, может быть нулевой.
Это один из самых распространённых паттернов ошибки в языке C и С++. Пруфы. В исходных Qt кодах тоже встречается немало ошибок этой разновидности:
- V595 [CWE-476] The 'self' pointer was utilized before it was verified against nullptr. Check lines: 1346, 1351. qcoreapplication.cpp 1346
- V595 [CWE-476] The 'currentTimerInfo' pointer was utilized before it was verified against nullptr. Check lines: 636, 641. qtimerinfo_unix.cpp 636
- V595 [CWE-476] The 'lib' pointer was utilized before it was verified against nullptr. Check lines: 325, 333. qlibrary.cpp 325
- V595 [CWE-476] The 'fragment.d' pointer was utilized before it was verified against nullptr. Check lines: 2262, 2266. qtextcursor.cpp 2262
- V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1581, 1583. qapplication.cpp 1581
- V595 [CWE-476] The 'window' pointer was utilized before it was verified against nullptr. Check lines: 1593, 1595. qapplication.cpp 1593
- V595 [CWE-476] The 'newHandle' pointer was utilized before it was verified against nullptr. Check lines: 873, 879. qsplitter.cpp 873
- V595 [CWE-476] The 'targetModel' pointer was utilized before it was verified against nullptr. Check lines: 454, 455. qqmllistmodel.cpp 454
- V595 [CWE-476] The 'childIface' pointer was utilized before it was verified against nullptr. Check lines: 102, 104. qaccessiblequickitem.cpp 102
- V595 [CWE-476] The 'e' pointer was utilized before it was verified against nullptr. Check lines: 94, 98. qquickwindowmodule.cpp 94
- V595 [CWE-476] The 'm_texture' pointer was utilized before it was verified against nullptr. Check lines: 235, 239. qsgplaintexture.cpp 235
- V595 [CWE-476] The 'm_unreferencedPixmaps' pointer was utilized before it was verified against nullptr. Check lines: 1140, 1148. qquickpixmapcache.cpp 1140
- V595 [CWE-476] The 'camera' pointer was utilized before it was verified against nullptr. Check lines: 263, 264. assimpimporter.cpp 263
- V595 [CWE-476] The 'light' pointer was utilized before it was verified against nullptr. Check lines: 273, 274. assimpimporter.cpp 273
- V595 [CWE-476] The 'channel' pointer was utilized before it was verified against nullptr. Check lines: 337, 338. assimpimporter.cpp 337
- V595 [CWE-476] The 'm_fwb' pointer was utilized before it was verified against nullptr. Check lines: 2492, 2500. designerpropertymanager.cpp 2492
Фрагмент N43: использование указателя до его проверки в рамках одного выражения
Та же самая ситуация, что и выше. Однако в этот раз разыменование и проверка указателя находятся в одном выражении. Классическая ошибка, возникающая из-за невнимательности при написании кода и при последующем code-review.
void QFormLayoutPrivate::updateSizes()
{
....
QFormLayoutItem *field = m_matrix(i, 1);
....
if (userHSpacing < 0 && !wrapAllRows && (label || !field->fullRow) && field)
....
}Предупреждение PVS-Studio: V713 [CWE-476] The pointer 'field' was utilized in the logical expression before it was verified against nullptr in the same logical expression. qformlayout.cpp 405
Минутка отдыха
Я устал. Думаю, устали и читатели. Тут устанешь, даже если просто бегло просматривать текст статьи :). Поэтому я пошёл за второй чашечкой кофе. Первую я выпил где-то на рубеже 12-ого фрагмента. Приглашаю и читателей сходить за любимым напитком.
И пока все ушли на кухню, рекламная пауза. Приглашаю команду, занимающуюся разработкой проекта Qt рассмотреть вопрос приобретения лицензии на анализатор кода PVS-Studio. Запросить прайс можно здесь. С нашей стороны – поддержка и помощь в настройке. Да, согласен, я сегодня более навязчив, чем обычно. Это эксперимент :).
Фрагмент N44-N72: нет проверки, что вернула функция malloc
void assignData(const QQmlProfilerEvent &other)
{
if (m_dataType & External) {
uint length = m_dataLength * (other.m_dataType / 8);
m_data.external = malloc(length); // <=
memcpy(m_data.external, other.m_data.external, length); // <=
} else {
memcpy(&m_data, &other.m_data, sizeof(m_data));
}
}Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 277, 276. qqmlprofilerevent_p.h 277
Нельзя просто взять и использовать указатель, который вернула функция malloc. Нужно обязательно проверить, не является ли этот указатель нулевым, даже если очень лень всем этим заниматься. Для этого есть 4 причины, которые описаны в статье "Почему важно проверять, что вернула функция malloc".
Перед нами один из случаев, где отсутствует необходимая проверка. Есть и другие предупреждения, но из-за их количества включать весь список в статью не хочется. На всякий случай, я выписал 28 предупреждений в файл: qt6-malloc.txt. Но на самом деле разработчикам, конечно, лучше самим перепроверить проект и самостоятельно изучить предупреждения. У меня не было задачи выявить как можно больше ошибок.
Что интересно, на фоне важных забытых проверок есть совершенно ненужные. Речь идёт о вызове оператора new, который в случае ошибки выделения памяти сгенерирует исключение std::bad_alloc. Вот один из примеров такой избыточной проверки:
static QImageScaleInfo* QImageScale::qimageCalcScaleInfo(....)
{
....
QImageScaleInfo *isi;
....
isi = new QImageScaleInfo;
if (!isi)
return nullptr;
....
}Предупреждение PVS-Studio: V668 [CWE-570] There is no sense in testing the 'isi' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qimagescale.cpp 245
P.S. Здесь читатели всегда задают вопрос, учитывает ли анализатор placement new или "new (std::nothrow) T"? Да, учитывает и не выдаёт для них ложные срабатывания.
Избыточный код ("код с запахом")
Встречаются ситуации, когда анализатор выдаёт предупреждения на код, который является корректным, но избыточным. Например, есть повторная проверка одной и той же переменной. При этом даже затруднительно сказать, ложное это срабатывание или нет. Формально анализатор прав, но и настоящую ошибку он не нашёл.
Пожалуй, можно сказать, что это "код с запахом". Раз этот код смущает анализатор, он может смущать и программистов, усложняя поддержку такого кода. Приходится тратить больше времени, чтобы понять, что происходит.
Часто я вообще не рассматриваю в статьях такие предупреждения. Это неинтересно. Однако в проекте Qt вдруг таких случаев оказалось удивительно много. Явно больше, чем обычно. Поэтому я решил уделить этому немного внимания и разобрать несколько таких случаев. Думаю, будет полезно провести рефакторинг этих и многих других аналогичных мест. Для этого потребуется использовать полный отчёт, не руководствоваться тем, что я выпишу в статью.
Итак, взглянем на несколько показательных случаев.
Фрагмент N73: "код с запахом" — обратная проверка
void QQuick3DSceneManager::setWindow(QQuickWindow *window)
{
if (window == m_window)
return;
if (window != m_window) {
if (m_window)
disconnect(....);
m_window = window;
connect(....);
emit windowChanged();
}
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'window != m_window' is always true. qquick3dscenemanager.cpp 60
Если window==m_window, то функция завершает работу. Последующая обратная проверка не имеет никакого смысла и только загромождает код.
Фрагмент N74: "код с запахом" – странная инициализация
QModelIndex QTreeView::moveCursor(....)
{
....
int vi = -1;
if (vi < 0)
vi = qMax(0, d->viewIndex(current));
....
}Предупреждение PVS-Stduio: V547 [CWE-571] Expression 'vi < 0' is always true. qtreeview.cpp 2219
Что это? Зачем так писать? Код можно упростить до одной строки:
int vi = qMax(0, d->viewIndex(current));Фрагмент N75: "код с запахом" – недостижимый код
bool copyQtFiles(Options *options)
{
....
if (unmetDependencies.isEmpty()) {
if (options->verbose) {
fprintf(stdout, " -- Skipping %s, architecture mismatch.\n",
qPrintable(sourceFileName));
}
} else {
if (unmetDependencies.isEmpty()) {
if (options->verbose) {
fprintf(stdout, " -- Skipping %s, architecture mismatch.\n",
qPrintable(sourceFileName));
}
} else {
fprintf(stdout, " -- Skipping %s. It has unmet dependencies: %s.\n",
qPrintable(sourceFileName),
qPrintable(unmetDependencies.join(QLatin1Char(','))));
}
}
....
}Предупреждение PVS-Studio: V571 [CWE-571] Recurring check. The 'if (unmetDependencies.isEmpty())' condition was already verified in line 2203. main.cpp 2209
На первый взгляд перед нами респектабельный код, формирующий подсказку. Но давайте приглядимся. Если первый раз условие unmetDependencies.isEmpty() выполнилось, то второй раз этого уже не произойдёт. Это нестрашно, так как автор планировал вывести то же самое сообщение. Настоящей ошибки нет, но код переусложнён. Он может быть упрощен до следующего варианта:
bool copyQtFiles(Options *options)
{
....
if (unmetDependencies.isEmpty()) {
if (options->verbose) {
fprintf(stdout, " -- Skipping %s, architecture mismatch.\n",
qPrintable(sourceFileName));
}
} else {
fprintf(stdout, " -- Skipping %s. It has unmet dependencies: %s.\n",
qPrintable(sourceFileName),
qPrintable(unmetDependencies.join(QLatin1Char(','))));
}
....
}Фрагмент N76: "код с запахом" – сложный тернарный оператор
bool QDockAreaLayoutInfo::insertGap(....)
{
....
QDockAreaLayoutItem new_item
= widgetItem == nullptr
? QDockAreaLayoutItem(subinfo)
: widgetItem ? QDockAreaLayoutItem(widgetItem)
: QDockAreaLayoutItem(placeHolderItem);
....
}Предупреждение PVS-Studio: V547 [CWE-571] Expression 'widgetItem' is always true. qdockarealayout.cpp 1167
Возможно, здесь мы имеем дело с настоящей ошибкой. Но я больше склоняюсь к тому, что этот код несколько раз менялся и в результате получился бессмысленно сложным (избыточным). Его можно сократить до:
QDockAreaLayoutItem new_item
= widgetItem == nullptr
? QDockAreaLayoutItem(subinfo) : QDockAreaLayoutItem(widgetItem);Фрагмент N77: "код с запахом" – избыточная защита
typedef unsigned int uint;
ReturnedValue TypedArrayCtor::virtualCallAsConstructor(....)
{
....
qint64 l = argc ? argv[0].toIndex() : 0;
if (scope.engine->hasException)
return Encode::undefined();
// ### lift UINT_MAX restriction
if (l < 0 || l > UINT_MAX)
return scope.engine->throwRangeError(QLatin1String("Index out of range."));
uint len = (uint)l;
if (l != len)
scope.engine->throwRangeError(
QStringLiteral("Non integer length for typed array."));
....
}Предупреждение PVS-Studio: V547 [CWE-570] Expression 'l != len' is always false. qv4typedarray.cpp 306
Кто-то очень переживает, что значение 64-битной переменной не вмещается в 32-битную переменную unsigned. И использует сразу две проверки корректности. При этом вторая проверка избыточна.
Вот этого условия более чем достаточно:
if (l < 0 || l > UINT_MAX)Приведенный ниже фрагмент можно смело удалить, и программа менее надёжной не станет:
uint len = (uint)l;
if (l != len)
scope.engine->throwRangeError(
QStringLiteral("Non integer length for typed array."));Дальше продолжать не буду. Думаю, идею вы поняли.
Здесь можно сделать маленький вывод: результатом использования анализатора PVS-Studio будет не только устранение ошибок, но и упрощение кода.
Другие ошибки
Я остановился после того, как описал 77 дефектов. Это красивое число и выписанного более чем достаточно, чтобы написать статью. Однако это не значит, что нет других ошибок, которые способен выявить PVS-Studio. При изучении лога я был весьма поверхностен и пропускал всё, где нужно было разбираться более пары минут, ошибка это или нет :).
Поэтому я всегда прошу не полагаться на ошибки, которые мы описываем в наших статьях, а выполнять анализ проектов самостоятельно.
Заключение
Статический анализ – это круто! После внедрения PVS-Studio будет экономить время и нервы, выявляя множество ошибок сразу после написания кода. Намного лучше искать на code review с коллегами не опечатки, а высокоуровневые ошибки и обсуждать эффективность реализованного алгоритма. Тем более, как показывает практика, эти дурацкие опечатки всё равно отлично прячутся при просмотре кода глазами. Так что пусть их лучше ищет программа, а не человек.
Если у вас ещё остались вопросы или возражения, приглашаю познакомиться со статьёй "Причины внедрить в процесс разработки статический анализатор кода PVS-Studio". С вероятность 90 % вы найдете в ней ответ на ваши вопросы :). В оставшихся 10 % случаев напишите нам, пообщаемся :).
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Date Processing Attracts Bugs or 77 Defects in Qt 6.