Every year, we witness the same drama: bugs wreak havoc on our code as if asserting their dominance. But today, the tide turns—it's time for judgment. Let's dive into the most intriguing bugs we've uncovered this year.
Over the past twelve months, our team has scrutinized countless errors from open-source C and C++ projects. Heated debates and discussions unfolded in comments to defend each bug's innocence. Now, it's time for reckoning!
We'll delve into 10 of the most notorious bugs from these projects that we've covered in the articles. For eager fans of C and C++, we've picked the most popular C and C++ articles for this year:
The full list is at the link.
Defendant N10
To keep the suspense alive and not reveal the most dangerous villain right away, we'll start from the bottom of the list. Bring out the tenth offender! Ah yes, of course—it's an infamous memory leak. Let's show where it's hiding:
int sceNetAdhocMatchingSetHelloOpt(int matchingId,
int optLenAddr, u32 optDataAddr)
{
....
if (optLenAddr > context->hellolen)
{
hello = realloc(hello, optLenAddr);
}
....
}At first glance, the code is fine, and the program works correctly 99% of the time. Until realloc can't fulfill the programmer's request. Let's take a look at the documentation for the realloc function:
If there is not enough memory, the old memory block is not freed and null pointer is returned.
So, if there's not enough memory during reallocation, we'll write a null pointer to the hello pointer, and there'll be a leak. This is what the analyzer reports:
The PVS-Studio warning: 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
We found this bug in the PPSSPP project. The full article is at the link.
Defendant N9
Alright, we've dealt with that perp—now it's time for the next one. Wait, it's not just one? Two? How many? Who are the real offenders here?
Now let's play a mini-game: find typos in the OpenVINO project code. Whoever found them deserve kudos! Whoever didn't find the typos deserve kudos, too! However, we hope it's obvious to everyone. Here's a great example of why developers need a static analyzer.
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;
}It's amusing that many devs think it's pointless to look for errors in such code. So, they just scroll down to see an answer right away.
There's also an irony in how, despite weighing up inefficiency and labor costs, developers still monotonously search for errors without using static analyzers.
It's so simple: take the analyzer, run the analysis, look at the list of errors. Profit. No need to waste time looking for bugs—they're already highlighted. Just fix them and that's it.
The PVS-Studio warnings:
Here are typos:
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;
}Obviously, there should not be identical values, and duplicates will never be used.
We found this bug in the OpenVINO project. The full article is at the link.
Defendant N8
It was painful to look at, yeah? Alright, let's move on to the next... Wait, what is this!? Holy molly... Each of bug is awful in its own way. Now, look at this naughty one.
Since this case is tricky and very confusing, take a peek at the warning:
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
So, we can see the issue lies in incorrect overriding of a virtual function in arbitrary classes. What exactly is wrong? Let's find out.
Let's start with the base class:
typedef struct CurvesGeometry { .... };
namespace bke
{
....
class CurvesGeometry : public ::CurvesGeometry { .... };
class CurvesFieldInput : public fn::FieldInput
{
....
virtual std::optional<AttrDomain> preferred_domain(
const CurvesGeometry &curves) const;
};
....
}The preferred_domain virtual function takes a parameter of the bke::CurvesGeometry type. We'll keep that in mind.
Now, take a look at the child:
namespace blender::nodes::node_geo_input_curve_handles_cc
{
class HandlePositionFieldInput final : public bke::CurvesFieldInput
{
....
std::optional<AttrDomain> preferred_domain(
const CurvesGeometry & /*curves*/) const;
};
}Found an issue? If not, don't worry, we didn't understand it at first either :)
In the base class, the virtual function accepts a parameter with an unqualified name, CurvesGeometry. When the compiler searches for this type, it starts with the scope of the CurvesFieldInput class and looks in all framed scopes until it finds the type. As a result, the bke::CurvesGeometry type is found.
Now let's look at the derived classes. They're defined in a different namespace from the one in which the base class is located. The compiler also starts searching for the needed CurvesGeometry name, doesn't find it in the framed scopes, and reaches the global one. Yet, the global namespace also has CurvesGeometry, just not the one we need to override the function :)
We can fix it by specifying a qualified type name—just use the C++11 (override) capabilities and protect future generations from errors:
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;
};
}We found this bug in the Blender project. The full article is at the link.
Defendant N7
The seventh defendant, what are you draggin' behind your back? Show us quickly! Ah! A little thief! Decided to pocket a juicy chunk of code, huh? No chance, we're bringing this to the light.
Honestly, we really wanted to paste the full code of the function here—which has almost 400 code lines—to make searching for errors more interesting. However, we don't want to abuse your mouse wheel, so we've included just the most intriguing part.
The file: 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;
}Please note the 32, 34, and 37 branches of the switch statement above. Honestly, we can't think of any reason for using break; right before the executable code.
We thought that they were just temporary kludges. But no, they were added at the same time as this code. According to the history, this code is about 14 years old. If any readers can suggest what exactly is going on here—you're welcome in the comments.
The analyzer points you to this code fragment with the warning:
V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. symbolgroupvalue.cpp 1565
We found this bug in the Qt Creator project. The full article is at the link.
To easily view the analysis results, you can use the PVS-Studio extension (plugin) for Qt Creator. For more details on its installation and usage, refer to the documentation: Using the PVS-Studio extension for Qt Creator.
Defendant N6
Ha-ha-ha-ha! Copy-paste errors, we've been expecting you! These bugs lurk in almost every codebase, and they're not always easy to spot. Now, one of them is on our trial. Step forward and reveal itself!
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));
....
}The analyzer warning:
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
The same v::is<WebViewSourceAttachMenu>(_source) expressions are checked in the condition. Developers may need to replace one of them with something else—but with what?
Take a look at the last condition line and see how the values of the following expressions are checked:
(attached->inAttachMenu || attached->inMainMenu)Let's consider the variable names and identical expressions that the analyzer addresses. Perhaps the developers should fix the condition as follows:
const auto allowClipboardRead =
v::is<WebViewSourceMainMenu>(_source)
|| v::is<WebViewSourceAttachMenu>(_source)
....We found this bug in the Telegram project. The full article is at the link.
Defendant N5
Halfway through, already we've seen plenty of the perps' antennae. There's danger in the courtroom air. Yes, all the guilty buggies should be eradicated in future releases. Especially those we'll look at next.
Here comes the fifth defendant: a seemingly tiny bug, but what the damage it can cause! This one definitely deserves a spot behind bars.
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
}
....
}The PVS-Studio warning: V523 The 'then' statement is equivalent to the 'else' statement. qd_game_object_moving.cpp 2781
The analyzer points out that the same action is performed regardless of the condition:
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
st = get_default_state();
else
st = get_default_state();Note that the code is formatted in a strange way. The else keyword is at the beginning of the line.
If we look at the code, we can see that the developers wanted to write the #else preprocessor directive but not the else statement. However, they made a typo and forgot to write the hash symbol (#).
The fixed code:
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);
#endifWe found this bug in the qdEngine project. The full article is at the link.
Defendant N4
Alright, where's the fourth one, does anyone see it? Where has that bug vanished to? Alright, show us the crime scene—we'll find this villain.
static const char *script_list[][2] = {
....
{ "Myanmar / Burmese", "Mymr" },
{ "Nag Mundari", "Nagm" },
{ "Nandinagari", "Nand" },
....
}The reader may ask, "What's wrong here?" We wouldn't have figured it out ourselves if it weren't for the V1076 diagnostic rule. Interestingly enough, this is the first warning we've written out. The diagnostic rule checks the program code for invisible characters. Such characters are like backdoors that a programmer may not see because of the text display settings in the development environment, but the compiler sees and correctly processes them.
The analyzer warning:
V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. locales.h 1114
Let's take a closer look at the next line:
{ "Nag Mundari", "Nagm" },It contains the backdoor with an invisible character. If we use a hex editor, we can see the following:
There are 3 bytes sandwiched between the double quotation mark and the N character: E2, 80, and 8B. They correspond to the ZERO WIDTH SPACE (U+200B) Unicode character.
The strings from the script_list array that contains the "infected" string literal go into the TranslationServer::script_map hash table. The key of the hash table is the second string literal of the pair, and the value is the first one. So, the backdoored string literal goes into the hash table as a value, and the hash search isn't broken.
Next, we can look at where the value from the hash table could potentially go. We've found a few places:
TranslationServer::get_locale_name function. By analyzing the caller functions, we can see that this string gets into the GUI ([1], [2], [3], [4]) one way or another.TranslationServer::get_script_name function. By analyzing the caller functions, we can also see that the string will get into the GUI ([1], [2]).Most likely, the backdoor was added accidentally by copying the name from some website. We can simply delete this character from the string literal.
We found this bug in the Godot Engine project. The full article is at the link.
Defendant N3
The three vicious perps are on the court stand. Which one of them is the most dangerous criminal after all?
The third bug, are you trying to be clever? Computing complex equations on which important variables depend? Write a full confession! You broke it.
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;
....
}The PVS-Studio warning:
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
The analyzer warns that the level1_table_count value always equals 0 because the level0_table_count left operand is less than the HASHES_PER_L1_HASH right operand during an integer division operation. The value of HASHES_PER_L1_HASH is 41412, so to determine the level0_table_count value, take a look at the code above.
The file_block variable is computed by dividing the true_block by BLOCKS_PER_FILE, so it's within the range of [0 .. 82823].
The BLOCKS_PER_L0_HASH variable divides the value by 408, and the 1 is added to the result. When file_block reaches its maximum value, we'll get 202, so the level0_table_count variable value is within the range of [1 .. 203].
So, the level1_table_count variable is computed as 203/41412+1, and equals 1 for any true_block values.
Could we get it wrong somewhere? It appears not, it's not just our analyzer that warns about it.
It's an interesting case where we can't even spot the error at a glance. A reviewer might easily miss it because it's time-consuming and tedious to calculate manually.
There is a code comment that might shed light on this mystery. Perhaps someone has already got some thoughts on the matter?
We found this bug in the Xenia project. The full article is at the link.
Defendant N2
What's this? A null pointer dereference? It's the same story every year... No project review is without this bad guy. We can spot its tiny footprints in almost every codebase. Await harsh sentences. Now, let's see the crime scene!
Take a look at the following structure:
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.
}In its constructor, a number of functions are called to create some entities. We'll look only at the functions to which arguments are passed (there are six). For example, here are some of them:
ScheduleDAGSDNodes *llvm::createBURRListDAGScheduler(SelectionDAGISel *IS,
CodeGenOptLevel OptLevel)
{
const TargetSubtargetInfo &STI = IS->MF->getSubtarget();
....
}Or
ScheduleDAGSDNodes* createDefaultScheduler(SelectionDAGISel *IS,
CodeGenOpt::Level OptLevel)
{
const TargetLowering *TLI = IS->TLI;
const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
....
}If we look at the function call in the constructor, we see that nullptr is passed as its first argument. It's that first IS parameter that's dereferenced in the very first line of the function.
It's a strange code. Perhaps the LLVM developers know something and can actually control UB.
Every function with the first zero argument gets dereferenced.
So, here are the analyzer warnings:
We found this bug in the LLVM project. The full article is at the link.
Defendant N1
You've been waiting for it. We show it! Beware! Here it is—the leader of the bug gang! This is the day of reckoning! We've been waiting for it all year. Step forward and unveil yourself!
Let's examine the list of named constants:
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
};Now note the string array:
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"
};And now look at the code that turns the named constant into the string:
const char *qed_dbg_get_status_str(enum dbg_status status)
{
return (status < MAX_DBG_STATUS) ?
s_status_str[status] : "Invalid debug status";
}Where's the error?
Found it?
There's a hint that you could theoretically catch on to. In the array, a single empty string separates all rows except for one place. Let's start with the PVS-Studio analyzer warning:
V557 Array overrun is possible. The value of 'status' index could reach 58. qede_debug.c 7149
It points to the code fragment, where the index array returns the string pointer:
return (status < MAX_DBG_STATUS) ?
s_status_str[status] : "Invalid debug status";Initially, we diagnosed it incorrectly and unpretentiously. We inadvertently thought we found an off-by-one error here. It looked like a typical incorrect check, which we encountered many times in various projects.
Here is the essence of a classic antipattern. There is an enum whose last component determines the number of elements.
enum Efoo {
A,
B,
C,
COUNT
};Constants in enum are assigned values starting from 0. So, the COUNT auxiliary constant will be equal to 3, which corresponds to the number of the main named constants.
There's an array where each named constant corresponds to something:
char Cfoo[] = { 'A', 'B', 'C' };It's a common bug when developers check that the index doesn't overrun the array bounds:
char Convert(unsigned id)
{
return (id > COUNT) ? 0 : Cfoo[id];
}The check doesn't operate correctly: if id is equal to COUNT, the array will be out of bounds. PVS-Studio gives a similar warning about these kinds of errors.
The correct option would be one of the following:
return (id >= COUNT) ? 0 : Cfoo[id]; // OK
return (id < COUNT) ? Cfoo[id] : 0; // OKNothing special here. Let's move on. We can write out a warning about array overrun in the article but don't analyze the bug in the DPDK check article.
Then at the last minute—STOP!
THE CHECK IS ACTUALLY CORRECT!
return (status < MAX_DBG_STATUS) ?
s_status_str[status] : "Invalid debug status";Now it's less clear and more intriguing! Why did the analyzer issue a warning? A false positive? I don't think so. There's no way it could get tangled up.
Rolling up my sleeves, I start looking over the code. Here it is!
A missing line for the DBG_STATUS_NO_MATCHING_FRAMING_MODE constant.
In the enum:
DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS,
DBG_STATUS_NO_MATCHING_FRAMING_MODE,
DBG_STATUS_VFC_READ_ERROR,In the array:
/* 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",Look at a separator with two empty lines. Something went wrong. Maybe the array was filled incorrectly, or the row was accidentally deleted, or a branch merge failed, or there's another reason.
However, once we've already found the bug, two empty lines look suspicious. Well, developers will hardly pay attention to them in code reviews. Even if they do, they'll just remove one of them for beauty's sake. No one's going to say, "We might have missed something. Let's check it" :)
Results of the error:
DBG_STATUS_NO_MATCHING_FRAMING_MODE.We found this bug in the DPDK project. The full article is at the link.
The court is silent. Even the judge is speechless. The most dangerous criminals of the year have been apprehended. The only thing left is to destroy them, but that's another story... However, we wouldn't have been able to find these errors without the help of our trusty detective—the PVS-Studio analyzer.
In 2024, we've tackled quite a few bugs, but there are still countless projects worth checking. Our detective is ready to help you hunt down dangerous code fragments lurking in your project:
Bring your own license with you:
And if you'd like to read about all the most fascinating bugs for this year, I invite you to check out the top 10 bugs from projects written in Java and C#: