Топ 10 ошибок в C++ проектах за 2018 год +47



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



Занимательные места мы искали с помощью статического анализатора кода PVS-Studio. Он умеет обнаруживать ошибки и потенциальные уязвимости в коде, написанном на упомянутых выше языках.

Если вам интересно поискать ошибки самостоятельно, вы всегда можете скачать и попробовать наш анализатор. Мы предоставляем бесплатную версию анализатора для студентов и программистов-энтузиастов, бесплатную лицензию для разработчиков open-source проектов, а также ознакомительную версию для всех-всех-всех. Кто знает, может быть к следующему году вы сможете составить свой топ-10? :)

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

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


Источник: И снова в космос: как единорог Stellarium посещал

Эта ошибка была обнаружена при проверке виртуального планетария Stellarium.

Приведенный фрагмент кода хоть и является небольшим, но таит в себе довольно хитрую ошибку:

Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
  : distance(0.0f), sDistance(0.0f)
{
  Plane(v1, v2, v3, SPolygon::CCW);
}

Нашли?

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

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

Вместо вложенного вызова конструктора следовало использовать делегирующий конструктор, введенный в C++11. Например, можно было сделать так:

Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3)
  : Plane(v1, v2, v3, SPolygon::CCW)
{
  distance = 0.0f;
  sDistance = 0.0f;
}

Тогда все необходимые поля были бы корректно проинициализированы. Разве не замечательно?

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


Источник: Perl 5: как в макросах ошибки прятались

На девятом месте красуется примечательный макрос из исходного кода Perl 5.

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

PP(pp_match)
{
  ....
  MgBYTEPOS_set(mg, TARG, truebase, RXp_OFFS(prog)[0].end);
  ....
}

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

(((targ)->sv_flags & 0x00000400) && (!((targ)->sv_flags & 0x00200000) ||
S_sv_only_taint_gmagic(targ)) ? (mg)->mg_len = ((prog->offs)[0].end),
(mg)->mg_flags |= 0x40 : ((mg)->mg_len = (((targ)->sv_flags & 0x20000000)
&& !__builtin_expect(((((PL_curcop)->cop_hints + 0) & 0x00000008) ?
(_Bool)1 :(_Bool)0),(0))) ? (ssize_t)Perl_utf8_length( (U8 *)(truebase),
(U8 *)(truebase)+((prog->offs)[0].end)) : (ssize_t)((prog->offs)[0].end),
(mg)->mg_flags &= ~0x40));

Предупреждение PVS-Studio: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '&&' operator. pp_hot.c 3036

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

Говорят, что макросы — зло. Конечно, бывают моменты, когда они оказываются незаменимыми, но если есть возможность заменить макрос на функцию — следует обязательно это сделать.

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

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


Источник: Chromium: другие ошибки

Следующий пример был взят из цикла статей об анализе проекта Chromium. Крылась она в библиотеке 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

Ошибка заключается в том, что размер вектора formats изменяется внутри range-based-for цикла. Range-based циклы основаны на итераторах, поэтому изменение размера контейнера внутри таких циклов может привести к инвалидации этих итераторов.

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

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

Например, при использовании метода push_back может произойти реаллокация вектора — и тогда итераторы станут указывать на недопустимую область памяти.

Чтобы избежать подобных ошибок, следует придерживаться правила: никогда не изменяйте размер контейнера внутри цикла, условия которого привязаны к этому контейнеру. Это касается range-based-циклов и циклов, использующих итераторы. О том, какие операции могут повлечь инвалидацию итераторов, можно почитать в дискуссии на StackOverflow.

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


Источник: Godot: к вопросу о регулярном использовании статических анализаторов кода

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

