Когда мы пишем статьи про проверки каких-либо проектов с помощью PVS-Studio, то, как правило, у нас прибавляется клиентов. Тут все честно. Программисты не любят рекламу, но охотно отзываются на интересные материалы, которые легко проверить. Поэтому мы не рекламируем свой инструмент, а просто показываем, что он умеет. Однако, хотя мы проверили код Chromium уже три раза и трижды находили в нем ошибки, ордера с почтой в google.com в моей почте до сих пор нет. Поскольку мне интересно, что я делаю не так, и почему Google пока не использует PVS-Studio, я решил написать очередную статью.
PVS-Studio интегрируется в Ninja для проверки Chomium.
Эта статья состоит из двух частей. В первой рассказывается об инфраструктуре проекта Chromium и нюансах интеграции, во второй приведены очередные найденные ошибки.
Хотите узнать, почему разрабатывать Chromium сложно и далеко не каждый инструмент для программистов может быть использован в проекте Chromium? Тогда читаем...
Разработка проектов типа Chromium чрезвычайно сложна. На самом деле, мне даже немного неловко писать "проектов типа Chromium", так как других проектов подобного уровня я не знаю. Нет, понятно, что есть ядро Linux, есть среда Visual Studio и много еще больших и крупных проектов. Но лично мне довелось пока "постоять рядом" только с Chromium. И то, что я видел – очень интересно любому программисту, так как там есть чему поучиться.
Например, при разработке Chromium не очень-то активно, оказывается, используют Visual Studio. Причина проста – Chromium содержит огромное количество проектов, и IDE от Microsoft откровенно говоря "не тянет" такое количество. "Ага!" - сказали суровые линуксоиды... "Еще бы!!!" Но при разработке Chromium не используют и линуксовые Makefile. Ровно по той же самой причине – стандартный GNU make "не тянет" такое количество проектов и работает слишком долго.
Что же используют разработчики Chromium? Это сборочная система GYP (Generate Your Projects). С ее помощью можно генерировать либо файлы .vcxproj (MSBuild/Visual C++), либо файлы системы Ninja (это такой сильно упрощенный и быстрый makefile). Поэтому для того, чтобы регулярно использовать какой-нибудь статический анализ, надо интегрировать его именно в эту сборочную систему. Чем мы и занялись для того, чтобы продать PVS-Studio в Google.
Проект Chromium примечателен тем, что размер его исходного кода на языках C/C++, учитывая сторонние библиотеки (англ. third party library), превышает 500МБ, а каждое изменение кода проверяется десятками тысяч автоматических тестов параллельно на сотнях тестовых компьютеров различных архитектур и конфигураций. Можно отметить и высокую скорость разработки в данном проекте: в 2012-м году в репозиторий Chromium было создано более 48 тысяч ревизий более чем 900 уникальными авторами, что соответствует в среднем одной ревизии каждые 11 минут и одной ревизии в неделю от каждого активного участника проекта.
Проект такого размера и скорости разработки накладывает особо жесткие требования к универсальности, точности и эффективности детекторов ошибок, а также всей системе тестирования в целом. Многие ошибки, неточности и неэффективности детекторов были впервые обнаружены именно при тестировании Chromium. В частности, оказалось неоправданно сложным применение коммерческих детекторов без открытого исходного кода — зачастую они некорректно обрабатывали даже базовые примитивы проекта, при этом исправление этих недочетов требовало длительного ожидания выхода новой версии детектора.
Хотя PVS-Studio и не является open source проектом, в гибкости нам не откажешь. Поэтому мы захотели показать на примере Chromium, что можем встроиться в его сборочную систему и проверять такой большой проект без проблем.
В дистрибутиве PVS-Studio можно выделить 2 главных компонента: command-line анализатор PVS-Studio.exe и IDE плагин, интегрирующий этот command-line анализатор в одну из поддерживаемых сред разработки (Microsoft Visual Studio и Embarcadero RAD Studio).
Command-line анализатор по принципу своей работы очень схож с компилятором — он вызывается для каждого из проверяемых файлов с параметрами, включающими в себя, в том числе, и оригинальные параметры компиляции этого файла. Затем анализатор вызывает необходимый ему внешний препроцессор (в соответствии с компилятором, используемым при сборке проверяемого файла) и производит непосредственный анализ полученного препроцессированного временного файла (i-файла), т.е. файла, в котором раскрыты все include и define директивы.
Но использование анализатора PVS-Studio.exe не ограничивается IDE плагинами. Как уже было сказано выше, command-line анализатор, очень близок в своём использовании непосредственно к компилятору. Соответственно, его вызов можно встроить, при необходимости, и напрямую в сборочную систему, наравне с самим компилятором. Например, если ваш проект собирается в IDE Eclipse с помощью gcc, вы можете встроить проверку PVS-Studio в ваши makefile'ы.
Для прямой интеграции статического анализа в сборочный процесс необходимо встроить вызов PVS-Studio.exe в сборочный сценарий рядом с непосредственным вызовом самого компилятора C/C++, и передать анализатору те же параметры, что передаются компилятору (а также несколько дополнительных параметров, контролирующих вывод результатов анализа). Это требование обусловлено тем, что статический анализатор необходимо будет запустить для каждого проверяемого файла, с соответствующими этому файлу специфичными параметрами. А это удобнее всего сделать как раз в том месте, где и происходит автоматический обход всех исходных файлов проекта.
Как написано выше, Chromium разрабатывается с помощью сборочной системы GYP (Generate Your Projects), позволяющей получить нативные проектные файлы для различных ОС и компиляторов. Т.к. на данный момент анализатор PVS-Studio поддерживает только ОС Windows, рассмотрим возможные варианты сборки Chromium с помощью компилятора Visual C++ 10. Данный компилятор (cl.exe) поставляется в составе интегрированной среды разработки Visual Studio, а также может быть установлен отдельно из бесплатного пакета Windows SDK.
Используемая в Chromium система GYP позволяет получить проектные файлы MSBuild (vcxproj) для сборки Chromium с помощью компилятора Visual C++ (cl.exe). Сборочная система MSBuild является частью пакета .NET Framework, являющегося одним из стандартных компонентов ОС семейства Windows.
Наиболее простым способом проверки проекта Chromium анализатором PVS-Studio будет использование "родного" для него IDE плагина среды Visual Studio. Проектные файлы MSBuild можно открыть для проверки в среде разработки Visual Studio. При этом IDE плагин PVS-Studio автоматически соберёт всю необходимую информацию о каждом из файлов проекта и запустит для них статический анализатор PVS-Studio.exe. Заметим, что бесплатная версия среды разработки Visual Studio Express Edition не поддерживает работу с IDE плагинами.
Сборку и проверку проектных файлов vcxproj можно также произвести напрямую, без использования среды Visual Studio, с помощью сборочной системы MSBuild (а точнее, используя command-line утилиту MSBuild.exe). Для проверки проектов статическим анализатором в таком режиме необходимо будет напрямую интегрировать в каждый проектный файл вызов command-line анализатора PVS-Studio.exe (можно также и импортировать во все проектные файлы общий props файл, содержащий подобный вызов анализатора).
Стоит заметить, что хотя MSBuild и позволяет напрямую из своих сборочных скриптов (которыми, в том числе, и являются проектные файлы vcxproj) вызывать исполняемые файлы, вызов таких сборочных инструментов, как компилятор и компоновщик в стандартных проектах осуществляется с помощью специальных подключаемых модулей-обёрток (называемых в терминологии MSBuild сборочными задачами – Build Tasks). Дистрибутив PVS-Studio содержит подобный модуль сборочной задачи для сценариев MSBuild, а также использующий его Props (property sheet) файл, который можно напрямую импортировать в стандартные проекты vcxproj для интеграции в них статического анализа.
Сборка Chromium под Windows с помощью компилятора cl.exe возможна и с помощью скриптов сборочной системы Ninja, которые также можно получить генератором проектов GYP.
Как было описано выше, для прямой интеграции статического анализа PVS-Studio в сборочный процесс необходимо встроить вызов PVS-Studio.exe в том месте, где происходит обход исходных файлов сборочной системой при их компиляции.
В случае с файлами системы Ninja подобный способ интеграции оказывается затруднённым тем, что собираемые файлы жёстко прописываются в авто-сгенерированных файлах *.ninja как зависимости для obj файлов. В связи с этим, для интеграции анализа на данном шаге сборки необходимо будет модифицировать соответствующие этому шагу правила сборки (описанные в общем файле build.ninja): это сс и cxx, т.к. именно эти правила используются при обходе всех исходных файлов.
На данный момент нам не удалось напрямую добавить в правила cc и cxx вызов PVS-Studio.exe. Правила сборки системы Ninja позволяют использовать для определения команды к выполнению только одну переменную command. При этом, в соответствии с документацией, переменная command может содержать и несколько команд, разделённых символами &&.Однако, если добавить к существующему правилу, как например:
command = ninja -t msvc -e $arch -- $cc /nologo /showIncludes /FC
@$out.rsp /c $in /Fo$out /Fd$pdbnameвызов PVS-Studio.exe:
PVS = "PVS-Studio.exe"
...
command = ninja -t msvc -e $arch -- $cc /nologo /showIncludes /FC
@$out.rsp /c $in /Fo$out /Fd$pdbname && $PVS –cfg "c:\test.cfg"то такой вызов воспринимается, как часть аргументов для процесса ninja, и, соответственно, передаётся при вызове –t msvs как аргумент в cl.exe ($cc). Аналогично, если поместить вызов $PVS в начало строки, то остальные параметры после && передаются в качестве аргументов уже в PVS-Studio.exe.
Конечно, для обхода данного ограничения можно написать программу-обёртку, которая по очереди сначала вызовет ninja, а затем PVS-Studio.exe, и прописать вызов этой обёртки в начало переменной command для интересующих нас сборочных правил (cc и cxx). Это мы и сделали для того, чтобы проверить Chromium с помощью PVS-Studio таким образом.
Главное, что необходимо помнить при использовании PVS-Studio.exe в режиме прямой интеграции со сборочной системой (т.е. без IDE плагина) - это необходимость произвести препроцессирование каждого проверяемого исходного файла перед непосредственным выполнением анализа. PVS-Studio.exe должен в качестве одного из параметров своего запуска получить флаг cl-params, после которого необходимо передать все "оригинальные" параметры компиляции данного файла. PVS-Studio.exe сам запустит внешний препроцессор (например, cl.exe), добавив к данным параметрам флаги, управляющие препроцессором (флаг /P в случае cl.exe).
Однако, из-за различий в поведении препроцессора и компилятора возможна и ситуация, в которой флагов компиляции может оказаться недостаточно для корректного препроцесирования исходного C/C++ файла. В частности, препроцессирование может оказаться невозможным, если в переданных препроцессору include путях отсутствует путь до директории, содержащей заголовочный файл, используемый как precompiled заголовок. В такой ситуации компиляция будет проходить успешно (конечно, если уже был сгенерирован указанный компилятору pch файл), но препроцессор будет завершать работу с ошибкой "cannot open include file".
IDE плагин PVS-Studio для разрешения данной проблемы в случае использования в файле precompiled заголовка сканирует все файлы проекта, содержащего проверяемый файл, и добавляет в include пути директорию файла, используемого для генерации требуемого pch (которых в проекте может быть и несколько). Для корректной же работы PVS-Studio.exe в режиме прямой интеграции необходимо обеспечить корректность работы препроцессора, также передавая этот путь в числе других параметров компиляции. Это можно сделать, добавив в перечень передаваемых анализатору аргументов компилятора ещё один –I (/I) параметр с соответствующей директорией.
Проект Chromium содержит несколько сотен подобных файлов, т.е. файлов, использующих precompiled заголовки, к которым при их компиляции в Includes не передаётся путь до папки, содержащей сами h файлы, из которых эти заголовки были получены. Для корректной проверки этих файлов анализатором PVS-Studio в режиме прямой интеграции (т.е. без использования плагина) необходимо модифицировать сборочную систему описанным выше образом перед запуском анализа.
Но можно поступить проще. Например, мы просто отключили precompiled headers при сборке Chromium для интеграции PVS-Studio в сборочную систему.
В результате такой интеграции будет получен отчет в так называемом "сыром" (raw) виде. Его можно открыть в нашей же утилите PVS-Studio Standalone (про которую я писал здесь). И начать работать в полноценной среде с навигацией и прочими удобствами.
Итак, как же выглядит интеграция PVS-Studio в сборочную систему Chromium?
А теперь переходим ко второй части статьи – примерам обнаруженных ошибок.
Целью очередной проверки Chromium был не поиск ошибок, а осваивание новой системы сборки. Точнее встраивание PVS-Studio в нее. Поэтому Андрей Карпов просмотрел диагностические сообщения очень и очень поверхностно. Тем не менее, он всё равно кое-что углядел и прислал мне несколько фрагментов кода, снабдив их комментариями. То, что даже быстрый просмотр позволяет найти ошибки не удивительно для проектов такого размера как Chromium. Как бы они не были хорошо написаны, в них всегда есть ошибки. Тем более проект Chromium быстро развивается и обрастает новыми функциями и библиотеками.
Ошибки в большинстве своём относятся к сторонним библиотекам. Впрочем, от этого они не перестают быть ошибками. Поскольку, авторы Chromium очень активно развивают библиотеки, из которых строится проект, мы думаем, им будет интересно посмотреть, что смог найти в них анализатор PVS-Studio.
Да, хочется напомнить, что это далеко не первая проверка Chromium:
Именно поэтому, сложно убедить Андрея уделить просмотру ошибок больше времени. Во-первых, уже не так интересно. Во-вторых, полноценно проанализировать отчёт могут только разработчики Chromium. Делать анализ в одиночку, тем более, не будучи знакомым с проектом и библиотеками, неподъемный труд. Да и вообще, проект можно (и нужно) проверять каждый день, а не раз в полгода. Но это уже на совести сообщества, которое работает над развитием проекта Chromium и отдельных библиотек.
static SECStatus
ssl3_SendEncryptedExtensions(sslSocket *ss)
{
static const unsigned char P256_SPKI_PREFIX[] = {
0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a, 0x86,
0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a,
0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07, 0x03,
0x42, 0x00, 0x04
};
....
if (.... ||
memcmp(spki->data, P256_SPKI_PREFIX,
sizeof(P256_SPKI_PREFIX) != 0))
{
PORT_SetError(SSL_ERROR_INVALID_CHANNEL_ID_KEY);
rv = SECFailure;
goto loser;
}
....
}Предупреждение PVS-Studio (библиотека Network Security Services): V526 The 'memcmp' function returns 0 if corresponding buffers are equal. Consider examining the condition for mistakes. ssl3con.c 10533
Из-за неправильно поставленной скобки функция memcmp() сравнивает один байт.
Выражение "sizeof(P256_SPKI_PREFIX) != 0" всегда истинно. То-есть значение этого выражения равно 1.
Правильная проверка должна быть такой:
if (.... ||
memcmp(spki->data, P256_SPKI_PREFIX,
sizeof(P256_SPKI_PREFIX)) != 0)void SkCanvasStack::pushCanvas(....) {
....
for (int i = fList.count() - 1; i > 0; --i) {
SkIRect localBounds = canvasBounds;
localBounds.offset(origin - fCanvasData[i-1].origin);
fCanvasData[i-1].requiredClip.op(localBounds,
SkRegion::kDifference_Op);
fList[i-i]->clipRegion(fCanvasData[i-1].requiredClip);
}
....
}Слабо заметить подозрительное? :) Анализатору нет.
Предупреждение PVS-Studio (библиотека Skia Graphics Engine): V501 There are identical sub-expressions to the left and to the right of the '-' operator: i - i SkCanvasStack.cpp 38
В качестве индекса несколько раз используется выражение [i - 1]. А в одном месте, написано [i-i]. Это очень похоже на опечатку. Скорее всего, здесь тоже должна вычитаться единица.
Code* Code::FindFirstCode() {
ASSERT(is_inline_cache_stub());
DisallowHeapAllocation no_allocation;
int mask = RelocInfo::ModeMask(RelocInfo::CODE_TARGET);
for (RelocIterator it(this, mask); !it.done(); it.next())
{
RelocInfo* info = it.rinfo();
return
Code::GetCodeFromTargetAddress(info->target_address());
}
return NULL;
}Предупреждение PVS-Studio (Chromium): V612 An unconditional 'return' within a loop. objects.cc 10326
Цикл будет завершен сразу после первой итерации. Скорее всего, здесь забыли условие. Возможно, код корректен. Но всё равно он весьма подозрителен и на него стоит обратить внимание.
А вот ещё похожий цикл:
int SymbolTable::Symbolize() {
....
if (socketpair(AF_UNIX, SOCK_STREAM,
0, child_fds[i]) == -1)
{
for (int j = 0; j < i; j++) {
close(child_fds[j][0]);
close(child_fds[j][1]);
PrintError("Cannot create a socket pair");
return 0;
}
}
....
}Предупреждение PVS-Studio (библиотека tcmalloc): V612 An unconditional 'return' within a loop. symbolize.cc 154
Кажется, здесь фигурная скобка закрывается не там, где надо. По всей видимости, код должен был выглядеть так:
if (socketpair(AF_UNIX, SOCK_STREAM,
0, child_fds[i]) == -1)
{
for (int j = 0; j < i; j++) {
close(child_fds[j][0]);
close(child_fds[j][1]);
}
PrintError("Cannot create a socket pair");
return 0;
}class CONTENT_EXPORT EventPacket {
....
InputEvents::const_iterator begin() const
{ return events_.end(); }
InputEvents::const_iterator end() const
{ return events_.end(); }
....
protected:
InputEvents events_;
....
};Предупреждение PVS-Studio (Chromium): V524 It is odd that the body of 'end' function is fully equivalent to the body of 'begin' function. event_packet.h 36
Функции beign() и end() возвращают одно и тоже. Наверное, функция begin() должна выглядеть так:
InputEvents::const_iterator begin() const
{ return events_.begin(); }__inline__ unsigned long long int rdtsc()
{
#ifdef __x86_64__
unsigned int a, d;
__asm__ volatile ("rdtsc" : "=a" (a), "=d" (d));
return (unsigned long)a | ((unsigned long)d << 32);
#elif defined(__i386__)
unsigned long long int x;
__asm__ volatile ("rdtsc" : "=A" (x));
return x;
#else
#define NO_CYCLE_COUNTER
return 0;
#endif
}Предупреждение PVS-Studio (библиотека SMHasher): V629 Consider inspecting the '(unsigned long) d << 32' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. Platform.h 78
Эта функция частично работает. Однако, она может подвести, если тип long окажется 32-битным. Вот здесь "(unsigned long)d << 32" произойдёт переполнение. Чтобы этого избежать, код надо модифицировать следующим образом:
return (unsigned long long)a |
((unsigned long long)d << 32);Ключевое слово 'break' постоянно забывают при использовании оператора выбора. Забыть его может кто угодно и когда угодно. Не теряйте бдительность.
Первый пример:
static v8::Handle<v8::Value>
toV8Object(....)
{
switch (extension->name()) {
....
case WebGLExtension::WebGLCompressedTextureATCName:
extensionObject = toV8(....);
referenceName = "webGLCompressedTextureATCName";
case WebGLExtension::WebGLCompressedTexturePVRTCName:
extensionObject = toV8(....);
referenceName = "webGLCompressedTexturePVRTCName";
break;
}
....
}Предупреждения PVS-Studio (библиотека WebKit):
Обсуждать здесь нечего. Забыли 'break'. Точка.
Второй пример:
bool ScriptDebugServer::executeSkipPauseRequest(....)
{
const char* v8MethodName;
switch (request)
{
case ScriptDebugListener::NoSkip:
return false;
case ScriptDebugListener::Continue:
return true;
case ScriptDebugListener::StepInto:
v8MethodName = stepIntoV8MethodName;
case ScriptDebugListener::StepOut:
v8MethodName = stepOutV8MethodName;
}
....
}Предупреждение PVS-Studio (библиотека WebKit): V519 The 'v8MethodName' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 412, 414. ScriptDebugServer.cpp 414
Андрей Карпов прислал ещё несколько фрагментов кода. Но они не очень интересные, так что я их пропущу.
Неинтересным я считаю вот такие ляпы:
int linux_get_device_address (....,
uint8_t *busnum, uint8_t *devaddr,
....)
{
....
*busnum = __read_sysfs_attr(ctx, sys_name, "busnum");
if (0 > *busnum)
return *busnum;
....
}Предупреждение PVS-Studio (библиотека LibUSB): V547 Expression '0 > * busnum' is always false. Unsigned type value is never < 0. linux_usbfs.c 620
Указатель 'busnum' ссылается на беззнаковую переменную, имеющую тип uint8_t. Это значит, что условие (0 > *busnum) никогда не выполнится.
То есть самая настоящая ошибка. Но скучная. Так что я заканчиваю с описанием ошибок.
Вы видите, что PVS-Studio регулярно находит ошибки в коде Chromium. Вы видите, что теперь PVS-Studio легко встроить в сборочную систему. Мы готовы всячески вам в этом содействовать, если что-то не будет получаться. Дело за вами – давайте вместе будем повышать качество Chromium. Внедряйте PVS-Studio в своем проекте!
P.S. При написании данной статьи ни одно NDA не было нарушено, вся информация получена из открытых источников.