В 2018 году почти три месяца, а это значит, что пришло время (хотя и с некоторым опозданием) составить список топ-10 ошибок, обнаруженных анализатором PVS-Studio в проектах C ++ за последний год. Вот так!

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

Десятое место

Источник: Проверка Notepad ++: пять лет спустя

Ошибка была обнаружена в одном из самых популярных текстовых редакторов Notepad ++.

Вот код:

TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
  int returnvalue;
  TCHAR mbuffer[100];
  int result;
  BYTE keys[256];
  WORD dwReturnedValue;
  GetKeyboardState(keys);
  result = ToAscii(static_cast<UINT>(wParam),
    (lParam >> 16) && 0xff, keys, &dwReturnedValue, 0);
  returnvalue = (TCHAR) dwReturnedValue;
  if(returnvalue < 0){returnvalue = 0;}
  wsprintf(mbuffer, TEXT("return value = %d"), returnvalue);
  if(result!=1){returnvalue = 0;}
  return (TCHAR)returnvalue;
}

Предупреждение PVS-Studio: V560 Часть условного выражения всегда истинна: ​​0xff. babygrid.cpp 711

Анализатору не понравилось выражение (lParam ›› 16) && 0xff. Второй аргумент, переданный в функцию ToAscii, всегда будет иметь значение 0 или 1, что будет зависеть исключительно от левого подвыражения, (lParam ›› 16). Очевидно, что оператор & следует использовать вместо &&.

Девятое место

Источник: Передай привет разработчикам Яндекса

Эта ошибка была обнаружена в проекте ClickHouse, разработанном Яндекс.

bool executeForNullThenElse(....)
{
  ....
  const ColumnUInt8 * cond_col =
    typeid_cast<const ColumnUInt8 *>(arg_cond.column.get());
  ....
  if (cond_col)
  {
    ....
  }
  else if (cond_const_col)
  {
    ....
  }
  else
    throw Exception(
      "Illegal column " + cond_col->getName() +
      " of first argument of function " + getName() +
      ". Must be ColumnUInt8 or ColumnConstUInt8.",
      ErrorCodes::ILLEGAL_COLUMN);
  ....
}

Предупреждение PVS-Studio: V522 может иметь место разыменование нулевого указателя cond_col. FunctionsConditional.h 765

Этот код является примером неправильной обработки ошибки, требующей создания исключения. Обратите внимание на проверку указателя cond_col в операторе if. Если элемент управления достигает ветки else, где должно быть сгенерировано исключение, указатель cond_col определенно будет нулевым, но он будет разыменован в cond_col- › getName () при формировании текста сообщения об ошибке.

Восьмое место

Источник: Сравнение качества кода Firebird, MySQL и PostgreSQL

Это одна из ошибок, которые мы обнаружили в проекте MySQL при сравнении качества кода Firebird, MySQL и PostgreSQL.

Вот фрагмент кода с ошибкой:

mysqlx::XProtocol* active()
{
  if (!active_connection)
    std::runtime_error("no active session");
  return active_connection.get();
}

Предупреждение PVS-Studio: V596 Объект создан, но не используется. Ключевое слово throw могло отсутствовать: throw runtime_error (FOO); mysqlxtest.cc 509

Если нет активного соединения (! Active_connection), будет создан объект исключения типа std :: runtime_error и… все. После создания он будет просто удален, и метод будет продолжен. Очевидно, что программист забыл добавить ключевое слово throw для исключения.

Седьмое место

Источник: Как найти 56 потенциальных уязвимостей в коде FreeBSD за один вечер

Как найти 56 потенциальных уязвимостей за один вечер? Конечно, используя статический анализ!

Вот один из дефектов, обнаруженных в коде FreeBSD:

int mlx5_core_create_qp(struct mlx5_core_dev *dev,
      struct mlx5_core_qp *qp,
      struct mlx5_create_qp_mbox_in *in,
      int inlen)
{
  ....
  struct mlx5_destroy_qp_mbox_out dout;
  ....
err_cmd:
  memset(&din, 0, sizeof(din));
  memset(&dout, 0, sizeof(dout));
  din.hdr.opcode = cpu_to_be16(MLX5_CMD_OP_DESTROY_QP);
  din.qpn = cpu_to_be32(qp->qpn);
  mlx5_cmd_exec(dev, &din, sizeof(din), &out, sizeof(dout));
  return err;
}

Предупреждение PVS-Studio: V597 Компилятор мог удалить вызов функции memset, которая используется для сброса объекта dout. Для удаления личных данных следует использовать функцию memset_s (). mlx5_qp.c 159

Обратите внимание на выражение memset (& dout, 0, sizeof (dout)). Программист хотел стереть данные в блоке памяти, выделенном для dout, заполнив этот блок нулями. Этот метод обычно используется, когда вам нужно стереть некоторые личные данные, чтобы они не «задерживались» в памяти.

Однако после этого dout нигде не используется (sizeof (dout) не учитывается), что позволяет компилятору удалить этот вызов memset поскольку такая оптимизация не повлияет на поведение программы с точки зрения C / C ++. В результате данные, предназначенные для стирания, все еще могут быть там.

Вот еще кое-что по этому поводу:

Шестое место

Источник: Долгожданная проверка CryEngine V

CryEngine V, первый игровой движок в этом списке.

int CTriMesh::Slice(....)
{
  ....
  bop_meshupdate *pmd = new bop_meshupdate, *pmd0;
  pmd->pMesh[0]=pmd->pMesh[1] = this;  AddRef();AddRef();
  for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next);
    pmd0->next = pmd;
  ....
}

Предупреждение PVS-Studio: V529 Нечетная точка с запятой ; после оператора for. boolean3d.cpp 1314

Если бы я не процитировал этот фрагмент кода, как я, - сокращенный и изолированный от остальной части кода - вы бы так же легко заметили ошибку - этот подозрительный ';' после того, как цикл for указал анализатором? Обратите внимание, как форматирование кода (отступ перед следующим выражением) также предполагает, что символ «;» не нужен и что pmd0- ›next = pmd; выражение должно быть телом цикла. Но, по логике цикла for, в этом месте имеет место неправильное форматирование кода, что сбивает с толку, а не логическая ошибка. Кстати, в CryEngine поправили форматирование кода.

Пятое место

Источник: Статический анализ как часть процесса разработки в Unreal Engine

Этот дефект был обнаружен при исправлении ранее обнаруженных PVS-Studio ошибок в коде игрового движка Unreal Engine.

for(int i = 0; i < SelectedObjects.Num(); ++i)
{
  UObject* Obj = SelectedObjects[0].Get();
  EdObj = Cast<UEditorSkeletonNotifyObj>(Obj);
  if(EdObj)
  {
    break;
  }
}

Предупреждение PVS-Studio: V767 Подозрительный доступ к элементу массива SelectedObjects по постоянному индексу внутри цикла. skeletonnotifydetails.cpp 38

Программист хотел, чтобы цикл проходил по всем элементам, чтобы найти первый элемент типа UEditorSkeletonNotifyObj, но допустил досадную опечатку, используя постоянный индекс 0 вместо счетчика цикла i в выражение SelectedObjects [0] .Get (). Это заставит цикл проверять только первый элемент.

Четвертое место

Источник: 27 000 ошибок в операционной системе Tizen

Эта ошибка была обнаружена при проверке операционной системы Tizen вместе со сторонними компонентами, которые она использовала. Статья длинная; он содержит много хороших примеров ошибок, поэтому я рекомендую проверить его.

Но вернемся к этому конкретному случаю:

int _read_request_body(http_transaction_h http_transaction,
                       char **body)
{
  ....
  *body = realloc(*body, new_len + 1);
  ....
  memcpy(*body + curr_len, ptr, body_size);
  body[new_len] = '\0';
  curr_len = new_len;
  ....
}

