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

Всем встать!

В течение целого года мы смотрели на разные ошибки из Open Source проектов на языках C и C++. Как много было обсуждений и споров, чтобы доказать невиновность каждого жучка. Настало время суда над ними!

Мы рассмотрим 10 самых интересных ошибок из проектов, которые были разобраны в отдельных статьях. А для самых увлечённых мы выбрали наиболее популярные статьи по C и C++ за этот год:

• Проверяем Blender

• Как не надо проверять размер массива в С++

• Дюжина ошибок мессенджера Telegram

• Насекомое 13 лет сидит в вашем компиляторе и не собирается оттуда вылезать

• PPSSPP или всё же psp? Смотрим баги в коде из будущего

Полный список статей нашего блога можно на сайте по ссылке.

Обвиняемые баги

Подсудимый N10

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

int sceNetAdhocMatchingSetHelloOpt(int matchingId,
                                   int optLenAddr, u32 optDataAddr)
{
  ....
  if (optLenAddr > context->hellolen)
  {
    hello = realloc(hello, optLenAddr);
  }
  ....
}

С первого взгляда в этом коде всё в порядке, и в 99% случаев программа будет работать нормально. До тех пор, пока realloc не сможет удовлетворить запрос программиста. Обратимся к документации функции realloc:

If there is not enough memory, the old memory block is not freed and null pointer is returned.

То есть, если при реалокации не хватит памяти, мы запишем в указатель hello нулевой указатель, и произойдёт утечка. Об этом и сообщает анализатор:

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'hello' is lost. Consider assigning realloc() to a temporary pointer. sceNetAdhoc.cpp 5407

Этот баг был обнаружен в проекте PPSSPP, а полную версию статьи можно найти по ссылке.

Подсудимый N9

Так, с одним разобрались, теперь следующий. В смысле он не один? А кто из них обвиняемый?

Тогда сыграем в мини-игру: надо найти опечатки в коде проекта OpenVINO. Кто нашёл, тот молодец! Кто не нашёл, тоже молодцы! Однако, надеюсь, всем стало очевидно — этот пример является одной большой причиной использовать статический анализатор.

template <class Key, class Value>
using caseless_unordered_map = std::unordered_map<Key, Value, 
                                   CaselessHash<Key>, CaselessEq<Key>>;
using TypeToNameMap = ov::intel_cpu::
                          caseless_unordered_map<std::string, Type>;

