Андрей Карпов

02 Фев 2018

Теги:

#Cpp

Chromium: другие ошибки

02 Фев 2018

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

Undefined и unspecified behavior
Ошибки в логике
Неправильные ожидания
Неправильные проверки указателей
Передаём привет разработчикам компании Google
Заключение

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

0559_Chromium_7_miscellaneous_ru/image1.png

Как я писал в вводной статье, просматривая отчёт, выданный анализатором PVS-Studio, я заметил около 250 ошибок в проекте Chromium и использованных в нём библиотеках. Отчёт я смотрел достаточно бегло, поэтому, на самом деле, ошибок можно найти намного больше.

После вводной статьи я написал еще 6, где рассматривал различные паттерны ошибок. Писал я эти статьи, писал, и всё равно, у меня остаётся около 70 примеров ошибок. Сделать на основании оставшихся багов статьи на разные темы я затрудняюсь. Возможно, я устал. Впрочем, есть и другая причина - меня ждёт отчёт о проверке XNU и мне не терпится им позаниматься.

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

Undefined и unspecified behavior

Проект Chromium.

void DeviceMediaAsyncFileUtil::CreateOrOpen(
    std::unique_ptr<FileSystemOperationContext> context, ....) {
  ....
  CreateSnapshotFile(
      std::move(context), url,
      base::Bind(
        &NativeMediaFileUtil::CreatedSnapshotFileForCreateOrOpen,
        base::RetainedRef(context->task_runner()),
        file_flags, callback));
}

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'context' might take place. device_media_async_file_util.cc 322

В зависимости от везения, а точнее от используемого компилятора, этот код может как работать правильно, так и приводить к разыменовыванию нулевого указателя.

Будет разыменование нулевого указателя или нет, зависит от того, в каком порядке будут вычисляться аргументы при вызове функции CreateSnapshotFile. В C++ порядок вычисления аргументов функции не определен (unspecified behavior). Если первым окажется вычислен аргумент std::move(context), то здесь context->task_runner() произойдёт разыменование нулевого указателя.

Рекомендация. Не надо стараться напихать в одну строку как можно больше действий – это часто подводит. Пишите простой код, и он с большей вероятностью получится правильным.

Проект Chromium.

std::unordered_map<std::string, int> thread_colors_;

std::string TraceLog::EventToConsoleMessage(....) {
  ....
  thread_colors_[thread_name] = (thread_colors_.size() % 6) + 1;
  ....
}

Предупреждение PVS-Studio: V708 CWE-758 Dangerous construction is used: 'm[x] = m.size()', where 'm' is of 'unordered_map' class. This may lead to undefined behavior. trace_log.cc 1343

Этот код настолько сложен, что даже непонятно, есть ли сейчас в нём неопределённое поведение или нет. Дело в том, что стандарт C++ меняется и некоторые конструкции, которые ранее приводили к неопределённому поведению, становятся корректными. Поясню это на простых примерах:

i = ++i + 2;       // undefined behavior until C++11
i = i++ + 2;       // undefined behavior until C++17
f(i = -2, i = -2); // undefined behavior until C++17
f(++i, ++i);       // undefined behavior until C++17,
                   // unspecified after C++17
i = ++i + i++;     // undefined behavior 
cout << i << i++;  // undefined behavior until C++17
a[i] = i++;        // undefined behavior until C++17
n = ++i + i;       // undefined behavior

Вернёмся к коду из проекта Chromium. Выражение thread_colors_[thread_name] может создать новый элемент в контейнере, а может и не создать, и вернуть ссылку на уже существующий элемент. Главное, что thread_colors_[thread_name] может изменять количество элементов в ассоциативном контейнере.

Выражение (thread_colors_.size() % 6) + 1 зависит от количества элементов в ассоциативном контейнере thread_colors_.

Будут получаться разные значения в зависимости от того, вычисляется первым выражение слева от оператора присваивания = или выражение справа.

От чего зависит порядок вычисления? От версии используемого языка. В любом случае так писать не надо. Этот код очень сложен для понимания.

Рекомендация. Совет всё тот же: не надо стараться напихать в одну строку как можно больше действий.

Библиотека ICU.

U_DRAFT uint32_t U_EXPORT2 ubiditransform_transform(....)
{
  ....
  const UBiDiAction *action = NULL;
  ....
  if (action + 1) {
    updateSrc(....);
  }
  ....
}

Предупреждение PVS-Studio: V694 CWE-571 The condition (action + 1) is only false if there is pointer overflow which is undefined behavior anyway. ubiditransform.cpp 502

Условие всегда истинно. Теоретически, ложным оно может стать, если произойдёт переполнение, но это приводит к undefined behavior.