Предупреждение PVS-Studio: V527 Странно, что значение \ 0 присвоено указателю типа char. Вероятно, имелось ввиду: * body [new_len] = ‘\ 0’. http_request.c 370

Ошибка скрывается в выражении body [new_len] = ‘\ 0’. Обратите внимание, что параметр body имеет тип char **, поэтому результат выражения body [new_len] имеет тип char *. Но разработчик допустил ошибку, еще раз забыв разыменовать указатель, и попытался записать в указатель значение «\ 0» (которое будет интерпретировано как нулевой указатель).

Это приводит нас к этим двум проблемам:

  • Нулевой указатель будет написан в глуши.
  • В конец строки не будет добавлен нулевой символ.

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

(*body)[new_len] = '\0';

Третье место

Источник: Как PVS-Studio может помочь в обнаружении уязвимостей?

Мы вошли в тройку лидеров. Приведенный ниже фрагмент кода привлек наше внимание, когда мы искали ответ на вопрос: «Насколько хорошо PVS-Studio выполняет поиск по CVE?» (ответ смотрите в статье выше). Код взят из проекта illumos-gate.

static int devzvol_readdir(....)
{
  ....
  char *ptr;
  ....
  ptr = strchr(ptr + 1, '/') + 1;
  rw_exit(&sdvp->sdev_contents);
  sdev_iter_datasets(dvp, ZFS_IOC_DATASET_LIST_NEXT, ptr);
  ....
}

Предупреждение PVS-Studio: V769 Указатель 'strchr (ptr + 1,' / ')' в выражении 'strchr (ptr + 1,' / ') + 1' мог быть nullptr. В таком случае результирующее значение будет бессмысленным, и его не следует использовать.

Функция strchr возвращает указатель на первое вхождение символа, указанного вторым аргументом, в строке, указанной первым аргументом. Если такой символ не найден, strchr вернет NULL. Однако программист не принимает во внимание эту возможность и добавляет значение 1 к любому возвращаемому значению. В результате указатель ptr всегда будет отличаться от нуля, а это значит, что дальнейшие проверки ptr! = NULL не смогут определить, действителен ли он. При определенных обстоятельствах это в конечном итоге приведет к панике ядра.

Эта ошибка была классифицирована как CVE-2014–9491: функция devzvol_readdir в illumos не проверяет возвращаемое значение вызова strchr, что позволяет удаленным злоумышленникам вызвать отказ service (разыменование указателя NULL и паника) через неопределенные векторы.

Хотя эта CVE была впервые обнаружена в 2014 году, мы обнаружили ее в ходе нашего собственного исследования в 2017 году, и поэтому она находится в этом списке.

Второе место

Источник: Статический анализ как часть процесса разработки в Unreal Engine

Ошибка, занявшая второе место, была обнаружена в… да, снова в Unreal Engine. Мне это слишком нравится, чтобы не упомянуть об этом.

Примечание. Я действительно думал включить еще пару примеров из статьи о Unreal Engine, но тогда в одном проекте было бы слишком много ошибок, а я не хотел. Так что я рекомендую вам ознакомиться с этой статьей, особенно с предупреждениями V714 и V709.

Этот пример довольно длинный, но вам понадобится весь этот код, чтобы понять, в чем проблема.

bool FCreateBPTemplateProjectAutomationTests::RunTest(
  const FString& Parameters)
{
  TSharedPtr<SNewProjectWizard> NewProjectWizard;
  NewProjectWizard = SNew(SNewProjectWizard);
  TMap<FName, TArray<TSharedPtr<FTemplateItem>> >& Templates =
    NewProjectWizard->FindTemplateProjects();
  int32 OutMatchedProjectsDesk = 0;
  int32 OutCreatedProjectsDesk = 0;
  GameProjectAutomationUtils::CreateProjectSet(Templates, 
    EHardwareClass::Desktop, 
    EGraphicsPreset::Maximum, 
    EContentSourceCategory::BlueprintFeature,
    false,
    OutMatchedProjectsDesk,
    OutCreatedProjectsDesk);
  int32 OutMatchedProjectsMob = 0;
  int32 OutCreatedProjectsMob = 0;
  GameProjectAutomationUtils::CreateProjectSet(Templates, 
    EHardwareClass::Mobile,
    EGraphicsPreset::Maximum,
    EContentSourceCategory::BlueprintFeature,
    false,
    OutMatchedProjectsMob,
    OutCreatedProjectsMob);
  return ( OutMatchedProjectsDesk == OutCreatedProjectsDesk ) &&
         ( OutMatchedProjectsMob  == OutCreatedProjectsMob  );
}