static const TypeToNameMap& get_type_to_name_tbl() {
    static const TypeToNameMap type_to_name_tbl = {
        {"Constant", Type::Input},
        {"Parameter", Type::Input},
        {"Result", Type::Output},
        {"Eye", Type::Eye},
        {"Convolution", Type::Convolution},
        {"GroupConvolution", Type::Convolution},
        {"MatMul", Type::MatMul},
        {"FullyConnected", Type::FullyConnected},
        {"MaxPool", Type::Pooling},
        {"AvgPool", Type::Pooling},
        {"AdaptiveMaxPool", Type::AdaptivePooling},
        {"AdaptiveAvgPool", Type::AdaptivePooling},
        {"Add", Type::Eltwise},
        {"IsFinite", Type::Eltwise},
        {"IsInf", Type::Eltwise},
        {"IsNaN", Type::Eltwise},
        {"Subtract", Type::Eltwise},
        {"Multiply", Type::Eltwise},
        {"Divide", Type::Eltwise},
        {"SquaredDifference", Type::Eltwise},
        {"Maximum", Type::Eltwise},
        {"Minimum", Type::Eltwise},
        {"Mod", Type::Eltwise},
        {"FloorMod", Type::Eltwise},
        {"Power", Type::Eltwise},
        {"PowerStatic", Type::Eltwise},
        {"Equal", Type::Eltwise},
        {"NotEqual", Type::Eltwise},
        {"Greater", Type::Eltwise},
        {"GreaterEqual", Type::Eltwise},
        {"Less", Type::Eltwise},
        {"LessEqual", Type::Eltwise},
        {"LogicalAnd", Type::Eltwise},
        {"LogicalOr", Type::Eltwise},
        {"LogicalXor", Type::Eltwise},
        {"LogicalNot", Type::Eltwise},
        {"Relu", Type::Eltwise},
        {"LeakyRelu", Type::Eltwise},
        {"Gelu", Type::Eltwise},
        {"Elu", Type::Eltwise},
        {"Tanh", Type::Eltwise},
        {"Sigmoid", Type::Eltwise},
        {"Abs", Type::Eltwise},
        {"Sqrt", Type::Eltwise},
        {"Clamp", Type::Eltwise},
        {"Exp", Type::Eltwise},
        {"SwishCPU", Type::Eltwise},
        {"HSwish", Type::Eltwise},
        {"Mish", Type::Eltwise},
        {"HSigmoid", Type::Eltwise},
        {"Round", Type::Eltwise},
        {"PRelu", Type::Eltwise},
        {"Erf", Type::Eltwise},
        {"SoftPlus", Type::Eltwise},
        {"SoftSign", Type::Eltwise},
        {"Select", Type::Eltwise},
        {"Log", Type::Eltwise},
        {"BitwiseAnd", Type::Eltwise},
        {"BitwiseNot", Type::Eltwise},
        {"BitwiseOr", Type::Eltwise},
        {"BitwiseXor", Type::Eltwise},
        {"Reshape", Type::Reshape},
        {"Squeeze", Type::Reshape},
        {"Unsqueeze", Type::Reshape},
        {"ShapeOf", Type::ShapeOf},
        {"NonZero", Type::NonZero},
        {"Softmax", Type::Softmax},
        {"Reorder", Type::Reorder},
        {"BatchToSpace", Type::BatchToSpace},
        {"SpaceToBatch", Type::SpaceToBatch},
        {"DepthToSpace", Type::DepthToSpace},
        {"SpaceToDepth", Type::SpaceToDepth},
        {"Roll", Type::Roll},
        {"LRN", Type::Lrn},
        {"Split", Type::Split},
        {"VariadicSplit", Type::Split},
        {"Concat", Type::Concatenation},
        {"ConvolutionBackpropData", Type::Deconvolution},
        {"GroupConvolutionBackpropData", Type::Deconvolution},
        {"StridedSlice", Type::StridedSlice},
        {"Slice", Type::StridedSlice},
        {"Tile", Type::Tile},
        {"ROIAlign", Type::ROIAlign},
        {"ROIPooling", Type::ROIPooling},
        {"PSROIPooling", Type::PSROIPooling},
        {"DeformablePSROIPooling", Type::PSROIPooling},
        {"Pad", Type::Pad},
        {"Transpose", Type::Transpose},
        {"LSTMCell", Type::RNNCell},
        {"GRUCell", Type::RNNCell},
        {"AUGRUCell", Type::RNNCell},
        {"RNNCell", Type::RNNCell},
        {"LSTMSequence", Type::RNNSeq},
        {"GRUSequence", Type::RNNSeq},
        {"AUGRUSequence", Type::RNNSeq},
        {"RNNSequence", Type::RNNSeq},
        {"FakeQuantize", Type::FakeQuantize},
        {"BinaryConvolution", Type::BinaryConvolution},
        {"DeformableConvolution", Type::DeformableConvolution},
        {"TensorIterator", Type::TensorIterator},
        {"Loop", Type::TensorIterator},
        {"ReadValue", Type::MemoryInput},  // for construction from name
                                           // ctor, arbitrary name is used
        {"Assign", Type::MemoryOutput},    // for construction from layer ctor
        {"Convert", Type::Convert},
        {"NV12toRGB", Type::ColorConvert},
        {"NV12toBGR", Type::ColorConvert},
        {"I420toRGB", Type::ColorConvert},
        {"I420toBGR", Type::ColorConvert},
        {"MVN", Type::MVN},
        {"NormalizeL2", Type::NormalizeL2},
        {"ScatterUpdate", Type::ScatterUpdate},
        {"ScatterElementsUpdate", Type::ScatterElementsUpdate},
        {"ScatterNDUpdate", Type::ScatterNDUpdate},
        {"Interpolate", Type::Interpolate},
        {"RandomUniform", Type::RandomUniform},
        {"ReduceL1", Type::Reduce},
        {"ReduceL2", Type::Reduce},
        {"ReduceLogicalAnd", Type::Reduce},
        {"ReduceLogicalOr", Type::Reduce},
        {"ReduceMax", Type::Reduce},
        {"ReduceMean", Type::Reduce},
        {"ReduceMin", Type::Reduce},
        {"ReduceProd", Type::Reduce},
        {"ReduceSum", Type::Reduce},  
        {"ReduceLogSum", Type::Reduce},
        {"ReduceLogSumExp", Type::Reduce},
        {"ReduceSumSquare", Type::Reduce},
        {"Broadcast", Type::Broadcast},  
        {"EmbeddingSegmentsSum", Type::EmbeddingSegmentsSum},
        {"EmbeddingBagPackedSum", Type::EmbeddingBagPackedSum},
        {"EmbeddingBagOffsetsSum", Type::EmbeddingBagOffsetsSum},
        {"Gather", Type::Gather},
        {"GatherElements", Type::GatherElements},
        {"GatherND", Type::GatherND},
        {"GridSample", Type::GridSample},
        {"OneHot", Type::OneHot},
        {"RegionYolo", Type::RegionYolo},
        {"ShuffleChannels", Type::ShuffleChannels},
        {"DFT", Type::DFT},
        {"IDFT", Type::DFT},
        {"RDFT", Type::RDFT},
        {"IRDFT", Type::RDFT},
        {"Abs", Type::Math},
        {"Acos", Type::Math},
        {"Acosh", Type::Math},  
        {"Asin", Type::Math},
        {"Asinh", Type::Math},
        {"Atan", Type::Math},
        {"Atanh", Type::Math},
        {"Ceil", Type::Math},
        {"Ceiling", Type::Math},
        {"Cos", Type::Math},
        {"Cosh", Type::Math},
        {"Floor", Type::Math},
        {"HardSigmoid", Type::Math},
        {"If", Type::If},
        {"Neg", Type::Math},
        {"Reciprocal", Type::Math},
        {"Selu", Type::Math},
        {"Sign", Type::Math},
        {"Sin", Type::Math},
        {"Sinh", Type::Math},
        {"SoftPlus", Type::Math},
        {"Softsign", Type::Math},
        {"Tan", Type::Math},
        {"CTCLoss", Type::CTCLoss},
        {"Bucketize", Type::Bucketize},
        {"CTCGreedyDecoder", Type::CTCGreedyDecoder},
        {"CTCGreedyDecoderSeqLen", Type::CTCGreedyDecoderSeqLen},
        {"CumSum", Type::CumSum},
        {"DetectionOutput", Type::DetectionOutput},
        {"ExperimentalDetectronDetectionOutput",
                      Type::ExperimentalDetectronDetectionOutput},
        {"LogSoftmax", Type::LogSoftmax},
        {"TopK", Type::TopK},
        {"GatherTree", Type::GatherTree},
        {"GRN", Type::GRN},
        {"Range", Type::Range},
        {"Proposal", Type::Proposal},
        {"ReorgYolo", Type::ReorgYolo},
        {"ReverseSequence", Type::ReverseSequence},
        {"ExperimentalDetectronTopKROIs", 
                      Type::ExperimentalDetectronTopKROIs},
        {"ExperimentalDetectronROIFeatureExtractor",
                      Type::ExperimentalDetectronROIFeatureExtractor},
        {"ExperimentalDetectronPriorGridGenerator",
                      Type::ExperimentalDetectronPriorGridGenerator},
        {"ExperimentalDetectronGenerateProposalsSingleImage",
                      Type::ExperimentalDetectronGenerateProposalsSingleImage},
        {"ExtractImagePatches", Type::ExtractImagePatches},
        {"GenerateProposals", Type::GenerateProposals},
        {"Inverse", Type::Inverse},
        {"NonMaxSuppression", Type::NonMaxSuppression},
        {"NonMaxSuppressionIEInternal", Type::NonMaxSuppression},
        {"NMSRotated", Type::NonMaxSuppression},
        {"MatrixNms", Type::MatrixNms},
        {"MulticlassNms", Type::MulticlassNms},
        {"MulticlassNmsIEInternal", Type::MulticlassNms},
        {"Multinomial", Type::Multinomial},
        {"Reference", Type::Reference},
        {"Subgraph", Type::Subgraph},
        {"PriorBox", Type::PriorBox},
        {"PriorBoxClustered", Type::PriorBoxClustered},
        {"Interaction", Type::Interaction},
        {"MHA", Type::MHA},
        {"Unique", Type::Unique},
        {"Ngram", Type::Ngram},
        {"ScaledDotProductAttention", Type::ScaledDotProductAttention},
        {"ScaledDotProductAttentionWithKVCache", 
                          Type::ScaledDotProductAttention},
        {"PagedAttentionExtension", Type::ScaledDotProductAttention},
        {"RoPE", Type::RoPE},
        {"GatherCompressed", Type::Gather},
        {"CausalMaskPreprocess", Type::CausalMaskPreprocess},
    };
    return type_to_name_tbl;
}