Библиотека WebRTC.

std::vector<SdpVideoFormat>
StereoDecoderFactory::GetSupportedFormats() const
{
  std::vector<SdpVideoFormat> formats = ....;
  for (const auto& format : formats) {           // <=
    if (cricket::CodecNamesEq(....)) {
      ....
      formats.push_back(stereo_format);          // <=
    }
  }
  return formats;
}

Предупреждение PVS-Studio: V789 CWE-672 Iterators for the 'formats' container, used in the range-based for loop, become invalid upon the call of the 'push_back' function. stereocodecfactory.cc 89

Анализатор обнаружил инвалидацию итератора в range-based for цикле. Приведённый выше код аналогичен следующему:

for (auto format = begin(formats), __end = end(formats); 
     format != __end; ++format) {
  if (cricket::CodecNamesEq(....)) {
    ....
    formats.push_back(stereo_format);
  }
}

Теперь видно, что при вызове функции push_back может произойти инвалидация итераторов format и __end, если произойдёт реаллокация памяти внутри вектора.

Рекомендация. Помните, что в range-based for циклах нельзя, как и в циклах организованных с помощью итераторов, менять количество элементов в контейнере.

Ошибки в логике

Проект Chromium.

STDMETHOD(GetInputScopes)(InputScope** input_scopes,
                          UINT* count) override
{
  if (!count || !input_scopes)
    return E_INVALIDARG;
  *input_scopes = static_cast<InputScope*>(CoTaskMemAlloc(
      sizeof(InputScope) * input_scopes_.size()));
  if (!input_scopes) {
    *count = 0;
    return E_OUTOFMEMORY;
  }
  ....
}

Предупреждение PVS-Studio: V649 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 67, 71. tsf_input_scope.cc 71

Нет смысла повторно проверять указатель input_scopes, так как если он будет нулевым, то сработает проверка в начале функции, и функция вернёт статус E_INVALIDARG.

Ошибка в том, что в "if (!input_scopes)" забыли оператор *. Из-за этого не проверятся указатель, который возвращает функция CoTaskMemAlloc. Код должен быть таким:

*input_scopes = static_cast<InputScope*>(CoTaskMemAlloc(
    sizeof(InputScope) * input_scopes_.size()));
if (!*input_scopes) {
 *count = 0;
  return E_OUTOFMEMORY;
}

Библиотека Skia.

SkOpSpan* SkOpContour::undoneSpan() {
  SkOpSegment* testSegment = &fHead;
  bool allDone = true;
  do {
    if (testSegment->done()) {
      continue;
    }
    allDone = false;
    return testSegment->undoneSpan();
  } while ((testSegment = testSegment->next()));
  if (allDone) {
    fDone = true;
  }
  return nullptr;
}

Анализатор PVS-Studio считает этот код подозрительным сразу по двум причинам:

V547 CWE-571 Expression 'allDone' is always true. skopcontour.cpp 43
V1001 CWE-563 The 'allDone' variable is assigned but is not used until the end of the function. skopcontour.cpp 40

Очень подозрительный код, но мне сложно понять, как он должен работать и в чём именно здесь ошибка. Желающие могут сами попробовать разобраться, как должна выглядеть функция undoneSpan на самом деле.

Библиотека WebKit.

WebString StringConstraint::ToString() const {
  ....
  bool first = true;
  for (const auto& iter : exact_) {
    if (!first)
      builder.Append(", ");
    builder.Append('"');
    builder.Append(iter);
    builder.Append('"');
  }
  ....
}

Предупреждение PVS-Studio: V547 CWE-570 Expression '!first' is always false. webmediaconstraints.cpp 302

Так как переменная first всегда равна true, то запятые между элементами добавляться не будут. Корректный вариант кода:

bool first = true;
for (const auto& iter : exact_) {
  if (first)
    first = false;
  else
    builder.Append(", ");
  builder.Append('"');
  builder.Append(iter);
  builder.Append('"');
}

Библиотека ICU.

uint32_t CollationDataBuilder::setPrimaryRangeAndReturnNext(....)
{
  ....
  } else {
    // Short range: Set individual CE32s.
    for(;;) {
      utrie2_set32(....);
      ++start;
      primary = Collation::incThreeBytePrimaryByOffset(....);
      if(start > end) { return primary; }
    }
    modified = TRUE;       // <=
  }
}

Предупреждение PVS-Studio: V779 CWE-561 Unreachable code detected. It is possible that an error is present. collationdatabuilder.cpp 392

Цикл может быть прерван только вызовом оператора return. Это значит, что присваивание, расположенное после цикла, никогда не будет выполнено.

Библиотека Ced.