void AnimationNodeBlendSpace1D::add_blend_point(
  const Ref<AnimationRootNode> &p_node, float p_position, int p_at_index)
{
  ERR_FAIL_COND(blend_points_used >= MAX_BLEND_POINTS);
  ERR_FAIL_COND(p_node.is_null());

  ERR_FAIL_COND(p_at_index < -1 || p_at_index > blend_points_used);

  if (p_at_index == -1 || p_at_index == blend_points_used) {
    p_at_index = blend_points_used;
  } else {
    for (int i = blend_points_used - 1; i > p_at_index; i++) {
      blend_points[i] = blend_points[i - 1];
    }
  }
  ....
}

Предупреждение PVS-Studio: V621 CWE-835 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. animation_blend_space_1d.cpp 113

Рассмотрим подробнее условие цикла. Переменная-счетчик инициализируется значением blend_points_used — 1. При этом, исходя из двух предыдущих проверок (в ERR_FAIL_COND и в if), становится понятно, что на момент выполнения цикла blend_points_used будет всегда больше, чем p_at_index. Таким образом, либо условие цикла всегда будет истинно, либо цикл не будет выполняться совсем.

Если blend_points_used — 1 == p_at_index, то цикл не выполняется.

Во всех остальных случаях проверка i > p_at_index всегда будет истинной, так как счётчик i увеличивается на каждой итерации цикла.

Может показаться, что цикл будет выполняться вечно, но это не так.

Во-первых, возникнет целочисленное переполнение переменной i, что является неопределенным поведением. Следовательно, полагаться на это не стоит.

Если бы i имела тип unsigned int, то после достижения счетчиком максимально возможного значения оператор i++ превратил бы его в 0. Такое поведение определено стандартом и называется «Unsigned wrapping». Однако следует знать, что использование такого механизма тоже является не очень хорошей идеей.

Это было во-первых, но ведь еще есть во-вторых! Дело в том, что до целочисленного переполнения даже не дойдет. Куда раньше произойдет выход за границу массива. Это значит, что произойдет попытка доступа к области памяти за пределами выделенного для массива блока. И это тоже является неопределенным поведением. Классический пример :)

Чтобы было легче избегать подобных ошибок, могу дать лишь пару рекомендаций:

  1. Пишите более простой и понятный код
  2. Проводите более тщательный Code Review и пишите больше тестов для свеженаписанного кода
  3. Используйте статические анализаторы ;)


Шестое место


Источник: Amazon Lumberyard: крик души

Еще один пример из индустрии геймдева, а именно — из исходного кода ААА-движка Amazon Lumberyard.