Забавно, что многие, увидев приведённый выше код, сочтут выполнение задачи по поиску ошибок в нём неразумной потерей времени. Как результат, сразу пролистают сюда, дабы посмотреть ответ.

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

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

Предупреждения анализатора:

1. V766 An item with the same key '"Abs"' has already been added. cpu_types.cpp 178

2. V766 An item with the same key '"SoftPlus"' has already been added. cpu_types.cpp 198

Если кратко и по делу, то вот они — опечатки:

static const TypeToNameMap& get_type_to_name_tbl() {
  static const TypeToNameMap type_to_name_tbl = {
    ....,
    {"Abs", Type::Eltwise},                   // <=
    ....,
    {"SoftPlus", Type::Eltwise},              // <=
    ....,
    {"Abs", Type::Math},                      // <=
    ....,
    {"SoftPlus", Type::Math},                 // <=
    ...., 
  };
  return type_to_name_tbl;
}

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

Этот баг был обнаружен в проекте OpenVINO, а полную версию статьи можно найти по ссылке.

Подсудимый N8

На это было страшно смотреть. Ладно, теперь переходим к следующ... а ты что вообще такое!? Да уж, каждый из них по-своему ужасен. Сейчас узнаем, что натворил этот жук.

Но сначала, так как случай непростой и очень запутанный, предупреждение анализатора:

V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'preferred_domain' in derived class 'HandlePositionFieldInput' and base class 'CurvesFieldInput'. node_geo_input_curve_handles.cc 95

Итак, из текста предупреждения видно, что проблема у нас в неправильном переопределении виртуальной функции в произвольных классах. Что не так? Давайте разбираться.

Начнём с базового класса:

typedef struct CurvesGeometry { .... };

namespace bke
{
  ....
  class CurvesGeometry : public ::CurvesGeometry { .... };

  class CurvesFieldInput : public fn::FieldInput 
  {
    ....
    virtual std::optional<AttrDomain> preferred_domain(
      const CurvesGeometry &curves) const;
  };
  ....
}

Виртуальная функция preferred_domain принимает параметр типа bke::CurvesGeometry. Запомним.

Теперь посмотрим на наследника:

namespace blender::nodes::node_geo_input_curve_handles_cc
{
  class HandlePositionFieldInput final : public bke::CurvesFieldInput 
  {
    ....
    std::optional<AttrDomain> preferred_domain(
      const CurvesGeometry & /*curves*/) const;
  };
}

Нашли проблему? Если нет, то не расстраивайтесь, я тоже сначала не понял :). Будем разбираться.

В базовом классе виртуальная функция принимает параметр с неквалифицированным именем CurvesGeometry. Когда компилятор будет осуществлять поиск этого типа, он начнёт с области видимости класса CurvesFieldInput и будет заглядывать во все обрамляющие области видимости, пока не встретит этот тип. В итоге будет найден тип bke::CurvesGeometry.

Теперь посмотрим на производные классы. Они определены в пространстве имён, отличном от того, где располагается базовый класс. Компилятор также начнёт поиск нужного имени CurvesGeometry, не встретит его в обрамляющих областях видимости и дойдёт до глобального. А в глобальном пространстве имён тоже есть CurvesGeometry, только не тот, что нам нужен для переопределения функции :)

Для исправления надо всего лишь указать квалифицированное имя типа. Ну и воспользуемся возможностями C++11 (override), защитив будущие поколения от ошибок:

namespace blender::nodes::node_geo_input_curve_handles_cc
{
  class HandlePositionFieldInput final : public bke::CurvesFieldInput 
  {
    ....
    std::optional<AttrDomain> preferred_domain(
      const bke::CurvesGeometry & /*curves*/) const override;
  };
}

Этот баг был обнаружен в проекте Blender, а полную версию статьи можно найти по ссылке.

Подсудимый N7