Обратите внимание на одну вещь, необходимую для понимания проблемы. Пары переменных OutMatchedProjectsDesk, OutCreatedProjectsDesk и OutMatchedProjectsMob, OutCreatedProjectsMob инициализируются нулевым значением при объявлении и затем передаются в качестве аргументов метода CreateProjectSet.

После этого они сравниваются в выражении в инструкции return. Следовательно, метод CreateProjectSet должен инициализировать два последних аргумента.

Теперь давайте посмотрим на метод CreateProjectSet, в котором были допущены ошибки.

static void CreateProjectSet(.... int32 OutCreatedProjects,
                                  int32 OutMatchedProjects)
{
  ....
  OutCreatedProjects = 0;
  OutMatchedProjects = 0;
  ....
  OutMatchedProjects++;
  ....
  OutCreatedProjects++;
  ....
}

Предупреждения PVS-Studio:

  • V763 Параметр OutCreatedProjects всегда перезаписывается в теле функции перед использованием. gameprojectautomationtests.cpp 88
  • V763 Параметр OutMatchedProjects всегда перезаписывается в теле функции перед использованием. gameprojectautomationtests.cpp 89

Программист забыл объявить параметры OutCreatedProjects и OutMatchedProjects как ссылки, что приводит к простому копированию значений их соответствующих аргументов. В результате показанный ранее метод RunTest всегда возвращает true, поскольку все сравниваемые переменные сохраняют одно и то же значение, присвоенное при инициализации - 0.

Это правильная версия:

static void CreateProjectSet(.... int32 &OutCreatedProjects,
                                  int32 &OutMatchedProjects)

Первое место

Источник: Цени статический анализ кода!

Как только я увидел этот баг, у меня не осталось сомнений относительно лидера этого топа. Что ж, смотрите сами. Пожалуйста, не читайте дальше, пока сами не обнаружите ошибку в приведенном ниже коде. Кстати, StarEngine - тоже игровой движок.

PUGI__FN bool set_value_convert(
  char_t*& dest,
  uintptr_t& header,
  uintptr_t header_mask,
  int value)
{
  char buf[128];
  sprintf(buf, "%d", value);
  return set_value_buffer(dest, header, header_mask, buf);
}

Итак, есть ли удача в поиске ошибки? :)

Предупреждение PVS-Studio: V614 Использован неинициализированный буфер buf. Рассмотрите возможность проверки первого фактического аргумента функции printf. pugixml.cpp 3362

Вы, должно быть, задавались вопросом: «printf? Почему анализатор упоминает printf, когда есть только вызов sprint? »

Вот и все! sprintf - это макрос, расширяющийся до (!) std :: printf!

#define sprintf std::printf

В результате неинициализированный буфер buf используется как строка формата. Круто, правда? Я считаю, что эта ошибка заслуживает первого места.

Ссылка на заголовочный файл с объявлением макроса.

Заключение

Надеюсь, вам понравились ошибки в этом списке. Лично мне они показались довольно интересными. У вас, конечно, может быть другое мнение, поэтому смело составляйте свой список Топ-10… на основе статей в нашем блоге или списка дефектов, обнаруженных PVS-Studio в open-source проектах.

Напомним, все перечисленные здесь дефекты (а также многие другие) были обнаружены анализатором PVS-Studio, который я рекомендую попробовать и на своих проектах - скачать здесь.