void TranslateVariableNameByOperandType(....)
{
  //  Igor: yet another Qualcomm's special case
  //  GLSL compiler thinks that -2147483648 is
  //  an integer overflow which is not
  if (*((int*)(&psOperand->afImmediates[0])) == 2147483648)
  {
    bformata(glsl, "-2147483647-1");
  }
  else
  {
    //  Igor: this is expected to fix
    //  paranoid compiler checks such as Qualcomm's
    if (*((unsigned int*)(&psOperand->afImmediates[0])) >= 2147483648)
    {
      bformata(glsl, "%d",
          *((int*)(&psOperand->afImmediates[0])));
    }
    else
    {
      bformata(glsl, "%d",
          *((int*)(&psOperand->afImmediates[0])));
    }
  }
  bcatcstr(glsl, ")");
  ....
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700

Amazon Lumberyard разрабатывается как кроссплатформенный движок. Поэтому разработчики стараются поддерживать как можно больше компиляторов. Программист Игорь столкнулся с компилятором Qualcomm, о чем нам говорят комментарии.

Неизвестно, смог ли Игорь выполнить свою задачу и справиться с «параноидальными» проверками компилятора, но он оставил после себя весьма странный код. Странный он тем, что как then-, так и else-ветви оператора if содержат абсолютно идентичный код. Скорее всего, такая ошибка допущена в результате неаккуратного Copy-Paste.

Даже не знаю, что тут можно посоветовать. Поэтому просто пожелаю разработчикам Amazon Lumberyard успехов в исправлении ошибок, а программисту Игорю – удачи!

Пятое место


Источник: В очередной раз анализатор PVS-Studio оказался внимательнее человека

Со следующим примером приключилась интересная история. Мой коллега Андрей Карпов готовил статью об очередной проверке фреймворка Qt. В ходе выписывания примечательных ошибок он столкнулся с предупреждением анализатора, которое посчитал ложным. Вот соответствующий фрагмент кода и предупреждение:

QWindowsCursor::CursorState QWindowsCursor::cursorState()
{
  enum { cursorShowing = 0x1, cursorSuppressed = 0x2 };
  CURSORINFO cursorInfo;
  cursorInfo.cbSize = sizeof(CURSORINFO);
  if (GetCursorInfo(&cursorInfo)) {
    if (cursorInfo.flags & CursorShowing)   // <= V616
  ....
}

Предупреждение PVS-Studio: V616 CWE-480 The 'CursorShowing' named constant with the value of 0 is used in the bitwise operation. qwindowscursor.cpp 669

То есть PVS-Studio ругался на место, в котором, очевидно, ошибки нет! Не может быть, чтобы константа CursorShowing равнялась 0, ведь буквально парой строк выше она инициализирована значением 1.

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

И только при подробном анализе выяснилось, что PVS-Studio вновь оказался внимательнее человека. Значение 0x1 присваивается именованной константе cursorShowing, а в побитовой операции «и» участвует именованная константа CursorShowing. Это совершенно разные константы, ведь первая начинается со строчной буквы, а вторая — с заглавной.

Код успешно компилируется, ведь класс QWindowsCursor действительно содержит константу с таким именем. Вот её определение:

class QWindowsCursor : public QPlatformCursor
{
public:
  enum CursorState {
    CursorShowing,
    CursorHidden,
    CursorSuppressed
  };
  ....
}

Если именованной enum-константе не присвоить значение явно, оно будет инициализировано по умолчанию. Так как CursorShowing является первым элементом перечисления, ему будет присвоено значение 0.

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

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


Источник: Стреляем в ногу, обрабатывая входные данные

Мы приближаемся к тройке финалистов, а на очереди ошибка из проекта FreeSWITCH.

static const char *basic_gets(int *cnt)
{
  ....
  int c = getchar();
  if (c < 0) {
    if (fgets(command_buf, sizeof(command_buf) - 1, stdin) 
          != command_buf) {
      break;
    }
    command_buf[strlen(command_buf)-1] = '\0'; /* remove endline */
    break;
  }
  ....
}

Предупреждение PVS-Studio: V1010 CWE-20 Unchecked tainted data is used in index: 'strlen(command_buf)'.

Анализатор предупреждает, что в выражении strlen(command_buf) — 1 используются непроверенные данные. И действительно: если command_buf окажется пустой с точки зрения языка C строкой (содержащей единственный символ — '\0') то strlen(command_buf) вернет 0. В таком случае произойдет обращение command_buf[-1], что представляет собой неопределенное поведение. Беда!

Самый сок этой ошибки даже не в том, почему она случается, а в том, как она случается. Данная ошибка является одним из тех приятных примеров, которые можно «пощупать» самостоятельно, воспроизвести. Можно запустить FreeSwitch, произвести некоторые действия, которые приведут к выполнению приведенного выше участка кода, и передать программе пустую строку на вход.

В итоге легким движением руки работающая программа превращается (да нет, не в элегантные шорты) в неработающую! Подробности о том, как воспроизвести эту ошибку, вы можете найти в статье-источнике по приведенной выше ссылке, а пока я приведу наглядный результат:



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

Теперь пора разобраться с нашими победителями: мы переходим в финал!



Третье место


Источник: NCBI Genome Workbench: научные исследования под угрозой

Тройку победителей открывает участок кода из проекта NCBI Genome Workbench — набора инструментов для изучения и анализа генетических данных. Хоть и не обязательно быть генетически модифицированным сверхчеловеком, чтобы обнаружить здесь ошибку, но знают о подобной возможности ошибиться довольно немногие.

/**
 * Crypt a given password using schema required for NTLMv1 authentication
 * @param passwd clear text domain password
 * @param challenge challenge data given by server
 * @param flags NTLM flags from server side
 * @param answer buffer where to store crypted password
 */
void
tds_answer_challenge(....)
{
  ....
  if (ntlm_v == 1) {
    ....
    /* with security is best be pedantic */
    memset(hash, 0, sizeof(hash));
    memset(passwd_buf, 0, sizeof(passwd_buf));
    ...
  } else {
    ....
  }
}

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

  • V597 The compiler could delete the 'memset' function call, which is used to flush 'hash' buffer. The memset_s() function should be used to erase the private data. challenge.c 365
  • V597 The compiler could delete the 'memset' function call, which is used to flush 'passwd_buf' buffer. The memset_s() function should be used to erase the private data. challenge.c 366

Сумели найти ошибку? Если да, то вы — молодец!.. ну или всё-таки генетически модифицированный сверхчеловек.

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

В таком случае они могут удалить «ненужный» вызов memset, и имеют на это полное право. Тогда буфер, который хранит важные данные, может остаться в памяти на радость злоумышленникам.

На фоне этого грамотейский комментарий «с безопасностью есть хорошо быть педантичным» выглядит еще забавнее. Судя по очень небольшому количеству предупреждений, выданных на этот проект, разработчики очень старались быть аккуратными и писать безопасный код. Однако, как мы видим, пропустить этот дефект безопасности очень просто. Согласно Common Weakness Enumeration дефект классифицируется как CWE-14: Compiler Removal of Code to Clear Buffers.

Чтобы очищение памяти было безопасным, следует использовать функцию memset_s(). Она не только является более безопасной, чем memset(), но еще и не может быть «проигнорирована» компилятором.

Второе место


Источник: Как PVS-Studio оказался внимательнее, чем три с половиной программиста

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

Письмо получил Евгений, бегло его просмотрел, и отправил Святославу. Святослав вдумчиво посмотрел на участок кода, присланный клиентом, и подумал «разве может анализатор настолько грубо ошибаться?». Поэтому он пошел за консультацией к Андрею. Он тоже проверил участок и решил: действительно, анализатор выдает ложные срабатывания.

Что поделаешь, надо исправлять. И только когда Святослав начал делать синтетические примеры, чтобы оформить задачу в багтрекер, он понял, в чем дело.

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

И это при том, что анализатор явно выдал предупреждения на ошибочные места!

Получится ли у вас найти такую пронырливую ошибку? Проверьте себя на зоркость и внимательность.


Предупреждение PVS-Studio:
  • V560 A part of conditional expression is always false: (ch >= 0x0FF21). decodew.cpp 525
  • V560 A part of conditional expression is always true: (ch <= 0x0FF3A). decodew.cpp 525
  • V560 A part of conditional expression is always false: (ch >= 0x0FF41). decodew.cpp 525
  • V560 A part of conditional expression is always true: (ch <= 0x0FF5A). decodew.cpp 525

Если у вас получилось — моего уважения вам не занимать!

Кроется ошибка в том, что оператор логического отрицания (!) применяется не ко всему условию, а только к его первому подвыражению:

!((ch >= 0x0FF10) && (ch <= 0x0FF19))

Если это условие выполняется, то значение переменной ch лежит в отрезке [0x0FF10...0x0FF19]. Тем самым, четыре дальнейших сравнения уже не имеют смысла: они всегда будут либо истинны, либо ложны.

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

const bool isLetterOrDigit =    (ch >= 0x0FF10 && ch <= 0x0FF19)  // 0..9
                             || (ch >= 0x0FF21 && ch <= 0x0FF3A)  // A..Z
                             || (ch >= 0x0FF41 && ch <= 0x0FF5A); // a..z 
if (!isLetterOrDigit)

Тогда, во-первых, скобок становится гораздо меньше, а во-вторых — увеличивается вероятность «поймать» случайно допущенную ошибку глазами.

А теперь — вишенка: мы переходим к первому месту!

Первое место


Источник: Система в шоке: интересные ошибки в исходных кодах легендарного System Shock

Итак, финалистом нашего сегодняшнего топа становится ошибка из легендарного System Shock! Эта игра, вышедшая аж в 1994 году, стала прародителем и вдохновителем таких культовых игр, как Dead Space, BioShock и Deus Ex.

Но сначала я должен кое в чем признаться. То, что я вам сейчас покажу, не содержит никакой ошибки. По большому счету, оно даже не является отрывком кода, но я просто не смог удержаться, чтобы не поделиться этим с вами!

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


// I'll give you fish, I'll give you candy, 
// I'll give you, everything I have in my hand

// that kid from the wrong side came over my house again,
// decapitated all my dolls
// and if you bore me, you lose your soul to me 
// - "Gepetto", Belly, _Star_

//  And here, ladies and gentlemen, 
// is a celebration of C and C++ and their untamed passion...
//  ==================
TerrainData  terrain_info;
//  Now the actual stuff...
//  =======================

// this is all outrageously horrible, as we dont know what
// we really need to deal with here

// And if you thought the hack for papers was bad,
// wait until you see the one for datas... - X

// Returns whether or not in the humble opinion of the
// sound system, the sample should be politely obliterated 
// out of existence

// it's a wonderful world, with a lot of strange men
// who are standing around, and they all wearing towels

Для наших русскоязычных читателей я сделал приблизительный вольный перевод:

// Я дам тебе рыбку, я дам тебе конфетку, 
// Я дам тебе все, что есть у меня в руках 

// этот мальчик с противоположной стороны 
// снова пришел ко мне домой
// обезглавил всех моих кукол
// и если ты мне надоешь, ты потеряешь душу для меня 
// - "Gepetto", Belly, _Star_

//  И вот, дамы и господа,
// перед нами триумф C и C++ и их неукрощенной страсти
//  ==================
TerrainData  terrain_info;
//  А теперь к делу...
//  =======================

// это всё невыразимо ужасающе, так как мы не знаем,
// с чем нам действительно нужно иметь здесь дело

// И если вы думали, что что хак для работы с бумагами был плох
// это вы еще не видели тот, что используется для работы с данными... - X

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

// это чудный мир, с кучей странных мужчин
// что повсюду стоят, и они в полотенцах

Вот такие комментарии оставляли разработчики игры в начале далеких девяностых… Между прочим, Даг Чёрч — главный дизайнер System Shock — занимался еще и написанием кода. Кто знает, может быть какие-нибудь из этих комментариев написаны лично им? Надеюсь, про мужчин в полотенцах — это не его рук дело :)

Заключение


В заключение хочу поблагодарить своих коллег за то, что искали новые ошибки и писали о них статьи. Спасибо, ребята! Без вас эта статья не получилась бы такой интересной.

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

Например, мы добавили поддержку нескольких новых компиляторов и расширили список диагностических правил. Также мы осуществили первоначальную поддержку стандартов MISRA C и MISRA C++. Самым главным и трудоемким нововведением стала поддержка нового языка. Да, теперь мы умеем анализировать код на Java! А еще у нас обновилась иконка :)

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