Номер семь, ты что там прячешь? Ну-ка показывай, что у тебя за спиной. Вот он, воришка. Решил забрать сладкий кусок кода себе. Ну уж нет, мы сейчас тебя выведем на чистую воду.

Честно, очень хотелось вставить сюда полный код этой функции — почти 400 строчек, — чтобы было интереснее искать ошибку. Но поберегу ваше колёсико мыши и оставлю только самую интересную часть.

Файл symbolgroupvalue.cpp:1196

static KnownType knownClassTypeHelper(const std::string &type,
                                      std::string::size_type pos,
                                      std::string::size_type endPos)
{
  ....
  // Remaining non-template types
  switch (endPos - qPos)
  {
    ....
  case 30:
    if (!type.compare(qPos, 30, "QPatternist::SequenceType::Ptr"))
      return KT_QPatternist_SequenceType_Ptr;
    if (!type.compare(qPos, 30, "QXmlStreamNamespaceDeclaration"))
      return KT_QXmlStreamNamespaceDeclaration;
    break;
  case 32:
    break;                                                                // <=
    if (!type.compare(qPos, 32, "QPatternist::Item::Iterator::Ptr"))
      return KT_QPatternist_Item_Iterator_Ptr;
  case 34:
    break;                                                                // <=
    if (!type.compare(qPos, 34, "QPatternist::ItemSequenceCacheCell"))
      return KT_QPatternist_ItemSequenceCacheCell;
  case 37:
    break;                                                                // <=
    if (!type.compare(qPos, 37, "QNetworkHeadersPrivate::RawHeaderPair"))
      return KT_QNetworkHeadersPrivate_RawHeaderPair;
    if (!type.compare(qPos, 37, "QPatternist::AccelTree::BasicNodeData"))
      return KT_QPatternist_AccelTree_BasicNodeData;
    break;
  }

  return KT_Unknown;
}

Прошу обратить внимание на ветки 32, 34 и 37 показанной выше инструкции switch. Честно, я не придумал какого-либо обоснования для использования break; прямо перед исполняемым кодом.

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

Анализатор предлагает обратить внимание на этот фрагмент кода при помощи предупреждения:

V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. symbolgroupvalue.cpp 1565

Этот баг был обнаружен в проекте Qt Creator, а полную версию статьи можно найти по ссылке.

Для удобного просмотра результатов анализа можно использовать расширение (плагин) PVS-Studio для Qt Creator. Подробнее про его установку и использование написано в документации "Использование расширения PVS-Studio для Qt Creator".

Подсудимый N6

Уха-ха-ха! Ошибки copy-paste, мы вас ждали. Такие жуки попадаются почти каждый код, а найти их не всегда легко. Но теперь один из них предстал перед судом. Покажи же, где ты скрывался!

void WebViewInstance::show(const QString &url, uint64 queryId) 
{
  ....
  const auto allowClipboardRead = v::is<WebViewSourceAttachMenu>(_source)
       || v::is<WebViewSourceAttachMenu>(_source)
       || (attached != end(bots)
         && (attached->inAttachMenu || attached->inMainMenu));
  ....
}

Предупреждение анализатора:

V501 There are identical sub-expressions 'v::is<WebViewSourceAttachMenu > (_source)' to the left and to the right of the '||' operator. bot_attach_web_view.cpp 1129

Из текста предупреждения нам становится очевидно, что в условии проверяются одни и те же выражения v::is<WebViewSourceAttachMenu>(_source). Возможно, одно из них необходимо заменить на что-то другое, но на что?..

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

(attached->inAttachMenu || attached->inMainMenu)

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

const auto allowClipboardRead = 
      v::is<WebViewSourceMainMenu>(_source)
   || v::is<WebViewSourceAttachMenu>(_source)
   ....

Этот баг был обнаружен в проекте Telegram, а полную версию статьи можно найти по ссылке.

Подсудимый N5

Уже половина преступников показали нам свои усики. Но сейчас начнётся настоящая жара. Да-да, всех провинившихся нужно выжигать в последующих релизах. И особенно тех, на кого мы посмотрим дальше.

Вот и номер пять: такой маленький жучок, но что же он творит! Такому точно нужно сесть за решётку.

const qdGameObjectStateWalk* qdGameObjectMoving::current_walk_state() const
{
  const qdGameObjectState* st = get_cur_state();
  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
    st = last_walk_state_;
    if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
      st = get_default_state();
else
    st = get_default_state();
    if(!st) st = get_state(0);
#endif
  }
  ....
}

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

Анализатор обращает внимание, что, независимо от условия, будет выполнено одно и то же действие:

  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
    st = get_default_state();
else
  st = get_default_state();

Обратите внимание, что код как-то странно отформатирован. Я имею в виду, что ключевое слово else находится в начале строки.

Если посмотреть на код рядом и немного подумать, то становится ясно, что на самом деле хотели написать не оператор else, а директиву препроцессора #else. Однако программист опечатался и забыл символ решётки (#).

Исправленный код:

const qdGameObjectState* st = get_cur_state();
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
  st = last_walk_state_;
  if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
    st = get_default_state();
#else
  st = get_default_state();
  if(!st) st = get_state(0);
#endif

Этот баг был обнаружен в проекте qdEngine, а полную версию статьи можно найти по ссылке.

Подсудимый N4

Так, а где четвёртый, его кто-нибудь видит? Куда же пропал этот жук? Ладно, покажите место преступления, сейчас найдём.

static const char *script_list[][2] = {
  ....
  { "Myanmar / Burmese", "Mymr" },
  { "​Nag Mundari", "Nagm" },
  { "Nandinagari", "Nand" },
  ....
}

