Главная
>
Публикации
>
C++
>
PVS-Studio vs Chromium

Андрей Карпов
Статей: 671

PVS-Studio vs Chromium

В этот раз победу одержало добро. А вернее, исходные коды проекта Chromium. Chromium - один из лучших проектов, который мы проверяли с помощью PVS-Studio.

Chromium - веб-браузер с открытым исходным кодом, разработанный компанией Google и предназначенный для предоставления пользователям быстрого и безопасного доступа в Интернет. На основе Chromium создаётся браузер Google Chrome. При этом Chromium является предварительной версией Google Chrome, а также ряда других альтернативных веб-обозревателей.

С точки зрения программиста Chromium - это решение (solution), состоящее из 473 проектов. Общий объем исходного кода на языке C/C++ составляет около 460 мегабайт. Количество строк посчитать затрудняюсь.

В эти 460 мегабайт входит большое количество различных библиотек. Если их отделить, то останется около 155 мегабайт. Существенно меньше, но всё равно много. Тем более всё относительно. Многие из этих библиотек сделаны разработчиками Chromium в рамках задачи создания Chromium. Хотя такие библиотеки живут сами по себе, их можно вполне отнести к самому браузеру.

Chromium стал самым большим и самым качественным проектом, с которым я познакомился в ходе испытаний PVS-Studio. При работе с проектом Chromium было на самом деле не очень понятно, кто кого проверяет. Мы нашли и исправили в PVS-Studio несколько ошибок, связанных с анализом Си++ файлов и с поддержкой специфичной структуры проекта.

О качестве исходного кода Chromium говорит множество моментов и используемых приемов. Например, большинство программистов для определения количества элементов в массиве используют следующую конструкцию:

int XX[] = { 1, 2, 3, 4 };
size_t N = sizeof(XX) / sizeof(XX[0]);

Обычно это оформляют в макрос следующего вида:

#define count_of(arg) (sizeof(arg) / sizeof(arg[0]))

Это вполне работоспособный и полезный макрос. Я и сам, если честно, всегда именно таким макросом и пользовался. Однако он может послужить причиной ошибки, так как ему случайно можно подсунуть простой указатель, и он не будет возражать по этому поводу. Поясню на примере:

void Test(int C[3])
{
  int A[3];
  int *B = Foo();
  size_t x = count_of(A); // Ok
  x = count_of(B); // Error
  x = count_of(C); // Error
}

Конструкция count_of(A) работает корректно и возвращает количество элементов в массиве A, равное трём.

Но если случайно применить count_of() к указателю, то результатом будет малоосмысленное значение. Беда в том, что макрос никак не предупредит программиста о странной конструкции вида count_of(B). Происходит деление размера указателя на размер элемента массива. Подобная ситуация кажется надуманной и искусственной, но я встречал её в реальных приложениях. В качестве примера приведу код из проекта Miranda IM:

#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))
int Cache_GetLineText(...., LPTSTR text, int text_size, ....)
{
  ....
  tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0);
  ....
}

Так что подобные ошибки вполне имеют место быть и хорошо бы иметь возможность защититься от них. Ещё проще ошибиться, если попробовать вычислить размер массива, переданного в качестве аргумента:

void Test(int C[3])
{
  x = count_of(C); // Error
}

Согласно стандарту языка Си++, переменная 'C' представляет собой обыкновенный указатель, а вовсе не массив. В результате в программах нередко можно встретить обработку только части переданного массива.

Раз уж зашла речь о таких ошибках, подскажу приём, как же можно узнать размер переданного массива. Для этого необходимо передать его по ссылке:

void Test(int (&C)[3])
{
  x = count_of(C); // Ok
}

Теперь результатом выражения count_of(C) будет значение 3.

Вернемся к Chromium. В нем используется такой макрос, который позволяет избежать описанных выше ошибок. Вот его реализация:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))

Идея этого магического заклинания в следующем. Шаблонная функция ArraySizeHelper принимает на вход массив произвольного типа длиной N. При этом функция возвращает ссылку на массив длиной N, состоящий из элементов типа 'char'. Реализация функции отсутствует, так как она не нужна. Для оператора sizeof() достаточно только объявления функции ArraySizeHelper. В макросе 'arraysize' вычисляется размер возвращаемого функцией ArraySizeHelper массива байт. Этот размер и есть количество элементов в массиве, длину которого мы хотим вычислить.

Если у вас опухла голова, то можете поверить мне на слово, что это работает. Причем работает гораздо лучше, чем рассмотренный ранее макрос 'count_of()'. Так как функция ArraySizeHelper принимает массив по ссылке, то в неё невозможно передать простой указатель. Напишем тестовый код:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))