На этом наш топ-10 C++ ошибок за 2018-й год подошел к концу. Какие места вам понравились больше всего и почему? Довелось ли вам столкнуться в 2018-м году с интересными примерами? Расскажите об этом в комментариях!

До следующих встреч!



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. Top 10 bugs of C++ projects found in 2018

Вы можете помочь и перевести немного средств на развитие сайта



Комментарии (25):

  1. ianzag
    /#19916626 / +5

    > Даже не знаю, что тут можно посоветовать. Поэтому просто пожелаю разработчикам Amazon Lumberyard успехов в исправлении ошибок, а программисту Игорю – удачи!

    Ну судя по этому коду товарищ Игорь Лобанчиков в целом широко отметился в этом проекте. Но — не оценили его комрады…

  2. dreamer-dead
    /#19916690

    Amazon Lumberyard разрабатывается как кроссплатформенный движок. Поэтому разработчики стараются поддерживать как можно больше компиляторов.


    Я надеюсь, было понятно, что это не С или С++ компилятор, так что странно читать о кроссплатформенности. Тут речь идет о компилятореGLSL для шейдеров, код которых и формирует та самая функция.

    Программа, которая например умеет работать с ATI и NVidia драйверами одновременно, вовсе не обязательно является кроссплатформенной.

  3. CodeRush
    /#19916746 / +3

    Вот еще прекрасное из кода библиотеки zlib 1.2.11, которая используется в миллионе разных проектов:

    #define CLEAR_HASH(s)     s->head[s->hash_size-1] = NIL;     zmemzero((Bytef *)s->head, (unsigned)(s->hash_size-1)*sizeof(*s->head));
    
    ...
    
    if (s->matches == 1)
        slide_hash(s);
    else
        CLEAR_HASH(s);

    Т.к. скобки угловые забыли поставить и в макросе, и в else, то внутрь попадает только первая строка макроса, а вторая исполняется независимо от условия s->matches == 1.
    Случай там довольно редкий (поэтому оно и ехало незаметно до 2018 года), но отлаживать такое было бы весьма забавно, и когда тебе о таком вот сообщает программа — это приятно.

    • Andrey2008
      /#19916838 / +2

      Это всё от злоупотреблений макросами :). Пример прям в тему. Как раз сегодня гостевой пост "Macro Evil in C++ Code" на эту тему опубликовал. Скоро на русском на Хабр его выложу.

      • CodeRush
        /#19917246 / +3

        Ну и еще от злоупотребления опусканием скобок потому что «там одна строка всего, а тут два символа дополнительно набирать!!!». Понаступаешь на эти грабли любовно разложенные лет 5, и начинаешь ставить по три пары скобок — на всякий случай. :)

  4. Siemargl
    /#19917260 / +1

    За пятое место срабатывание «просто повезло», т.к CursorShowing могла быть и 4.

    Должна быть другая диагностика — обнаружение минимально отличающихся именами переменных/типов/классов/enum…

    Я про это писал в «Надежном программировании»

  5. VasRedDoor
    /#19918664

    Подскажите, пожалуйста, по 4 месту, может я чего-то не знаю.
    fgets возвращает строку с \n или возвращает null, если поток больше не содержит данных (в нашем случае stdin перенаправлен на файл), получается записывает в command_buf как минимум один символ (перевод строки). Т.е. strlen(command_buf) будет всегда >= 1, если fgets вернул не указатель на command_buf. Разве нет?

    • GGribkov
      /#19918784

      Если ты ничего не введешь и нажмешь Enter, то fgets считает символ переноса строки и вернет указатель на "\n\0".
      Но в примере, который демонстрируется на видео, вводится не пустая строка, а строка, которая начинается с символа '\0'. Можно подробнее почитать об этом по ссылке на оригинальную статью.

      • zhekov
        /#19921032

        Если предположить, что файл может содержать '\0', то fgets() по определению не подходит. Потому что нужно как-нибудь отличать, содержит ли последняя строка '\0', или не кончается на '\n', а для предходных строк — ручками искать, где находится '\n'. Ну а имея в виду, что строка может быть длиннее буфера — вообще сложненко получается…

        Посему, если PVS считает, что в буфером fgets() может встретится '\0' — лучше сразу выдавать диагностику, что с fgets() все очень плохо…

        • GGribkov
          /#19921556

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

          • zhekov
            /#19922426

            Да может, кто бы спорил? Но изходя из такого предположения, Studio должно сразу ругатся на fgets(), потому как понять, что конкретно Вы прочитали — не совсем легко. Ну а конечная длина — только добавляет (немного) сложности.

      • VasRedDoor
        /#19921430

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


        Похоже, код просто не предполагает то, что stdin перенаправлен на файл. А если поток перенаправлен и в нем в последней строке не окажется \n, то последний символ удалится — больше похоже на note, чем ошибку.
        Это и удивило, что такой пустяк на 4й позиции ошибок оказался, хотя это и вовсе не ошибка, если код предназначен для работы исключительно из консоли и исключительно для ввода текстовых команд. Думаю, не стоит брать в счёт то, что интерпретатор cmd умеет такую дичь делать, как на видео. Если такое брать во внимание, то вообще fgets для stdin использовать нельзя окажется.


        Полагаю, время писать статью "Как VasRedDoor и zhekov оказлись внимательнее PVS Studio и 3х с половиной человек".


        Всем спасибо за ответы!

    • zhekov
      /#19920954

      Пустая строка = конец файла, и fgets() возвратит NULL, тоесть диагностика ошибочная.

      А вот если подледняя строка файла не кончаестя на '\n', то «remove endline» урежет значащий символ. Посему и код ошибочен…

  6. pvsur
    /#19918724

    Недавно 8-е место было для меня первым :).

    Только изменение контейнера при его обходе происходило не так явно, а на 5-6-м уровне глубины обработки… Мозг сломал, пока нашел бяку…

    Ну и 10-е место надо держать в голове. Обычно я так не делаю (конструктор из конструктора), поэтому пока и не натыкался, но все-таки интересная ситуация…

    А макросы, если сложнее одной строки, это зло.

  7. tyomitch
    /#19919382

    На месте предыдущей строки кода Святослав обнаружил вот это:
    (((targ)->sv_flags & 0x00000400) && (!((targ)->sv_flags & 0x00200000) ||
    S_sv_only_taint_gmagic(targ)) ? (mg)->mg_len = ((prog->offs)[0].end),
    (mg)->mg_flags |= 0x40 : ((mg)->mg_len = (((targ)->sv_flags & 0x20000000)
    && !__builtin_expect(((((PL_curcop)->cop_hints + 0) & 0x00000008) ?
    (_Bool)1 :(_Bool)0),(0))) ? (ssize_t)Perl_utf8_length( (U8 *)(truebase),
    (U8 *)(truebase)+((prog->offs)[0].end)) : (ssize_t)((prog->offs)[0].end),
    (mg)->mg_flags &= ~0x40));

    Выглядит как Перл… Батюшки, да это ж и есть Перл!

    • RabraBabr
      /#19919928

      Я эту штуку целый час разглядываю. Надо сказать, что остальные варианты весьма банальны.

  8. VasRedDoor
    /#19920050

    Если вводится не текст, то это уже не текстовый поток, использовать fgets с ним нельзя в принципе. Ожидается текст, а анализатор предлагает костыли писать к fgets, как то странно. Если анализатор предполагает, что stdin может быть бинарный потоком, то уже предлагать менять fgets на fread надо.
    Спасибо за ответ.

  9. MSerhiy
    /#19920904

    У меня недавано пару раз проскакивала ошибка, вызов resize вместо reserve для контейнеров, как так поулчалось, ума не приложу, видимо подсказка на «r». Такая ошибка может жить довольно долго.

    std::vector<int> v;
    v.resize(some_value);
    

  10. rat1
    /#19920910

    8-е место явно какая-то невалидная проблема. В случае с foreach у меня никаких проблем не возникает, но даже в случае с итераторами, если место под vector зарезервировать, проблем возникнуть не должно. Т.е. если вектор пересоздается, по логике вещей должны быть проблемы, если нет — почему проблема должна возникнуть?

    • Cerberuser
      /#19921028

      А как на этапе написания кода гарантировать, что он не пересоздастся?

  11. adlerm
    /#19920992

    Если у вас получилось — моего уважения вам не занимать!

    «второе место...» Фигня какая-то… из меня слабый программист но на эту ошибку я сразу обратил внимание.
    хотя, может как раз потому что навиду…