Читатель может задаться вопросом: "И что же здесь не так?" Мы бы и сами не поняли, если бы не срабатывание диагностического правила V1076. Что интересно, это первое выписанное нами срабатывание. Диагностическое правило проверяет текст программы на наличие невидимых символов. Такие символы — это своего рода закладки, которые программист может не видеть из-за настроек отображения текста в среде разработки, зато компилятор прекрасно их видит и обрабатывает.

Предупреждение анализатора:

V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. locales.h 1114

Давайте внимательно посмотрим на следующую строку:

{ "​Nag Mundari", "Nagm" },

Именно в ней содержится закладка с невидимым символом. Если воспользоваться hex-редактором, то можно заметить следующее:

Между двойной кавычкой и символом N затесались 3 байта: E2, 80 и 8B. Они соответствуют Unicode-символу ZERO WIDTH SPACE (U+200B).

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

Этот баг был обнаружен в проекте Godot Engine, а полную версию статьи можно найти по ссылке.

Подсудимый N3

Тройка лидеров этого года. Кто же всё-таки из вас самый опасный преступник?

Жук номер три, ты типа умным прикидываешься? Высчитываешь сложные уравнения, от которых зависят важные переменные? Признайся, это ты всё сломал.

void StfsContainerDevice::BlockToOffsetSVOD(size_t block, ....)
{
  ....
  const size_t BLOCK_SIZE = 0x800;
  const size_t HASH_BLOCK_SIZE = 0x1000;
  const size_t BLOCKS_PER_L0_HASH = 0x198;
  const size_t HASHES_PER_L1_HASH = 0xA1C4;
  const size_t BLOCKS_PER_FILE = 0x14388;
  const size_t MAX_FILE_SIZE = 0xA290000;
  const size_t BLOCK_OFFSET =
      header_.metadata.volume_descriptor.svod.start_data_block();
  ....

  // Resolve the true block address and file index
  size_t true_block = block - (BLOCK_OFFSET * 2);
  ....
  size_t file_block = true_block % BLOCKS_PER_FILE;
  size_t file_index = true_block / BLOCKS_PER_FILE;
  size_t offset = 0;

  // Calculate offset caused by Level0 Hash Tables
  size_t level0_table_count = (file_block / BLOCKS_PER_L0_HASH) + 1;
  offset += level0_table_count * HASH_BLOCK_SIZE;

  // Calculate offset caused by Level1 Hash Tables
  size_t level1_table_count = (level0_table_count / HASHES_PER_L1_HASH) + 1;
  offset += level1_table_count * HASH_BLOCK_SIZE;
  ....
}

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

V1064 The 'level0_table_count' operand of integer division is less than the 'HASHES_PER_L1_HASH' one. The result will always be zero. stfs_container_device.cc 500

Анализатор выдаёт предупреждение, что значение level1_table_count будет всегда равно 0, т.к. при целочисленном делении левый операнд level0_table_count меньше, чем правый HASHES_PER_L1_HASH. Значение последнего равняется 41412, а чтобы узнать значение первого, поднимемся чуть выше.

Переменная file_block вычисляется через остаток от деления переменной true_block на BLOCKS_PER_FILE и поэтому лежит в диапазоне [0 .. 82823].

Переменная BLOCKS_PER_L0_HASH делит это значение на 408, и затем к результату прибавляется 1. При наибольшем значении file_block в результате деления получится 202, поэтому значение переменной level0_table_count будет лежать в диапазоне [1 .. 203].

Переменная level1_table_count по итогу будет вычисляться как 203/41412+1, и при любых значениях переменной true_block будет равна 1.

Может, мы где-то ошиблись? Оказывается, что нет, так думает не только наш анализатор.

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

В коде есть комментарий, который, возможно, сможет помочь в решении этой загадки. Может, у вас уже есть какие-то идеи?

Этот баг был обнаружен в проекте Xenia, а полную версию статьи можно найти по ссылке.

Подсудимый N2

Это что? Разыменование нулевого указателя? Каждый год одно и то же... Ни одна проверка проекта не обходится без твоего участия. Твои крохотные лапки были почти в каждом коде. Жди суровое наказание. А пока покажите место преступления!

Для этого рассмотрим следующую структуру:

struct ForceCodegenLinking {
    ForceCodegenLinking() {
      // We must reference the passes in such a way that compilers will not
      // delete it all as dead code, even with whole program optimization,
      // yet is effectively a NO-OP. As the compiler isn't smart enough
      // to know that getenv() never returns -1, this will do the job.
      // This is so that globals in the translation units where these functions
      // are defined are forced to be initialized, populating various
      // registries.
      if (std::getenv("bar") != (char*) -1)
        return;

      (void) llvm::createFastRegisterAllocator();
      (void) llvm::createBasicRegisterAllocator();
      (void) llvm::createGreedyRegisterAllocator();
      (void) llvm::createDefaultPBQPRegisterAllocator();

      (void)llvm::createBURRListDAGScheduler(nullptr,
                                             llvm::CodeGenOptLevel::Default);
      (void)llvm::createSourceListDAGScheduler(nullptr,
                                               llvm::CodeGenOptLevel::Default);
      (void)llvm::createHybridListDAGScheduler(nullptr,
                                               llvm::CodeGenOptLevel::Default);
      (void)llvm::createFastDAGScheduler(nullptr,
                                         llvm::CodeGenOptLevel::Default);
      (void)llvm::createDefaultScheduler(nullptr,
                                         llvm::CodeGenOptLevel::Default);
      (void)llvm::createVLIWDAGScheduler(nullptr,
                                         llvm::CodeGenOptLevel::Default);
    }
  } ForceCodegenLinking; // Force link by creating a global definition.
}