void HzBoostWhack(DetectEncodingState* destatep,
                  uint8 byte1, uint8 byte2)
{
  if ((byte2 == '{') || (byte2 == '}')) {
    // Found ~{ or ~}
    Boost(destatep, F_HZ_GB_2312, kBoostOnePair);         
  } else if ((byte2 == '~') || (byte2 == '\n')) {
    // neutral
    destatep->enc_prob[F_HZ_GB_2312] += 0;                
  } else {
    // Illegal pair
    Whack(destatep, F_HZ_GB_2312, kBadPairWhack);         
  }
}

Предупреждение PVS-Studio: V751 Parameter 'byte1' is not used inside function body. compact_enc_det.cc 2559

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

Неправильные ожидания

Иногда программисты ошибочно представляют себе, как работают некоторые функции или конструкции языка. Давайте посмотрим на некоторые такие ошибки.

Проект Chromium.

void OnConvertedClientDisconnected() {
  // We have no direct way of tracking which
  // PdfToEmfConverterClientPtr got disconnected as it is a
  // movable type, short of using a wrapper.
  // Just traverse the list of clients and remove the ones
  // that are not bound.

  std::remove_if(
    g_converter_clients.Get().begin(),
    g_converter_clients.Get().end(),
    [](const mojom::PdfToEmfConverterClientPtr& client) {
      return !client.is_bound();
    });
}

Предупреждение PVS-Studio: V530 CWE-252 The return value of function 'remove_if' is required to be utilized. pdf_to_emf_converter.cc 44

Функция remove_if ничего не удаляет, а только переставляет элементы в контейнере. Скорее всего, здесь должно быть написано:

auto trash = std::remove_if(........);
g_converter_clients.Get().erase(trash,
                                g_converter_clients.Get().end());

V8 engine.

void StringStream::Add(....) {
  ....
    case 'f': case 'g': case 'G': case 'e': case 'E': {
      double value = current.data_.u_double_;
      int inf = std::isinf(value);
      if (inf == -1) {
        Add("-inf");
      } else if (inf == 1) {
        Add("inf");
      } else if (std::isnan(value)) {
        Add("nan");
      } else {
        EmbeddedVector<char, 28> formatted;
        SNPrintF(formatted, temp.start(), value);
        Add(formatted.start());
      }
      break;
    }  ....
}

Предупреждение PVS-Studio: V547 CWE-570 Expression 'inf == - 1' is always false. string-stream.cc 149

Описание функции std::isinf: isinf.

Как видите, функция std::isinf возвращает тип bool. Таким образом, эта функция явно используется неправильно.

Библиотека Skia.

GrGLProgram* GrGLProgramBuilder::finalize() {
  ....
  std::unique_ptr<char> binary(new char[length]);
  ....
}

Предупреждение PVS-Studio: V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. grglprogrambuilder.cpp 272

Память выделяется с помощью оператора new [], а освобождаться будет с помощью оператора delete. Классу unique_ptr надо подсказать, как управлять памятью. Правильный код:

std::unique_ptr<char[]> binary(new char[length]);

Ещё один схожий ляп в библиотеке Skia:

GrGLProgram* GrGLProgramBuilder::finalize() {
  ....
  std::unique_ptr<uint8_t> data((uint8_t*) malloc(dataLength));
  ....
}

Предупреждение PVS-Studio: V554 CWE-762 Incorrect use of unique_ptr. The memory allocated with 'malloc' will be cleaned using 'delete'. grglprogrambuilder.cpp 275

Явно кто-то из программистов узнал о существовании класса std::unique_ptr, но пользоваться им ещё не умеет :). Память выделяется с помощью функции malloc, а освобождается с помощью оператора delete.

Правильный код:

std::unique_ptr<uint8_t, void (*)(void*)>
  data((uint8_t*) malloc(dataLength), std::free);

Библиотека WebKit.

struct ScrollAnchorData {
  WebString selector_;
  WebFloatPoint offset_;
  uint64_t simhash_;

  ScrollAnchorData(const WebString& selector,
                   const WebFloatPoint& offset,
                   uint64_t simhash)
    : selector_(selector), offset_(offset), simhash_(simhash) {}

  ScrollAnchorData() { 
    ScrollAnchorData(WebString(), WebFloatPoint(0, 0), 0); }
};

Предупреждение PVS-Studio: V603 CWE-665 The object was created but it is not being used. If you wish to call constructor, 'this->ScrollAnchorData::ScrollAnchorData(....)' should be used. webscrollanchordata.h 49

Это не вызов одного конструктора из другого конструктора. Просто создаётся и тут же уничтожается неименованный объект.

Неправильные проверки указателей

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

Первый вариант – это когда вначале указатель разыменовывают, а уже потом проверяют:

