Проверять проекты интересно, проверять известные проекты интересно вдвойне, особенно если пользуешься ими сам. Ещё интереснее было бы проанализировать проект с высоким качеством кода. Тогда удалось бы одним выстрелом убить двух зайцев - проверить сам проект, подтвердив или опровергнув качество его кода, так и посмотреть, насколько хорошо справился анализатор. Немного поразмыслив, я пришёл к выводу, что для этого отлично подойдёт популярный мессенджер Telegram.
Telegram - бесплатный мессенджер, ориентированный на международный рынок, позволяющий обмениваться как текстовыми сообщениями, так и медиафайлами различных форматов. Существуют версии под Android, iOS, Windows Phone, OS X, Windows, Linux.
Авторами проекта являются Павел и Николай Дуровы, знакомые популярной социальной сетью "В контакте". Особенный упор в приложении сделан на безопасность коммуникации и повышенную защиту (в следствие чего есть возможность создания секретных самоудаляющихся чатов и прочего). Для шифрования переписки используется технология MTProto, разработанная Николаем Дуровым.
Для анализа было выбрано десктопное Windows-приложение, исходный код которого можно найти в соответствующем репозитории на GitHub.
Стоит отметить, что приложение использует достаточно много сторонних библиотек, так что если решите самостоятельно собрать приложение - придётся повозиться. С другой стороны, разработчики снабдили нас отличной документацией по сборке и установке третьестороннего ПО, так что проблем возникнуть не должно.
Возможно, у вас остались вопросы по названию статьи. "Как так?" - спросите Вы. Понятно, как при помощи анализатора проверить исходный код проекта, но что означает обратная проверка?
Как я писал выше, от кода заранее можно было ожидать высокого качества. Не слукавлю, если скажу, что проектом занимаются профессионалы своего дела, ставящие к тому же приоритетом безопасность приложения, и странно было бы найти в нём много ошибок. К тому же периодически проводятся конкурсы на поиск уязвимостей, что также держит планку кода на уровне. Так что проверка проекта была бы неплохим способом посмотреть, как справится со своей задачей анализатор. Но об этом ниже.
Для проверки проекта использовался статический анализатор кода PVS-Studio. Рассматривались предупреждения общего назначения (GA) и оптимизации (OP) первого и второго уровня важности.
В принципе, можно заранее сделать оценку качества кода, так как всем нам хорошо известно качество работы сети "В контакте" в то время, когда Павел ещё занимал руководящую должность. Скажу сразу, что здесь всё так же хорошо. Ошибок нашлось не так много, что обуславливается 2 факторами:
Так что можно утверждать, что ребята трудятся на славу. Тем не менее, с помощью анализатора удалось найти несколько достаточно интересных мест, которые мы и рассмотрим чуть ниже.
Для статьи выбирались не все предупреждения анализатора, а некоторые наиболее интересные.
По некоторым местам нельзя дать однозначного ответа, является фрагмент кода ошибочным или нет, как его исправлять, так как для этого необходимо куда более детальное изучение исходного кода. Здесь стоит отметить, что это ещё раз подчёркивает необходимость использования статических анализаторов непосредственно разработчиками, пишущими код.
Хотелось бы также отметить саму процедуру проверки исходного кода анализатором. Так как имеется .sln-файл, запуск проверки достаточно прост. После сборки и установки всех сторонних библиотек достаточно убедиться, что само "решение" собирается без ошибок, после чего в несколько кликов мыши можно запустить анализ проекта. По его окончании останется только просмотреть полученный отчёт о найденных ошибках.
Примечание. С момента проверки исходного кода командой разработчиков было выпущено несколько обновлений, так что, возможно, некоторые фрагменты кода могут отличаться от приведённых в статье.
Давайте взглянем на следующий участок кода. Будучи выписанным отдельно, данный фрагмент кода не предоставляет проблем для обнаружения в нём ошибки:
void Window::placeSmallCounter(.... int size, int count, ....)
{
....
QString cnt = (count < 100) ? QString("%1").arg(count) :
QString("..%1").arg(count % 10, 1, 10, QChar('0'));
int32 cntSize = cnt.size();
....
int32 fontSize;
if (size == 16) {
fontSize = 8;
} else if (size == 32) {
fontSize = (cntSize < 2) ? 12 : 12;
} else {
fontSize = (cntSize < 2) ? 22 : 22;
}
....
}Предупреждение анализатора: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 12. Telegram window.cpp 1607
Найти ошибку (а точнее - 2) сейчас, когда код, её содержащий, выписан отдельно, не составит труда. При использовании тернарного оператора вне зависимости от логического результата в условии переменной 'fontSize' будет присвоено одно и то же значение. Вероятно, что вместо повторяющихся чисел '12' и '22' в каждом из тернарных операторов, как в данном примере, в каждом из них должно были быть числа '12' и '22' без повторов.
Согласитесь, что ошибка явно бросается в глаза. Как же её можно было допустить, спросите вы? Все мы люди и делаем ошибки, и если в данном случае найти её легко, то в 1700+ строках из этого файла эта ошибка без проблем затеряется на фоне остального кода.
Нередко в проектах встречается ошибка, когда указатель разыменовывается, а лишь затем проверяется на равенство nullptr. Telegram не стал исключением:
void DialogsWidget::dialogsReceived(....)
{
const QVector<MTPDialog> *dlgList = 0;
....
unreadCountsReceived(*dlgList);
....
if (dlgList)
....
}Предупреждение анализатора: V595 The 'dlgList' pointer was utilized before it was verified against nullptr. Check lines: 1620, 1626. Telegram dialogswidget.cpp 1620
Уже из данного фрагмента кода видно, что проверка указателя 'dlgList' применяется только после разыменовывания. Разыменовавание нулевого указателя является неопределённым поведением, а значит, ваша программа может работать нормально, аварийно завершиться, отослать все ваши пароли китайским хакерам или ещё чего хуже. Так что проверку на нулевой указатель следовало разместить до его разыменовывания.
Встретились ещё 14 подобных сообщений. В некоторых местах дела обстоят лучше и ошибки нет. Проверки просто повторяются (проверка->разыменовывание->проверка, при этом указатель не меняется), но не будем зацикливаться на этом, а пойдём дальше.
Следующий подозрительный фрагмент кода:
bool psShowOpenWithMenu(....)
{
....
IEnumAssocHandlers *assocHandlers = 0;
....
if (....)
{
....
IEnumAssocHandlers *assocHandlers = 0;
....
}
....
}Предупреждение анализатора: V561 It's probably better to assign value to 'assocHandlers' variable than to declare it anew. Previous declaration: pspecific_wnd.cpp, line 2031. Telegram pspecific_wnd.cpp 2107
Опять же, когда код выписан, и все лишнее из него убрано, легко увидеть переопределение переменной. В методе, длина которого не позволяет просмотреть его целиком на мониторе, это сделать не так просто.
Определяется переменная 'assocHandlers', после чего с ней выполняются какие-то операции, но ниже определяется ещё одна переменная с таким же типом и именем (причём точно таким же образом), и эта переменная уже никак не используется. Возможно вы возразите, что здесь никакой ошибки нет. Это пока что, грабли уже разложены и ждут момента, чтобы на них наступили. В будущем человек, который будет работать с кодом, может не заметить этого переопределения, и тогда уже ошибка проявит себя. Но, как не раз упоминалось, чем раньше устранена ошибка - тем лучше, так что нужно избегать таких мест.
Встретился ещё один схожий фрагмент кода. Соответствующее диагностическое сообщение:
V561 It's probably better to assign value to 'ms' variable than to declare it anew. Previous declaration: window.cpp, line 1371. Telegram window.cpp 1467
Идём дальше:
void HistoryImageLink::getState(.... const HistoryItem *parent, ....)
const
{
....
int skipx = 0, skipy = 0, height = _height;
const HistoryReply *reply = toHistoryReply(parent);
const HistoryForwarded *fwd = reply ? 0 :
toHistoryForwarded(parent);
....
if (reply) {
skipy = st::msgReplyPadding.top() + st::msgReplyBarSize.height() +
st::msgReplyPadding.bottom();
} if (fwd) {
skipy = st::msgServiceNameFont->height + st::msgPadding.top();
}
....
}Предупреждение анализатора: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. Telegram history.cpp 5181
По предупреждению анализатора понятно, что, возможно, предполагалось наличие ключевого слова 'else', а не новое условие. Тяжело сказать, как правильно исправить этот код. Возможно, его и не стоит править.
Это единственные 2 ветви, где переменная 'skipy' инициализируется каким-то значением. Из фрагмента видно, что изначально она инициализируется 0, а после (исходный код здесь не приведён, так как его много) идёт приращение этой переменной.
Отсюда можно сделать вывод что, возможно, второе условие 'if' излишне, или даже ошибочно (если оба условия будут истинными). Может быть предполагалась конструкция 'else-if' (судя по форматированию), однозначно сказать, глядя со стороны, сложно. Тем не менее это место может быть потенциально ошибочным.
Следующий подозрительный участок кода:
void DialogsListWidget::addDialog(const MTPDdialog &dialog)
{
History *history = App::history(App::peerFromMTP(dialog.vpeer),
dialog.vunread_count.v, dialog.vread_inbox_max_id.v);
....
SavedPeersByTime &saved(cRefSavedPeersByTime());
while (!saved.isEmpty() && history->lastMsg->date < saved.lastKey())
{
History *history = App::history(saved.last()->id);
....
}
....
}Предупреждение анализатора: V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. Telegram dialogswidget.cpp 949
Суть проблемы понятна из сообщения анализатора. В теле цикла объявляется переменная, совпадающая с переменной, используемой для контроля цикла. Чем может быть опасна эта ситуация? Изменение переменной в теле цикла никак не повлияет на условие выхода из него (так как изменяется другая переменная), из-за чего часть условия выхода из цикла может оказаться некорректной (например, может возникнуть бесконечный цикл).
Если это и не ошибка, то по крайней мере припрятанные в листве грабли.
Рассмотрим другое проблемное место:
bool update()
{
....
wstring fname = from[i], tofname = to[i];
....
WCHAR errMsg[2048];
....
wsprintf(errMsg, L"Failed to update Telegram :(\n%s is not
accessible.", tofname);
....
}Предупреждение анализатора: V510 The 'wsprintfW' function is not expected to receive class-type variable as third actual argument. Updater updater.cpp 255
Проблема кроется в третьем аргументе функции - объекте типа wstring. Так как список формальных параметров функции wsprintf оканчивается эллипсисом, в неё можно передавать аргументы любых типов, что уже таит в себе некую опасность. В качестве фактических аргументов эллипсиса должны выступать только POD-типы. Из форматной строки видно, что ожидается аргумента типа 'wchar_t *', но вместо этого мы передаём объект, что может привести к формированию бессмыслицы в буфере, или же к аварийному завершению программы.
Встретился фрагмента кода с лишним подвыражением в условном выражении:
QImage imageBlur(QImage img)
{
....
const int radius = 3;
....
if (radius < 16 && ....)
....
}Предупреждение анализатора: V560 A part of conditional expression is always true: radius < 16. Telegram images.cpp 241
Суть предупреждения предельно ясна - объявляется и тут же инициализируется переменная (к тому же - константа), а в условии её значение сравнивается с числовым литералом. Так как ни константа, ни числовой литерал (что естественно) не изменяются, условие всегда будет либо истинным, либо ложным (в данном случае - всегда 'true').
Встречался код, когда переменной дважды присваивалось значение, при этом между этими присваиваниями она никак не используется. Это может быть ошибкой, если подразумевалась другая переменная. В данном случае такой опасности нет (по крайней мере она незаметна), но понятно, что это ни к чему:
bool eBidiItemize(....)
{
....
dir = QChar::DirON; status.eor = QChar::DirEN;
dir = QChar::DirAN;
....
}Предупреждение анализатора: V519 The 'dir' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2084, 2085. Telegram text.cpp 2085
Странно выглядят места, где объявляется переменная, которая в последующем нигде не используется. Понятно, что нет ничего хорошего в разбросанных по коду неиспользуемых переменных. Пример подобного кода:
void extractMetaData(AVDictionary *dict)
{
....
for (....)
{
....
QString tmp = QString::fromUtf8(value);
}
}Предупреждение анализатора: V808 'tmp' object of 'QString' type was created but was not utilized. Telegram audio.cpp 2296
Как видно из фрагмента кода, объявляется переменная 'tmp', которая нигде не используется. Для её инициализации применяется вызов метода, более того - всё это происходит в теле цикла, что ещё несколько усугубляет ситуацию.
Это не единственное предупреждение подобного типа, встретилось ещё 16 подобных.
Проверка Telegram оказалась весьма интересной и расставила некоторые точки над 'i'.
Во-первых, давно хотелось проверить этот проект, и наконец-то это удалось. Несмотря на то, что перед непосредственной проверкой пришлось повозиться с установкой третьестороннего ПО, проблем это не вызвало, так как есть доходчивые инструкции по установке и сборке.
Во-вторых, код проекта оказался качественным, и это радует. Данный мессенджер ставит приоритетом конфиденциальность переписки, и было бы странно найти в нём множество ошибок.
В-третьих, PVS-Studio всё же удалось найти несколько странных мест (хочу напомнить, что в статье были выписаны не все, а наиболее интересные места), несмотря на то, что код пишут профессионалы, знающие своё дело, и проводятся конкурсы на поиск уязвимостей. Это подчёркивает качество анализатора и необходимость использования такого инструмента программистами.