В её конструкторе вызывается ряд функций по созданию неких сущностей. Нас интересуют только функции, которым передаются аргументы (их шесть штук). Вот, например, некоторые из них:

ScheduleDAGSDNodes *llvm::createBURRListDAGScheduler(SelectionDAGISel *IS,
                                                    CodeGenOptLevel OptLevel)
{
  const TargetSubtargetInfo &STI = IS->MF->getSubtarget();
  ....
}

Или

ScheduleDAGSDNodes* createDefaultScheduler(SelectionDAGISel *IS,
                                           CodeGenOpt::Level OptLevel) 
{
  const TargetLowering *TLI = IS->TLI;
  const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
  ....
}

Если мы посмотрим на вызов функции в конструкторе, то увидим, что в качестве первого аргумента ей передаётся nullptr. А это как раз первый параметр IS, который в первой же строке функции разыменовывается.

Странный код. Возможно, разработчики LLVM действительно что-то знают и научились управлять UB.

В каждой такой функции с нулевым первым аргументом происходит его разыменование.

Соответственно, срабатывания анализатора:

• V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createBURRListDAGScheduler' function. Inspect the first argument. Check lines: 'ScheduleDAGRRList.cpp:3147', 'LinkAllCodegenComponents.h:40'.

• V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createSourceListDAGScheduler' function. Inspect the first argument. Check lines: 'ScheduleDAGRRList.cpp:3161', 'LinkAllCodegenComponents.h:42'.

• V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createHybridListDAGScheduler' function. Inspect the first argument. Check lines: 'ScheduleDAGRRList.cpp:3175', 'LinkAllCodegenComponents.h:44'.

• ... (остальные три аналогичные)

Этот баг был обнаружен в проекте LLVM, а полную версию статьи можно найти по ссылке.

Подсудимый N1

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

Для этого сначала изучите список именованных констант:

enum dbg_status {
  DBG_STATUS_OK,
  DBG_STATUS_APP_VERSION_NOT_SET,
  DBG_STATUS_UNSUPPORTED_APP_VERSION,
  DBG_STATUS_DBG_BLOCK_NOT_RESET,
  DBG_STATUS_INVALID_ARGS,
  DBG_STATUS_OUTPUT_ALREADY_SET,
  DBG_STATUS_INVALID_PCI_BUF_SIZE,
  DBG_STATUS_PCI_BUF_ALLOC_FAILED,
  DBG_STATUS_PCI_BUF_NOT_ALLOCATED,
  DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS,
  DBG_STATUS_NO_MATCHING_FRAMING_MODE,
  DBG_STATUS_VFC_READ_ERROR,
  DBG_STATUS_STORM_ALREADY_ENABLED,
  DBG_STATUS_STORM_NOT_ENABLED,
  DBG_STATUS_BLOCK_ALREADY_ENABLED,
  DBG_STATUS_BLOCK_NOT_ENABLED,
  DBG_STATUS_NO_INPUT_ENABLED,
  DBG_STATUS_NO_FILTER_TRIGGER_256B,
  DBG_STATUS_FILTER_ALREADY_ENABLED,
  DBG_STATUS_TRIGGER_ALREADY_ENABLED,
  DBG_STATUS_TRIGGER_NOT_ENABLED,
  DBG_STATUS_CANT_ADD_CONSTRAINT,
  DBG_STATUS_TOO_MANY_TRIGGER_STATES,
  DBG_STATUS_TOO_MANY_CONSTRAINTS,
  DBG_STATUS_RECORDING_NOT_STARTED,
  DBG_STATUS_DATA_DIDNT_TRIGGER,
  DBG_STATUS_NO_DATA_RECORDED,
  DBG_STATUS_DUMP_BUF_TOO_SMALL,
  DBG_STATUS_DUMP_NOT_CHUNK_ALIGNED,
  DBG_STATUS_UNKNOWN_CHIP,
  DBG_STATUS_VIRT_MEM_ALLOC_FAILED,
  DBG_STATUS_BLOCK_IN_RESET,
  DBG_STATUS_INVALID_TRACE_SIGNATURE,
  DBG_STATUS_INVALID_NVRAM_BUNDLE,
  DBG_STATUS_NVRAM_GET_IMAGE_FAILED,
  DBG_STATUS_NON_ALIGNED_NVRAM_IMAGE,
  DBG_STATUS_NVRAM_READ_FAILED,
  DBG_STATUS_IDLE_CHK_PARSE_FAILED,
  DBG_STATUS_MCP_TRACE_BAD_DATA,
  DBG_STATUS_MCP_TRACE_NO_META,
  DBG_STATUS_MCP_COULD_NOT_HALT,
  DBG_STATUS_MCP_COULD_NOT_RESUME,
  DBG_STATUS_RESERVED0,
  DBG_STATUS_SEMI_FIFO_NOT_EMPTY,
  DBG_STATUS_IGU_FIFO_BAD_DATA,
  DBG_STATUS_MCP_COULD_NOT_MASK_PRTY,
  DBG_STATUS_FW_ASSERTS_PARSE_FAILED,
  DBG_STATUS_REG_FIFO_BAD_DATA,
  DBG_STATUS_PROTECTION_OVERRIDE_BAD_DATA,
  DBG_STATUS_DBG_ARRAY_NOT_SET,
  DBG_STATUS_RESERVED1,
  DBG_STATUS_NON_MATCHING_LINES,
  DBG_STATUS_INSUFFICIENT_HW_IDS,
  DBG_STATUS_DBG_BUS_IN_USE,
  DBG_STATUS_INVALID_STORM_DBG_MODE,
  DBG_STATUS_OTHER_ENGINE_BB_ONLY,
  DBG_STATUS_FILTER_SINGLE_HW_ID,
  DBG_STATUS_TRIGGER_SINGLE_HW_ID,
  DBG_STATUS_MISSING_TRIGGER_STATE_STORM,
  MAX_DBG_STATUS
};