p[n] = 1;
if (!p) return false;

Второй вариант – это когда вначале проверяли указатель перед использованием, а потом забыли это сделать:

if (p) p[0] = x;
p[1] = y;

Ошибки первого типа выявляет диагностика V595, а второго типа - диагностика V1004.

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

Тем не менее, сообщения V595 и V1004 заслуживают пристального внимания и правки кода. Анализатор PVS-Studio выдал кучу таких сообщений для кода Chromium и библиотек. К сожалению, как я описал в вводной статье, из-за макроса DCHECK большинство из них являются ложными срабатываниями. Поэтому мне очень быстро надоело просматривать эти сообщения. Более внимательно подойти к сообщениям V595 и V1004 следует после настройки анализатора.

В любом случае, уверяю, что ошибок, связанных с неправильной проверкой указателей, полно. Некоторые из них вы сможете найти, заглянув в файл chromium.txt. Чтобы найти остальные, нужны герои, которые настроят анализатор и изучат новый отчёт.

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

V595, первый пример, проект Chromium.

template <typename T>
void PaintOpReader::ReadFlattenable(sk_sp<T>* val) {
  // ....
  // Здесь аругмент val не используется и не проверяется.
  // ....  
  val->reset(static_cast<T*>(SkValidatingDeserializeFlattenable(
      const_cast<const char*>(memory_), bytes,
      T::GetFlattenableType())));
  if (!val)
    SetInvalid();
  ....
}

Предупреждение PVS-Studio: V595 CWE-476 The 'val' pointer was utilized before it was verified against nullptr. Check lines: 124, 126. paint_op_reader.cc 124

Указатель val разыменовывается без проверки на равенство nullptr.

V595, второй пример, проект Chromium.

void HttpAuthHandlerRegistryFactory::RegisterSchemeFactory(
    const std::string& scheme, 
    HttpAuthHandlerFactory* factory)
{
  factory->set_http_auth_preferences(http_auth_preferences());
  std::string lower_scheme = base::ToLowerASCII(scheme);
  if (factory)
    factory_map_[lower_scheme] = base::WrapUnique(factory);
  else
    factory_map_.erase(lower_scheme);
}

Предупреждение PVS-Studio: V595 CWE-476 The 'factory' pointer was utilized before it was verified against nullptr. Check lines: 122, 124. http_auth_handler_factory.cc 122

Указатель factory разыменовывается без проверки на равенство nullptr.

V1004, первый пример, библиотека PDFium.

void CFX_PSRenderer::SetClip_PathStroke(....,
  const CFX_Matrix* pObject2Device, ....)
{
  ....
  if (pObject2Device) {
    ....
  }
  ....
  m_ClipBox.Intersect(
    pObject2Device->TransformRect(rect).GetOuterRect());
  ....
}

Предупреждение PVS-Studio: V1004 CWE-476 The 'pObject2Device' pointer was used unsafely after it was verified against nullptr. Check lines: 237, 248. cfx_psrenderer.cpp 248

Указатель pObject2Device может быть нулевым, о чём свидетельствует проверка этого указателя на равенство nullptr. Однако далее указатель разыменовывается без предварительной проверки.

V1004, второй пример, библиотека SwiftShader.

VertexProgram::VertexProgram(...., const VertexShader *shader)
  : VertexRoutine(state, shader),
    shader(shader),
    r(shader->dynamicallyIndexedTemporaries)
{
  ....
  if(shader && shader->containsBreakInstruction())
  {
    enableBreak = ....;
  }

  if(shader && shader->containsContinueInstruction())
  {
    enableContinue = ....;
  }

  if(shader->isInstanceIdDeclared())
  {
    instanceID = ....;
  }  
}

Предупреждение PVS-Studio: V1004 CWE-476 The 'shader' pointer was used unsafely after it was verified against nullptr. Check lines: 43, 53. vertexprogram.cpp 53

Указатель shader может быть нулевым, о чём свидетельствуют проверки этого указателя на равенство nullptr. Однако далее указатель разыменовывается без предварительной проверки.

Передаём привет разработчикам компании Google

Команда PVS-Studio передаёт привет разработчикам компании Google и готова к сотрудничеству. Возможно обсудить как минимум два варианта:

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

Пробуйте PVS-Studio. Пишите нам. Мы поможем с проверкой проектов и выдадим временную лицензию для подробного ознакомления.

Заключение

Спасибо всем, кто добрался до конца цикла статей. Надеюсь, вам было интересно.

Как видите, даже в таком качественном проекте как Chromium есть масса ошибок, которые способен выявлять анализатор PVS-Studio. Предлагаю и вам начать использовать его в своих проектах.

#Cpp