void Test(int C[3])
{
  int A[3];
  int *B = Foo();
  size_t x = arraysize(A); // Ok
  x = arraysize(B); // Ошибка компиляции
  x = arraysize(C); // Ошибка компиляции
}

Ошибочный код просто не будет скомпилирован. По-моему это великолепно, если можно предупредить потенциальную ошибку ещё на этапе компиляции. Это замечательный пример, отражающий качество подхода к программированию. Моё уважение к разработчикам в Google.

Приведу еще один пример, совершенно другого плана, но также говорящий о качестве кода.

if (!file_util::Delete(db_name, false) &&
    !file_util::Delete(db_name, false)) {
  // Try to delete twice. If we can't, fail.
  LOG(ERROR) << "unable to delete old TopSites file";
  return false;
}

Этот код может показаться многим программистам странным. Какой смысл два раза пробовать удалить файл? А смысл есть. Тот, кто его писал, достиг просветления и суть бытия программ. Файл однозначно удаляется или не удаляется только в учебниках и абстрактном мире. В реальной системе бывает так, что файл только что нельзя было удалить и мгновением позже - можно. Причиной тому могут быть антивирусы, вирусы, системы контроля версий, и бог весть что ещё. Программисты часто не задумываются о подобных ситуациях. Они мыслят так, раз не удалость удалить файл, то значит и не получится. Но, если хочется сделать хорошо и не мусорить в каталогах, нужно учитывать эти посторонние воздействия. Я сталкивался с ровно такой же ситуацией, когда файл не удаляется один раз на 1000 запусков. И решение было ровно таким же. Ну, разве что я ещё на всякий случай Sleep(0) вставил посередине.

А что же проверка с помощью PVS-Studio? Код Chromium, пожалуй, самый качественный код, который я видел. Это подтверждается очень низкой плотностью ошибок, которые мы смогли найти. Если брать количественно, то ошибок конечно много. А вот если поделить это количество ошибок на объем кода, то, получается, что их практически нет. Какие это ошибки? Самые обыкновенные. Несколько примеров:

V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. platform time_win.cc 116

void NaCl::Time::Explode(bool is_local, Exploded* exploded) const {
  ....
  ZeroMemory(exploded, sizeof(exploded));
  ....
}

Опечатки делают все. Здесь просто забыли звёздочку. Должно было быть: sizeof(*exploded).

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400

static const int kClientEdgeThickness;
int height() const;
bool ShouldShowClientEdge() const;

void CustomFrameView::PaintMaximizedFrameBorder(gfx::Canvas* canvas) {
  ....
  int edge_height = titlebar_bottom->height() -
                    ShouldShowClientEdge() ? kClientEdgeThickness : 0;
  ....
}

Коварный оператор "?:" имеет более низкий приоритет, чем вычитание. Здесь нужны дополнительные скобочки:

int edge_height = titlebar_bottom->height() -
                  (ShouldShowClientEdge() ? kClientEdgeThickness : 0);

Проверка, которая не имеет смысла.

V547 Expression 'count < 0' is always false. Unsigned type value is never < 0. ncdecode_tablegen ncdecode_tablegen.c 197