Теперь изучите массив строк:

static const char * const s_status_str[] = {
  /* DBG_STATUS_OK */
  "Operation completed successfully",

  /* DBG_STATUS_APP_VERSION_NOT_SET */
  "Debug application version wasn't set",

  /* DBG_STATUS_UNSUPPORTED_APP_VERSION */
  "Unsupported debug application version",

  /* DBG_STATUS_DBG_BLOCK_NOT_RESET */
  "The debug block wasn't reset since the last recording",

  /* DBG_STATUS_INVALID_ARGS */
  "Invalid arguments",

  /* DBG_STATUS_OUTPUT_ALREADY_SET */
  "The debug output was already set",

  /* DBG_STATUS_INVALID_PCI_BUF_SIZE */
  "Invalid PCI buffer size",

  /* DBG_STATUS_PCI_BUF_ALLOC_FAILED */
  "PCI buffer allocation failed",

  /* DBG_STATUS_PCI_BUF_NOT_ALLOCATED */
  "A PCI buffer wasn't allocated",

  /* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */
  "The filter/trigger constraint dword offsets are not "
  "enabled for recording",


  /* DBG_STATUS_VFC_READ_ERROR */
  "Error reading from VFC",

  /* DBG_STATUS_STORM_ALREADY_ENABLED */
  "The Storm was already enabled",

  /* DBG_STATUS_STORM_NOT_ENABLED */
  "The specified Storm wasn't enabled",

  /* DBG_STATUS_BLOCK_ALREADY_ENABLED */
  "The block was already enabled",

  /* DBG_STATUS_BLOCK_NOT_ENABLED */
  "The specified block wasn't enabled",

  /* DBG_STATUS_NO_INPUT_ENABLED */
  "No input was enabled for recording",

  /* DBG_STATUS_NO_FILTER_TRIGGER_256B */
  "Filters and triggers are not allowed in E4 256-bit mode",

  /* DBG_STATUS_FILTER_ALREADY_ENABLED */
  "The filter was already enabled",

  /* DBG_STATUS_TRIGGER_ALREADY_ENABLED */
  "The trigger was already enabled",

  /* DBG_STATUS_TRIGGER_NOT_ENABLED */
  "The trigger wasn't enabled",

  /* DBG_STATUS_CANT_ADD_CONSTRAINT */
  "A constraint can be added only after a filter was "
  "enabled or a trigger state was added",

  /* DBG_STATUS_TOO_MANY_TRIGGER_STATES */
  "Cannot add more than 3 trigger states",

  /* DBG_STATUS_TOO_MANY_CONSTRAINTS */
  "Cannot add more than 4 constraints per filter or trigger state",

  /* DBG_STATUS_RECORDING_NOT_STARTED */
  "The recording wasn't started",

  /* DBG_STATUS_DATA_DID_NOT_TRIGGER */
  "A trigger was configured, but it didn't trigger",

  /* DBG_STATUS_NO_DATA_RECORDED */
  "No data was recorded",

  /* DBG_STATUS_DUMP_BUF_TOO_SMALL */
  "Dump buffer is too small",

  /* DBG_STATUS_DUMP_NOT_CHUNK_ALIGNED */
  "Dumped data is not aligned to chunks",

  /* DBG_STATUS_UNKNOWN_CHIP */
  "Unknown chip",

  /* DBG_STATUS_VIRT_MEM_ALLOC_FAILED */
  "Failed allocating virtual memory",

  /* DBG_STATUS_BLOCK_IN_RESET */
  "The input block is in reset",

  /* DBG_STATUS_INVALID_TRACE_SIGNATURE */
  "Invalid MCP trace signature found in NVRAM",

  /* DBG_STATUS_INVALID_NVRAM_BUNDLE */
  "Invalid bundle ID found in NVRAM",

  /* DBG_STATUS_NVRAM_GET_IMAGE_FAILED */
  "Failed getting NVRAM image",

  /* DBG_STATUS_NON_ALIGNED_NVRAM_IMAGE */
  "NVRAM image is not dword-aligned",

  /* DBG_STATUS_NVRAM_READ_FAILED */
  "Failed reading from NVRAM",

  /* DBG_STATUS_IDLE_CHK_PARSE_FAILED */
  "Idle check parsing failed",

  /* DBG_STATUS_MCP_TRACE_BAD_DATA */
  "MCP Trace data is corrupt",

  /* DBG_STATUS_MCP_TRACE_NO_META */
  "Dump doesn't contain meta data - it must be provided in image file",

  /* DBG_STATUS_MCP_COULD_NOT_HALT */
  "Failed to halt MCP",

  /* DBG_STATUS_MCP_COULD_NOT_RESUME */
  "Failed to resume MCP after halt",

  /* DBG_STATUS_RESERVED0 */
  "",

  /* DBG_STATUS_SEMI_FIFO_NOT_EMPTY */
  "Failed to empty SEMI sync FIFO",

  /* DBG_STATUS_IGU_FIFO_BAD_DATA */
  "IGU FIFO data is corrupt",

  /* DBG_STATUS_MCP_COULD_NOT_MASK_PRTY */
  "MCP failed to mask parities",

  /* DBG_STATUS_FW_ASSERTS_PARSE_FAILED */
  "FW Asserts parsing failed",

  /* DBG_STATUS_REG_FIFO_BAD_DATA */
  "GRC FIFO data is corrupt",

  /* DBG_STATUS_PROTECTION_OVERRIDE_BAD_DATA */
  "Protection Override data is corrupt",

  /* DBG_STATUS_DBG_ARRAY_NOT_SET */
  "Debug arrays were not set "
  "(when using binary files, dbg_set_bin_ptr must be called)",

  /* DBG_STATUS_RESERVED1 */
  "",

  /* DBG_STATUS_NON_MATCHING_LINES */
  "Non-matching debug lines - in E4, all lines must be of "
  "the same type (either 128b or 256b)",

  /* DBG_STATUS_INSUFFICIENT_HW_IDS */
  "Insufficient HW IDs. Try to record less Storms/blocks",

  /* DBG_STATUS_DBG_BUS_IN_USE */
  "The debug bus is in use",

  /* DBG_STATUS_INVALID_STORM_DBG_MODE */
  "The storm debug mode is not supported in the current chip",

  /* DBG_STATUS_OTHER_ENGINE_BB_ONLY */
  "Other engine is supported only in BB",

  /* DBG_STATUS_FILTER_SINGLE_HW_ID */
  "The configured filter mode requires a single Storm/block input",

  /* DBG_STATUS_TRIGGER_SINGLE_HW_ID */
  "The configured filter mode requires that all the constraints of a "
  "single trigger state will be defined on a single Storm/block input",

  /* DBG_STATUS_MISSING_TRIGGER_STATE_STORM */
  "When triggering on Storm data, the Storm to trigger on must be specified"
};

Последнее — код, который превращает именованную константу в строчку:

const char *qed_dbg_get_status_str(enum dbg_status status)
{
  return (status < MAX_DBG_STATUS) ?
    s_status_str[status] : "Invalid debug status";
}

Где ошибка?

Нашли?

На самом деле есть намёк, за который можно теоретически зацепиться. В массиве все строки отделяются одной пустой строкой, кроме одного места. Но начнём мы с предупреждения анализатора кода PVS-Studio:

V557 Array overrun is possible. The value of 'status' index could reach 58. qede_debug.c 7149

Оно указывает на место, где из массива по индексу извлекается указатель на строку:

return (status < MAX_DBG_STATUS) ?
  s_status_str[status] : "Invalid debug status";

Вначале я поставил неправильный скучный диагноз. По невнимательности я подумал, что передо мной off-by-one error. Уж очень это похоже на типовую неправильную проверку, которую я встречал много раз в разных проектах.

Суть классического антипаттерна. Есть enum, последний элемент которого используется как количество элементов в нём.

enum Efoo {
  A,
  B,
  C,
  COUNT
};

Константам в enum присваиваются значения, начиная с 0. Соответственно, вспомогательная константа COUNT окажется равна 3, что соответствует количеству основных именованных констант.

Есть какой-то массив, где каждой именованной константе что-то соответствует:

char Cfoo[] = { 'A', 'B', 'C' };

Часто допускают баг, когда проверяют, что индекс не выходит за границу массива:

char Convert(unsigned id)
{
  return (id > COUNT) ? 0 : Cfoo[id];
}

Проверка работает неправильно: если id окажется равен COUNT, то произойдёт выход за границу массива. Для таких ошибок анализатор PVS-Studio выдаёт предупреждение аналогично рассмотренному ранее.

Правильным будет один из следующих вариантов:

return (id >= COUNT) ? 0 : Cfoo[id];  // OK
return (id < COUNT) ? Cfoo[id] : 0;   // OK

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

И тут в последний момент — СТОП!

ЗДЕСЬ ЖЕ ВЕДЬ ПРАВИЛЬНАЯ ПРОВЕРКА НАПИСАНА!

return (status < MAX_DBG_STATUS) ?
  s_status_str[status] : "Invalid debug status";

Теперь непонятно и интересно! Почему анализатор выдал предупреждение? Ложное срабатывание? Это вряд ли, тут ему негде запутаться.

Засучив рукава, я начинаю внимательно изучать код. Вот оно!

Пропущена строка для константы DBG_STATUS_NO_MATCHING_FRAMING_MODE.

В enum:

DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS,
DBG_STATUS_NO_MATCHING_FRAMING_MODE,
DBG_STATUS_VFC_READ_ERROR,

В массиве:

/* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */
"The filter/trigger constraint dword offsets are not "
"enabled for recording",


/* DBG_STATUS_VFC_READ_ERROR */
"Error reading from VFC",

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

Впрочем, это сейчас, когда найден баг, две пустые строчку выглядят подозрительно. При обзоре никто не обратит на них внимание. А даже случайно заметив, просто удалят одну из них для красоты. Никто не скажет, "да тут небось что-то пропустили, пойдём проверим" :)

В результате этой ошибки:

• возможен выход за границу массива;

• функция возвращает неправильные строки для всех констант, начиная с DBG_STATUS_NO_MATCHING_FRAMING_MODE.

Этот баг был обнаружен в проекте DPDK, а полную версию статьи можно найти по ссылке.

Заключение

Тишина в зале. И даже суд молчит. Самые опасные преступники этого года наконец-то были найдены. Остаётся только полностью уничтожить их, но это уже совсем другая история... Однако мы бы не смогли найти эти ошибки, если бы нам не помог детектив — анализатор PVS-Studio.

За 2024 год мы рассмотрели немало багов, но проверить все проекты мы не можем. Наш детектив готов помочь вам найти опасные участки кода в вашем проекте:

• для проектов с открытым исходным кодом;

• для образовательных целей;

• 30-дневная пробная версия.

А если вы хотите знать обо всех самых интересных багах за этот год, то предлагаю посмотреть на топ-10 ошибок из проектов, написанных на Java и C#:

• 10 самых интересных ошибок в Java проектах за 2024 год;

• Топ-10 ошибок, найденных в C# проектах за 2024 год.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Aleksandra Uvarova. Court is in session: Top 10 most notorious C and C++ errors in 2024.