static void CharAdvance(char** buffer, size_t* buffer_size, size_t count) {
  if (count < 0) {
    NaClFatal("Unable to advance buffer by count!");
  } else {
  ....
}

Условие "count < 0" всегда ложно. Защита не сработает и потенциально какой-то буфер может быть переполнен. Это, кстати, пример, как статические анализаторы могут быть использованы для поиска уязвимостей. Злоумышленник может быстро выделить для себя те участки кода, которые содержат ошибки для их дальнейшего, более внимательного анализа. А вот еще один фрагмент кода, который может быть интересен с точки зрения безопасности:

V511 The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (salt)' expression. common visitedlink_common.cc 84

void MD5Update(MD5Context* context, const void* buf, size_t len);

VisitedLinkCommon::Fingerprint 
  VisitedLinkCommon::ComputeURLFingerprint(
  ....
 const uint8 salt[LINK_SALT_LENGTH])
{
  ....
  MD5Update(&ctx, salt, sizeof(salt));
  ....
}

Функция MD5Update() обработает столько байт, сколько занимает указатель. Не потенциальная ли дыра в плане шифрования данных? Я не знаю есть ли во всём этом какая-то опасность или нет. Но с точки зрения злоумышленников - это однозначно интересное место для более подробного изучения.

Корректный код должен выглядеть так:

MD5Update(&ctx, salt, sizeof(salt[0]) * LINK_SALT_LENGTH);

Или вот так:

VisitedLinkCommon::Fingerprint 
  VisitedLinkCommon::ComputeURLFingerprint(
  ....
 const uint8 (&salt)[LINK_SALT_LENGTH])
{
  ....
  MD5Update(&ctx, salt, sizeof(salt));
  ....
}

Еще пример про опечатку:

V501 There are identical sub-expressions 'host != buzz::XmlConstants::str_empty ()' to the left and to the right of the '&&' operator. chromoting_jingle_glue iq_request.cc 248

void JingleInfoRequest::OnResponse(const buzz::XmlElement* stanza) {
  ....
  std::string host = server->Attr(buzz::QN_JINGLE_INFO_HOST);
  std::string port_str = server->Attr(buzz::QN_JINGLE_INFO_UDP);
  if (host != buzz::STR_EMPTY && host != buzz::STR_EMPTY) {
  ....
}

На самом деле нужно проверить и переменную port_str:

if (host != buzz::STR_EMPTY && port_str != buzz::STR_EMPTY) {

Немного из классики:

V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293

bool NpProxyService::GetProxyValueJSONString(std::string* output) {
  DCHECK(output);
  output->empty();
  ....
}

Должно быть: output->clear();

А вот даже есть и работа с нулевым указателем:

V522 Dereferencing of the null pointer 'plugin_instance' might take place. Check the logical condition. chrome_frame_npapi chrome_frame_npapi.cc 517

bool ChromeFrameNPAPI::Invoke(...)
{
  ChromeFrameNPAPI* plugin_instance =
    ChromeFrameInstanceFromNPObject(header);
  if (!plugin_instance && (plugin_instance->automation_client_.get()))
    return false;
  ....
}

Ещё один пример проверки, которая никогда не сработает:

V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23

IdleState CalculateIdleState(unsigned int idle_threshold) {
  ....
  DWORD current_idle_time = 0;
  ....
  // Will go -ve if we have been idle for a long time (2gb seconds).
  if (current_idle_time < 0)
    current_idle_time = INT_MAX;
  ....
}

Пожалуй, следует остановиться. Я могу продолжать дальше, но это становится скучно. И это ведь только то, что относится к самому Chromium. А ведь есть ещё тесты с ошибками наподобие этой:

V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. interactive_ui_tests accessibility_win_browsertest.cc 306

void AccessibleChecker::CheckAccessibleChildren(IAccessible* parent) {
  ....
  auto_ptr<VARIANT> child_array(new VARIANT[child_count]);
  ....
}

И огромное количество библиотек, на которых и строится Chromium. Причем размер библиотек значительно больше, чем размер самого Chromium. А там тоже множество интересных мест в коде. Понятно, что возможно код с ошибками нигде не используется, но ошибка от этого не перестает быть ошибкой. Один первый попавшийся пример (библиотка ICU):

V547 Expression '* string != 0 || * string != '_'' is always true. Probably the '&&' operator should be used here. icui18n ucol_sit.cpp 242

U_CDECL_BEGIN static const char* U_CALLCONV
_processVariableTop(...)
{
  ....
  if(i == locElementCapacity && (*string != 0 || *string != '_')) {
    *status = U_BUFFER_OVERFLOW_ERROR;
  }
  ....
}

Выражение (*string != 0 || *string != '_') всегда истинно. Видимо должно быть: (*string == 0 || *string == '_').

Вывод

PVS-Studio потерпел поражение. Код Chromium - один из лучших кодов, которые мы анализировали. Мы практически ничего не нашли в Chromium. Вернее, мы нашли очень даже много ошибок, и в этой статье мы показали только небольшую их часть. Но если учесть, что все эти ошибки размазаны по исходному коду объемом 460 мегабайт, то, получается, что их практически нет.

P.S.

Отвечаю на вопрос: сообщим ли мы разработчикам Chromium о найденных ошибках? Нет, не сообщим. Это очень большой объем работы, который мы не можем позволить себе выполнять бесплатно. Проверка Chromium это совсем не проверка Miranda IM или проверка Ultimate Toolbox. Это большая работа, необходимо внимательно изучить все сообщения и разобраться действительно ли это ошибка или нет. Для этого необходимо ориентироваться в проекте. Мы перешлём перевод этой статьи разработчикам Chromium, и если им будет интересно, они смогут сами выполнить анализ проекта и проанализировать все диагностические сообщения. Да, для этого им будет необходимо приобрести PVS-Studio. Но любой отдел Google легко может себе это позволить.

P.P.S.

Нет, мы не жадные. Мы готовы помогать открытым проектам, наподобие FlylinkDC++. Но это разные вещи.