Возможно, читатели помнят мою статью под названием "Эффект последней строки". В ней идёт речь о замеченной мной закономерности: ошибка чаще всего допускается в последней строке однотипных блоков текста. Теперь я хочу рассказать о новом интересном наблюдении. Оказывается, программисты тяготеют к тому, чтобы допустить ошибку в функциях сравнения двух объектов. Такое утверждение выглядит неправдоподобным, однако, я покажу огромное количество примеров ошибок, которые шокируют читателя. Читайте новое исследование, будет интересно и страшно.
Я утверждаю, что программисты очень часто допускают ошибки в достаточно простых функциях, которые предназначены для сравнения двух объектов. Это утверждение основано на опыте проверки нашей командой большого количества открытых проектов на языках C, C++ и C#.
Функции, о которых идёт речь, носят такие названия, как IsEqual, Equals, Compare, AreEqual и так далее. Или перегруженные операторы, такие как ==, !=.
Я заметил, что при написании статей мне очень часто попадаются ошибки, относящиеся к функциям сравнения. Я решил исследовать этот вопрос подробнее и изучил базу найденных нами ошибок. Для этого по базе был выполнен поиск функций с именами, содержащими слова Cmp, Equal, Compare и так далее. Результат оказался весьма впечатляющим и шокирующим.
В общем-то эта история схожа с написанием статьи "Эффект последней строки". Точно также я заметил аномалию и решил изучить её подробнее. К сожалению, в отличии от упомянутой статьи, я не знаю какие числа и статистику мне привести. Возможно, я придумаю как и что можно посчитать - позже. Сейчас же, я руководствуюсь интуицией и могу только поделиться своими ощущениями. Они говорят, что ошибок в функциях сравнения много, и уверен, что у вас возникнет то же самое чувство, когда я покажу большое количество впечатляющих примеров.
На минутку вернемся к статье "Эффект последней строки". Кстати, если вы не читали её, то предлагаю сделать паузу и ознакомиться с ней. Есть и более детальный разбор этой темы: "Объяснение эффекта последней строки".
В целом можно сделать вывод, что причина ошибок в последних строках связана с тем, что разработчик уже мысленно перешел к следующим строкам/задачам вместо того, чтобы сосредоточиться на завершении текущего фрагмента. Как результат - в конце написания однотипных блоков текста, в последнем из них, он с большей вероятностью допустит опечатку, чем в предыдущих.
Я считаю, что в случае написания функции сравнения, разработчик вообще часто не сосредотачивается на ней, считая её тривиальной. Другими словами, он пишет код автоматически, не задумываясь над ним. В противном случае непонятно, как можно сделать подобную ошибку:
bool IsLuidsEqual(LUID luid1, LUID luid2)
{
return (luid1.LowPart == luid2.LowPart) &&
(luid2.HighPart == luid2.HighPart);
}Анализатор PVS-Studio обнаружил эту ошибку в коде проекта RunAsAdmin Explorer Shim (C++): V501 There are identical sub-expressions to the left and to the right of the '==' operator: luid2.HighPart == luid2.HighPart RAACommon raacommonfuncs.cpp 1511
Опечатка. Во второй строке должно быть написано: luid1.HighPart == luid2.HighPart.
Cогласитесь, код прост. Видимо, простота кода как раз всё и портит. Программист сразу воспринимает задачу по написанию такой функции стандартной и неинтересной. Он моментально решает в голове, как написать функцию, и остаётся только реализовать код. Это рутинный, но, к сожалению, необходимый процесс для того, чтобы перейти к написанию более важного, сложного и интересного кода. Мыслями разработчик уже там и... как результат - допускает ошибку.
Вдобавок программисты редко пишут юнит-тесты для таких функций. Опять подводит обманчивая простота этих функций. Кажется избыточным их тестировать, ведь эти функции такие простые и однотипные. Человек написал сотни таких функций за свою жизнь, неужели он может сделать в очередной функции ошибку?! Может, и делает.
При этом хочу обратить внимание, что речь в статье идёт не о коде студентов, которые учатся программировать. Речь идёт об ошибках в коде таких проектов, как GCC, Qt, GDB, LibreOffice, Unreal Engine 4, CryEngine V, Chromium, MongoDB, Oracle VM Virtual Box, FreeBSD, WinMerge, CoreCLR, MySQL, Mono, CoreFX, Roslyn, MSBuild и т.д. Всё очень серьезно.
Я покажу вам столько разнообразных примеров, что вам будет страшно ложиться спать.
Все ошибки в функциях сравнения будут разделены на несколько паттернов. В статье речь идёт об ошибках в проектах на языках C, C++ и C#, но как-то разделять эти языки смысла нет, так как многие паттерны одинаковы для разных языков.
Очень часто в функциях сравнения надо выполнить такие проверки:
Иногда программисты считают, что более красиво использовать тот же оператор <, но поменять местами имена переменных:
Однако, по невнимательности получаются вот такие проверки:
На самом деле, здесь два раза выполняется одно и то же сравнение. Возможно, вы не поняли о чём идёт речь, но сейчас мы перейдём к практическим примерам, и всё прояснится.
string _server;
....
bool operator<( const ServerAndQuery& other ) const {
if ( ! _orderObject.isEmpty() )
return _orderObject.woCompare( other._orderObject ) < 0;
if ( _server < other._server )
return true;
if ( other._server > _server )
return false;
return _extra.woCompare( other._extra ) < 0;
}Анализатор PVS-Studio обнаружил эту ошибку в коде проекта MongoDB (С++): V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 44, 46. parallel.h 46
Это условие:
if ( other._server > _server )всегда будет ложным, так как точно такая же проверка выполнялась двумя строчками выше. Корректный вариант кода:
if ( _server < other._server )
return true;
if ( other._server < _server )
return false;Обнаруживается такая ошибка и в коде проекта Chromium (C++):
enum ContentSettingsType;
struct EntryMapKey {
ContentSettingsType content_type;
...
};
bool OriginIdentifierValueMap::EntryMapKey::operator<(
const OriginIdentifierValueMap::EntryMapKey& other) const {
if (content_type < other.content_type)
return true;
else if (other.content_type > content_type)
return false;
return (resource_identifier < other.resource_identifier);
}Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 61, 63. browser content_settings_origin_identifier_value_map.cc 61
Я показал пример на языке C++, теперь очередь C#. Следующая ошибка найдена в коде IronPython and IronRuby (C#).
public static int Compare(SourceLocation left,
SourceLocation right) {
if (left < right) return -1;
if (right > left) return 1;
return 0;
}Предупреждение PVS-Studio (C#): V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless. SourceLocation.cs 156
Думаю, пояснения здесь излишни.
Примечание. Для C# здесь только один пример ошибки, а для C++ их было 2. И вообще, ошибок для C# кода в статье будет показано меньше, чем для C/C++. Однако, я не рекомендую спешить с выводом, что C# намного безопаснее. Дело в том, что анализатор PVS-Studio научился анализировать C# код относительно недавно, и мы просто проверили намного меньше проектов, написанных на C#, чем на С и C++.
Функции сравнения часто состоят из последовательных сравнений полей структур/классов. Такой код тяготеет к ошибкам, когда член класса начинает сравниваться сам с собой. При этом можно выделить два подвида ошибки.
В первом случае, забывают указать имя объекта и пишут так:
return m_x == foo.m_x &&
m_y == m_y && // <=
m_z == foo.m_z;Во втором случае, пишут одно и тоже имя объекта:
return zzz.m_x == foo.m_x &&
zzz.m_y == zzz.m_y && // <=
zzz.m_z == foo.m_z;Давайте теперь ознакомимся с этим паттерном ошибок на практике. Кстати, обратите внимание, что неправильное сравнение часто находится в последнем из однотипных блоков текста: привет "эффекту последней строки".
Ошибка найдена в коде проекта Unreal Engine 4 (C++):
bool
Compare(const FPooledRenderTargetDesc& rhs, bool bExact) const
{
....
return Extent == rhs.Extent
&& Depth == rhs.Depth
&& bIsArray == rhs.bIsArray
&& ArraySize == rhs.ArraySize
&& NumMips == rhs.NumMips
&& NumSamples == rhs.NumSamples
&& Format == rhs.Format
&& LhsFlags == RhsFlags
&& TargetableFlags == rhs.TargetableFlags
&& bForceSeparateTargetAndShaderResource ==
rhs.bForceSeparateTargetAndShaderResource
&& ClearValue == rhs.ClearValue
&& AutoWritable == AutoWritable; // <=
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: AutoWritable == AutoWritable rendererinterface.h 180
Код проекта Samba (C):
static int compare_procids(const void *p1, const void *p2)
{
const struct server_id *i1 = (struct server_id *)p1;
const struct server_id *i2 = (struct server_id *)p2;
if (i1->pid < i2->pid) return -1;
if (i2->pid > i2->pid) return 1;
return 0;
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '>' operator: i2->pid > i2->pid brlock.c 1901
Код проекта MongoDB (C++):
bool operator==(const MemberCfg& r) const {
....
return _id==r._id && votes == r.votes &&
h == r.h && priority == r.priority &&
arbiterOnly == r.arbiterOnly &&
slaveDelay == r.slaveDelay &&
hidden == r.hidden &&
buildIndexes == buildIndexes; // <=
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: buildIndexes == buildIndexes rs_config.h 101
Код проекта Geant4 Software (C++):
inline G4bool G4FermiIntegerPartition::
operator==(const G4FermiIntegerPartition& right)
{
return (total == right.total &&
enableNull == enableNull && // <=
partition == right.partition);
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: enableNull == enableNull G4hadronic_deex_fermi_breakup g4fermiintegerpartition.icc 58
Код проекта LibreOffice (C++):
class SvgGradientEntry
{
....
bool operator==(const SvgGradientEntry& rCompare) const
{
return (getOffset() == rCompare.getOffset()
&& getColor() == getColor() // <=
&& getOpacity() == getOpacity()); // <=
}
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: getColor() == getColor() svggradientprimitive2d.hxx 61
Код проекта Chromium (C++):
bool FileIOTest::MatchesResult(const TestStep& a,
const TestStep& b) {
....
return (a.data_size == a.data_size && // <=
std::equal(a.data, a.data + a.data_size, b.data));
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: a.data_size == a.data_size cdm_file_io_test.cc 367
Код проекта FreeCAD (C++):
bool FaceTypedBSpline::isEqual(const TopoDS_Face &faceOne,
const TopoDS_Face &faceTwo) const
{
....
if (surfaceOne->IsURational() !=
surfaceTwo->IsURational())
return false;
if (surfaceTwo->IsVRational() != // <=
surfaceTwo->IsVRational()) // <=
return false;
if (surfaceOne->IsUPeriodic() !=
surfaceTwo->IsUPeriodic())
return false;
if (surfaceOne->IsVPeriodic() !=
surfaceTwo->IsVPeriodic())
return false;
if (surfaceOne->IsUClosed() !=
surfaceTwo->IsUClosed())
return false;
if (surfaceOne->IsVClosed() !=
surfaceTwo->IsVClosed())
return false;
if (surfaceOne->UDegree() !=
surfaceTwo->UDegree())
return false;
if (surfaceOne->VDegree() !=
surfaceTwo->VDegree())
return false;
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'surfaceTwo->IsVRational()' to the left and to the right of the '!=' operator. modelrefine.cpp 780
Код проекта Serious Engine (C++):
class CTexParams {
public:
inline BOOL IsEqual( CTexParams tp) {
return tp_iFilter == tp.tp_iFilter &&
tp_iAnisotropy == tp_iAnisotropy && // <=
tp_eWrapU == tp.tp_eWrapU &&
tp_eWrapV == tp.tp_eWrapV; };
....
};Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180
Код проекта Qt (C++):
inline bool qCompare(QImage const &t1, QImage const &t2, ....)
{
....
if (t1.width() != t2.width() || t2.height() != t2.height()) {
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '!=' operator: t2.height() != t2.height() qtest_gui.h 101
Код проекта FreeBSD (C):
static int
compare_sh(const void *_a, const void *_b)
{
const struct ipfw_sopt_handler *a, *b;
a = (const struct ipfw_sopt_handler *)_a;
b = (const struct ipfw_sopt_handler *)_b;
....
if ((uintptr_t)a->handler < (uintptr_t)b->handler)
return (-1);
else if ((uintptr_t)b->handler > (uintptr_t)b->handler) // <=
return (1);
return (0);
}Предупреждение PVS-Studio: V501 There are identical sub-expressions '(uintptr_t) b->handler' to the left and to the right of the '>' operator. ip_fw_sockopt.c 2893
Код проекта Mono (C#):
static bool AreEqual (VisualStyleElement value1,
VisualStyleElement value2)
{
return
value1.ClassName == value1.ClassName && // <=
value1.Part == value2.Part &&
value1.State == value2.State;
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'value1.ClassName' to the left and to the right of the '==' operator. ThemeVisualStyles.cs 2141
Код проекта Mono (C#):
public int ExactInference (TypeSpec u, TypeSpec v)
{
....
var ac_u = (ArrayContainer) u;
var ac_v = (ArrayContainer) v;
....
var ga_u = u.TypeArguments;
var ga_v = v.TypeArguments;
....
if (u.TypeArguments.Length != u.TypeArguments.Length) // <=
return 0;
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'u.TypeArguments.Length' to the left and to the right of the '!=' operator. generic.cs 3135
Код проекта MonoDevelop (C#):
Accessibility DeclaredAccessibility { get; }
bool IsStatic { get; }
private bool MembersMatch(ISymbol member1, ISymbol member2)
{
if (member1.Kind != member2.Kind)
{
return false;
}
if (member1.DeclaredAccessibility != // <=1
member1.DeclaredAccessibility // <=1
|| member1.IsStatic != member1.IsStatic) // <=2
{
return false;
}
if (member1.ExplicitInterfaceImplementations().Any() ||
member2.ExplicitInterfaceImplementations().Any())
{
return false;
}
return SignatureComparer
.HaveSameSignatureAndConstraintsAndReturnTypeAndAccessors(
member1, member2, this.IsCaseSensitive);
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'member1.IsStatic' to the left and to the right of the '!=' operator. CSharpBinding AbstractImplementInterfaceService.CodeAction.cs 545
Код проекта Haiku (C++):
int __CORTEX_NAMESPACE__ compareTypeAndID(....)
{
int retValue = 0;
....
if (lJack && rJack)
{
if (lJack->m_jackType < lJack->m_jackType) // <=
{
return -1;
}
if (lJack->m_jackType == lJack->m_jackType) // <=
{
if (lJack->m_index < rJack->m_index)
{
return -1;
}
else
{
return 1;
}
}
else if (lJack->m_jackType > rJack->m_jackType)
{
retValue = 1;
}
}
return retValue;
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '<' operator: lJack->m_jackType < lJack->m_jackType MediaJack.cpp 783
Чуть ниже есть ещё одна точно такая же ошибка. Как я понимаю, в обоих случаях забыли заменить lJack на rJack.
Код проекта CryEngine V (C++):
bool
CompareRotation(const Quat& q1, const Quat& q2, float epsilon)
{
return (fabs_tpl(q1.v.x - q2.v.x) <= epsilon)
&& (fabs_tpl(q1.v.y - q2.v.y) <= epsilon)
&& (fabs_tpl(q2.v.z - q2.v.z) <= epsilon) // <=
&& (fabs_tpl(q1.w - q2.w) <= epsilon);
}Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z - q2.v.z entitynode.cpp 93
Данный вид ошибки возникает в программах на языках C и C++ и связан с неправильным использованием оператора sizeof. Ошибка заключается в вычислении не размера объекта, а размера указателя. Пример:
T *a = foo1();
T *b = foo2();
x = memcmp(a, b, sizeof(a));Вместо размера структуры T вычисляется размер указателя. Размер указателя зависит от используемой модели данных, но обычно это 4 или 8 байт. В результате сравнивается больше или меньше байт памяти, чем занимает структура.
Правильный вариант кода:
x = memcmp(a, b, sizeof(T));или
x = memcmp(a, b, sizeof(*a));Теперь давайте перейдём к практике. Вот как такая ошибка выглядит в коде проекта CryEngine V (C++):
bool
operator==(const SComputePipelineStateDescription& other) const
{
return 0 == memcmp(this, &other, sizeof(this));
}Предупреждение PVS-Studio: V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58
Код проекта Unreal Engine 4 (C++):
bool FRecastQueryFilter::IsEqual(
const INavigationQueryFilterInterface* Other) const
{
// @NOTE: not type safe, should be changed when
// another filter type is introduced
return FMemory::Memcmp(this, Other, sizeof(this)) == 0;
}Предупреждение PVS-Studio: V579 The Memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pimplrecastnavmesh.cpp 172
Функции сравнения часто вызывают другие функции сравнения. При этом одной из возможных ошибок является то, что два раза передаётся ссылка/указатель на один и тот же объект. Пример:
x = memcmp(A, A, sizeof(T));Здесь объект A будет сравнён сам с собой, что, естественно, не имеет смысла.
Начнём мы с ошибки, найденной в коде отладчика GDB (C):
static int
psymbol_compare (const void *addr1, const void *addr2,
int length)
{
struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value, // <=
sizeof (sym1->ginfo.value)) == 0
&& sym1->ginfo.language == sym2->ginfo.language
&& PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
&& PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2)
&& sym1->ginfo.name == sym2->ginfo.name);
}Предупреждение PVS-Studio: V549 The first argument of 'memcmp' function is equal to the second argument. psymtab.c 1580
Код проекта CryEngineSDK (C++):
inline bool operator != (const SEfResTexture &m) const
{
if (stricmp(m_Name.c_str(), m_Name.c_str()) != 0 || // <=
m_TexFlags != m.m_TexFlags ||
m_bUTile != m.m_bUTile ||
m_bVTile != m.m_bVTile ||
m_Filter != m.m_Filter ||
m_Ext != m.m_Ext ||
m_Sampler != m.m_Sampler)
return true;
return false;
}Предупреждение PVS-Studio: V549 The first argument of 'stricmp' function is equal to the second argument. ishader.h 2089
Код проекта PascalABC.NET (C#):
private List<string> enum_consts = new List<string>();
public override bool IsEqual(SymScope ts)
{
EnumScope es = ts as EnumScope;
if (es == null) return false;
if (enum_consts.Count != es.enum_consts.Count) return false;
for (int i = 0; i < es.enum_consts.Count; i++)
if (string.Compare(enum_consts[i],
this.enum_consts[i], true) != 0)
return false;
return true;
}Предупреждение PVS-Studio: V3038 The 'enum_consts[i]' argument was passed to 'Compare' method several times. It is possible that other argument should be passed instead. CodeCompletion SymTable.cs 2206
Здесь немного поясню. Ошибка в фактических аргументах функции Compare:
string.Compare(enum_consts[i], this.enum_consts[i], true)Дело в том, что enum_consts[i] и this.enum_consts[i] - это одно и то же. Как я понимаю, правильный вызов должен быть таким:
string.Compare(es.enum_consts[i], this.enum_consts[i], true)или
string.Compare(enum_consts[i], es.enum_consts[i], true)Распространённой ошибкой при программировании является, когда одна и та же проверка выполняется дважды. Пример:
return A == B &&
C == D && // <=
C == D && // <=
E == F;В подобных случаях возможны 2 варианта. Первый безобидный: одно сравнение лишнее, и его можно просто удалить. Второй хуже: хотели проверить какие-то другие переменные, но опечатались.
В любом случае подобный код заслуживает пристального внимания. Давайте я напугаю вас сильнее и покажу, что такую ошибку можно встретить даже в коде компилятора GCC (C):
static bool
dw_val_equal_p (dw_val_node *a, dw_val_node *b)
{
....
case dw_val_class_vms_delta:
return (!strcmp (a->v.val_vms_delta.lbl1,
b->v.val_vms_delta.lbl1)
&& !strcmp (a->v.val_vms_delta.lbl1,
b->v.val_vms_delta.lbl1));
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions '!strcmp(a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1)' to the left and to the right of the '&&' operator. dwarf2out.c 1428
Два раза вызывается функция strcmp с одним и тем же набором аргументов.
Код проекта Unreal Engine 4 (C++):
FORCEINLINE
bool operator==(const FShapedGlyphEntryKey& Other) const
{
return FontFace == Other.FontFace
&& GlyphIndex == Other.GlyphIndex // <=
&& FontSize == Other.FontSize
&& FontScale == Other.FontScale
&& GlyphIndex == Other.GlyphIndex; // <=
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'GlyphIndex == Other.GlyphIndex' to the left and to the right of the '&&' operator. fontcache.h 139
Код проекта Serious Engine (C++):
inline BOOL CValuesForPrimitive::operator==(....)
{
return (
(....) &&
(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&
....
(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&
....
);Предупреждение PVS-Studio: V501 There are identical sub-expressions '(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType)' to the left and to the right of the '&&' operator. worldeditor.h 580
Код проекта Oracle VM Virtual Box (C++):
typedef struct SCMDIFFSTATE
{
....
bool fIgnoreTrailingWhite;
bool fIgnoreLeadingWhite;
....
} SCMDIFFSTATE;
/* Pointer to a diff state. */
typedef SCMDIFFSTATE *PSCMDIFFSTATE;
/* Compare two lines */
DECLINLINE(bool) scmDiffCompare(PSCMDIFFSTATE pState, ....)
{
....
if (pState->fIgnoreTrailingWhite // <=
|| pState->fIgnoreTrailingWhite) // <=
return scmDiffCompareSlow(....);
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'pState->fIgnoreTrailingWhite' to the left and to the right of the '||' operator. scmdiff.cpp 238
Функция memcmp возвращает следующие значения типа int:
Обратите внимание. "Больше 0" означает любые числа, а вовсе не 1. Этими числами могут быть: 2, 3, 100, 256, 1024, 5555, 65536 и так далее. Это значит, что этот результат нельзя поместить в переменную типа char или short. Могут быть отброшены значащие биты, что приведет к нарушению логики выполнения программы.
Также из этого следует, что результат нельзя сравнивать с константами 1 или -1. Другими словами, так делать неправильно:
if (memcmp(a, b, sizeof(T)) == 1)
if (memcmp(x, y, sizeof(T)) == -1)Правильные сравнения:
if (memcmp(a, b, sizeof(T)) > 0)
if (memcmp(a, b, sizeof(T)) < 0)Коварность описанных ошибок в том, что код может долгое время успешно работать. Ошибки могут начать проявлять себя при переходе на новую платформу или при смене версии компилятора.
Код проекта ReactOS (C++):
HRESULT WINAPI CRecycleBin::CompareIDs(....)
{
....
return MAKE_HRESULT(SEVERITY_SUCCESS, 0,
(unsigned short)memcmp(pidl1->mkid.abID,
pidl2->mkid.abID,
pidl1->mkid.cb));
}Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned short' type variable is inappropriate. The significant bits could be lost breaking the program's logic. recyclebin.cpp 542
Код проекта Firebird (C++):
SSHORT TextType::compare(ULONG len1, const UCHAR* str1,
ULONG len2, const UCHAR* str2)
{
....
SSHORT cmp = memcmp(str1, str2, MIN(len1, len2));
if (cmp == 0)
cmp = (len1 < len2 ? -1 : (len1 > len2 ? 1 : 0));
return cmp;
}Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'short' type variable is inappropriate. The significant bits could be lost breaking the program's logic. texttype.cpp 338
Код проекта CoreCLR (C++):
bool operator( )(const GUID& _Key1, const GUID& _Key2) const
{ return memcmp(&_Key1, &_Key2, sizeof(GUID)) == -1; }Предупреждение PVS-Studio: V698 Expression 'memcmp(....) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'memcmp(....) < 0' instead. sos util.cpp 142
Код проекта OpenToonz (C++):
bool TFilePath::operator<(const TFilePath &fp) const
{
....
char differ;
differ = _wcsicmp(iName.c_str(), jName.c_str());
if (differ != 0)
return differ < 0 ? true : false;
....
}Предупреждение PVS-Studio: V642 Saving the '_wcsicmp' function result inside the 'char' type variable is inappropriate. The significant bits could be lost, breaking the program's logic. tfilepath.cpp 328
Данный паттерн ошибки свойственен программам на языке C#. В функциях сравнения иногда выполняют приведение типа с помощью оператора as. Ошибка заключается в том, что по невнимательности на null проверяется не новая ссылка, а исходная. Рассмотрим синтетический пример:
ChildT foo = obj as ChildT;
if (obj == null)
return false;
if (foo.zzz()) {}Проверка if (obj == null) защищает от ситуации, если переменная obj содержала нулевую ссылку. Однако, нет никакой защиты от случая, если окажется, что оператор as вернёт нулевую ссылку. Правильный код должен выглядеть так:
ChildT foo = obj as ChildT;
if (foo == null)
return false;
if (foo.zzz()) {}Как правило, ошибка возникает из-за невнимательности программиста. Подобные ошибки возможны и в программах на языке С и C++, но я не нашел в базе найденных нами ошибок ни одного такого случая.
Код проекта MonoDevelop (C#):
public override bool Equals (object o)
{
SolutionItemReference sr = o as SolutionItemReference;
if (o == null)
return false;
return (path == sr.path) && (id == sr.id);
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'sr'. MonoDevelop.Core SolutionItemReference.cs 81
Код проекта CoreFX (C#):
public override bool Equals(object comparand)
{
CredentialHostKey comparedCredentialKey =
comparand as CredentialHostKey;
if (comparand == null)
{
// This covers also the compared == null case
return false;
}
bool equals = string.Equals(AuthenticationType,
comparedCredentialKey.AuthenticationType, ....
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'comparand', 'comparedCredentialKey'. CredentialCache.cs 4007
Код проекта Roslyn (C#):
public override bool Equals(object obj)
{
var d = obj as DiagnosticDescription;
if (obj == null)
return false;
if (!_code.Equals(d._code))
return false;
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'd'. DiagnosticDescription.cs 201
Код проекта Roslyn (C#):
protected override bool AreEqual(object other)
{
var otherResourceString = other as LocalizableResourceString;
return
other != null &&
_nameOfLocalizableResource ==
otherResourceString._nameOfLocalizableResource &&
_resourceManager == otherResourceString._resourceManager &&
_resourceSource == otherResourceString._resourceSource &&
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'other', 'otherResourceString'. LocalizableResourceString.cs 121
Код проекта MSBuild (C#):
public override bool Equals(object obj)
{
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (obj == null) // <=
{
return false;
}
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'name'. AssemblyRemapping.cs 64
Код проекта Mono (C#):
public override bool Equals (object o)
{
UrlMembershipCondition umc = (o as UrlMembershipCondition);
if (o == null) // <=
return false;
....
return (String.Compare (u, 0, umc.Url, ....) == 0); // <=
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'umc'. UrlMembershipCondition.cs 111
Код проекта Media Portal 2 (C#):
public override bool Equals(object obj)
{
EpisodeInfo other = obj as EpisodeInfo;
if (obj == null) return false;
if (TvdbId > 0 && other.TvdbId > 0)
return TvdbId == other.TvdbId;
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'other'. EpisodeInfo.cs 560
Код проекта NASA World Wind (C#):
public int CompareTo(object obj)
{
RenderableObject robj = obj as RenderableObject;
if(obj == null) // <=
return 1;
return this.m_renderPriority.CompareTo(robj.RenderPriority);
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'robj'. RenderableObject.cs 199
В некоторых функциях сравниваются коллекции элементов. Естественно, что для их сравнения используются различные варианты циклов. Если писать код невнимательно, как и бывает с функциями сравнения, то с циклами легко что-то напутать. Рассмотрим несколько таких ситуаций.
Код проекта Trans-Proteomic Pipeline (C++):
bool Peptide::operator==(Peptide& p) {
....
for (i = 0, j = 0;
i < this->stripped.length(), j < p.stripped.length();
i++, j++) {
....
}Предупреждение PVS-Studio: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. tpplib peptide.cpp 191
Обратите внимание, что в условии используется оператор запятая. Код явно неверен, так как условие, стоящее слева от запятой, не учитывается. То есть условие слева вычисляется, но его результат никак не используется.
Код проекта Qt (C++):
bool equals( class1* val1, class2* val2 ) const
{
...
size_t size = val1->size();
...
while ( --size >= 0 ){
if ( !comp(*itr1,*itr2) )
return false;
itr1++;
itr2++;
}
...
}Предупреждение PVS-Studio: V547 Expression '-- size >= 0' is always true. Unsigned type value is always >= 0. QtCLucene arrays.h 154
Код проекта CLucene (C++):
class Arrays
{
....
bool equals( class1* val1, class2* val2 ) const{
static _comparator comp;
if ( val1 == val2 )
return true;
size_t size = val1->size();
if ( size != val2->size() )
return false;
_itr1 itr1 = val1->begin();
_itr2 itr2 = val2->begin();
while ( --size >= 0 ){
if ( !comp(*itr1,*itr2) )
return false;
itr1++;
itr2++;
}
return true;
}
....
}Предупреждение PVS-Studio: V547 Expression '-- size >= 0' is always true. Unsigned type value is always >= 0. arrays.h 154
Код проекта Mono (C#):
public override bool Equals (object obj)
{
....
for (int i=0; i < list.Count; i++) {
bool found = false;
for (int j=0; i < ps.list.Count; j++) { // <=
if (list [i].Equals (ps.list [j])) {
found = true;
break;
}
}
if (!found)
return false;
}
return true;
}Предупреждение PVS-Studio: V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' corlib-net_4_x PermissionSet.cs 607
По всей видимости допущена опечатка, и на самом деле в условии вложенного цикла должна использоваться переменная j, а не i:
for (int j=0; j < ps.list.Count; j++)Часто в функциях сравнения приходится писать код вот такого типа:
if (GetA().x == GetB().x && GetA().y == GetB().y)Чтобы сократить размер условий или для оптимизации, используют промежуточные переменные:
Type A = GetA();
Type B = GetB();
if (A.x == B.x && A.y == B.y)При этом по невнимательности иногда допускают ошибку и инициализируют временные переменные одним и тем же значением:
Type A = GetA();
Type B = GetA();Давайте посмотрим, как такие ошибки выглядят в коде реальных приложений.
Код проекта LibreOffice (C++):
bool CmpAttr(
const SfxPoolItem& rItem1, const SfxPoolItem& rItem2)
{
....
bool bNumOffsetEqual = false;
::boost::optional<sal_uInt16> oNumOffset1 =
static_cast<const SwFmtPageDesc&>(rItem1).GetNumOffset();
::boost::optional<sal_uInt16> oNumOffset2 =
static_cast<const SwFmtPageDesc&>(rItem1).GetNumOffset();
if (!oNumOffset1 && !oNumOffset2)
{
bNumOffsetEqual = true;
}
else if (oNumOffset1 && oNumOffset2)
{
bNumOffsetEqual = oNumOffset1.get() == oNumOffset2.get();
}
else
{
bNumOffsetEqual = false;
}
....
}Предупреждение PVS-Studio: V656 Variables 'oNumOffset1', 'oNumOffset2' are initialized through the call to the same function. It's probably an error or un-optimized code. Check lines: 68, 69. findattr.cxx 69
Код проекта Qt (C++):
AtomicComparator::ComparisonResult
IntegerComparator::compare(const Item &o1,
const AtomicComparator::Operator,
const Item &o2) const
{
const Numeric *const num1 = o1.as<Numeric>();
const Numeric *const num2 = o1.as<Numeric>();
if(num1->isSigned() || num2->isSigned())
....
}Предупреждение PVS-Studio: V656 Variables 'num1', 'num2' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'o1.as < Numeric > ()' expression. Check lines: 220, 221. qatomiccomparators.cpp 221
Очень многие ошибки, приведённые ранее, можно назвать последствиями неудачного Copy-Paste. Они попадали под какой-то ошибочный паттерн, и я решил, что их логично описать в соответствующих разделах. Однако, у меня осталось несколько ошибок, которые явно возникли из-за неудачного копирования кода, но которые я не знаю как классифицировать. Поэтому я просто собрал эти ошибки здесь.
Код проекта CoreCLR (C++):
int __cdecl Compiler::RefCntCmp(const void* op1, const void* op2)
{
....
if (weight1)
{
....
if (varTypeIsGC(dsc1->TypeGet()))
{
weight1 += BB_UNITY_WEIGHT / 2;
}
if (dsc1->lvRegister)
{
weight1 += BB_UNITY_WEIGHT / 2;
}
}
if (weight1)
{
....
if (varTypeIsGC(dsc2->TypeGet()))
{
weight1 += BB_UNITY_WEIGHT / 2; // <=
}
if (dsc2->lvRegister)
{
weight2 += BB_UNITY_WEIGHT / 2;
}
}
....
}Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'weight2' variable should be used instead of 'weight1'. clrjit lclvars.cpp 2702
Функция была длинная, поэтому она основательно сокращена для статьи. Если рассматривать код этой функции, то заметно, что часть кода была скопирована, но в одном месте программист забыл заменить переменную weight1 на weight2.
Код проекта WPF samples by Microsoft (C#):
public int Compare(GlyphRun a, GlyphRun b)
{
....
if (aPoint.Y > bPoint.Y) // <=
{
return -1;
}
else if (aPoint.Y > bPoint.Y) // <=
{
result = 1;
}
else if (aPoint.X < bPoint.X)
{
result = -1;
}
else if (aPoint.X > bPoint.X)
{
result = 1;
}
....
}Предупреждение PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 418, 422. txtserializerwriter.cs 418
Код проекта PascalABC.NET (C#):
public void CompareInternal(....)
{
....
else if (left is int64_const)
CompareInternal(left as int64_const, right as int64_const);
....
else if (left is int64_const)
CompareInternal(left as int64_const, right as int64_const);
....
}Предупреждение PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 597, 631. ParserTools SyntaxTreeComparer.cs 597
Код проекта SharpDevelop (C#):
public int Compare(SharpTreeNode x, SharpTreeNode y)
{
....
if (typeNameComparison == 0) {
if (x.Text.ToString().Length < y.Text.ToString().Length)
return -1;
if (x.Text.ToString().Length < y.Text.ToString().Length)
return 1;
}
....
}Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless NamespaceTreeNode.cs 87
Код проекта Coin3D (C++):
int
SbProfilingData::operator == (const SbProfilingData & rhs) const
{
if (this->actionType != rhs.actionType) return FALSE;
if (this->actionStartTime != rhs.actionStopTime) return FALSE;
if (this->actionStartTime != rhs.actionStopTime) return FALSE;
....
}Предупреждение PVS-Studio: V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 1205, 1206. sbprofilingdata.cpp 1206
Код проекта Spring (C++):
bool operator < (const aiFloatKey& o) const
{return mTime < o.mTime;}
bool operator > (const aiFloatKey& o) const
{return mTime < o.mTime;}Предупреждение PVS-Studio: V524 It is odd that the body of '>' function is fully equivalent to the body of '<' function. assimp 3dshelper.h 470
И последний особенно интересный фрагмент кода, который анализатор кода PVS-Studio нашёл в проекте MySQL (C++):
static int rr_cmp(uchar *a,uchar *b)
{
if (a[0] != b[0])
return (int) a[0] - (int) b[0];
if (a[1] != b[1])
return (int) a[1] - (int) b[1];
if (a[2] != b[2])
return (int) a[2] - (int) b[2];
if (a[3] != b[3])
return (int) a[3] - (int) b[3];
if (a[4] != b[4])
return (int) a[4] - (int) b[4];
if (a[5] != b[5])
return (int) a[1] - (int) b[5]; // <=
if (a[6] != b[6])
return (int) a[6] - (int) b[6];
return (int) a[7] - (int) b[7];
}Предупреждение PVS-Studio: V525 The code containing the collection of similar blocks. Check items '0', '1', '2', '3', '4', '1', '6' in lines 680, 682, 684, 689, 691, 693, 695. sql records.cc 680
Скорее всего, программист написал первое сравнение, потом второе и ему стало скучно. Поэтому он скопировал в буфер обмена блок текста:
if (a[1] != b[1])
return (int) a[1] - (int) b[1];И вставил его в текст программы нужное ему количество раз. Затем он менял индексы, но в одном месте ошибся, и получилось некорректное сравнение:
if (a[5] != b[5])
return (int) a[1] - (int) b[5];Примечание. Я подробнее рассуждаю об этой ошибке в мини-книге "Главный вопрос программирования, рефакторинга и всего такого" (см. главу "Не берите на себя работу компилятора").
В C# принято реализовывать методы Equals так, чтобы они корректно обрабатывали ситуацию, если в качестве аргумента приходит нулевая ссылка. К сожалению, реализация не всех методов соответствует этому правилу.
Код проекта GitExtensions (C#):
public override bool Equals(object obj)
{
return GetHashCode() == obj.GetHashCode(); // <=
}Предупреждение PVS-Studio: V3115 Passing 'null' to 'Equals(object obj)' method should not result in 'NullReferenceException'. Git.hub Organization.cs 14
Код проекта PascalABC.NET (C#):
public override bool Equals(object obj)
{
var rhs = obj as ServiceReferenceMapFile;
return FileName == rhs.FileName;
}Предупреждение PVS-Studio: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ICSharpCode.SharpDevelop ServiceReferenceMapFile.cs 31
Код проекта G3D Content Pak (C++):
bool Matrix4::operator==(const Matrix4& other) const {
if (memcmp(this, &other, sizeof(Matrix4) == 0)) {
return true;
}
...
}Предупреждение PVS-Studio: V575 The 'memcmp' function processes '0' elements. Inspect the 'third' argument. graphics3D matrix4.cpp 269
Одна закрывающаяся скобка стоит не на своём месте. В результате количество сравниваемых байт вычисляется выражением sizeof(Matrix4) == 0. Размер любого класса больше 0, а значит результат выражения равен 0. Таким образом, сравнивается 0 байт.
Правильный вариант:
if (memcmp(this, &other, sizeof(Matrix4)) == 0) {Код проекта Wolfenstein 3D (C++):
inline int operator!=( quat_t a, quat_t b )
{
return ( ( a.x != b.x ) || ( a.y != b.y ) ||
( a.z != b.z ) && ( a.w != b.w ) );
}Предупреждение PVS-Studio: V648 Priority of the '&&' operation is higher than that of the '||' operation. math_quaternion.h 167
В одном месте, видимо, случайно написали оператор && вместо ||.
Код проекта FlightGear (C):
static int tokMatch(struct Token* a, struct Token* b)
{
int i, l = a->strlen;
if(!a || !b) return 0;
....
}Предупреждение PVS-Studio: V595 The 'a' pointer was utilized before it was verified against nullptr. Check lines: 478, 479. codegen.c 478
Если в функцию передать в качестве первого аргумента NULL, то произойдёт разыменование нулевого указателя, хотя по задумке функция должна вернуть значение 0.
Код проекта WinMerge (C++):
int TimeSizeCompare::CompareFiles(int compMethod,
const DIFFITEM &di)
{
UINT code = DIFFCODE::SAME;
...
if (di.left.size != di.right.size)
{
code &= ~DIFFCODE::SAME;
code = DIFFCODE::DIFF;
}
...
}Предупреждение PVS-Studio: V519 The 'code' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 79, 80. Merge timesizecompare.cpp 80
Код проекта ReactOS (C++):
#define IsEqualGUID(rguid1, rguid2) \
(!memcmp(&(rguid1), &(rguid2), sizeof(GUID)))
static int ctl2_find_guid(....)
{
MSFT_GuidEntry *guidentry;
...
if (IsEqualGUID(guidentry, guid)) return offset;
...
}Предупреждение PVS-Studio: V512 A call of the 'memcmp' function will lead to underflow of the buffer 'guidentry'. oleaut32 typelib2.c 320
В качестве первого аргумента макроса выступает указатель. В результате вычисляется адрес указателя, что не имеет никакого смысла.
Правильный вариант:
if (IsEqualGUID(*guidentry, guid)) return offset;Код проекта IronPython and IronRuby (C#):
public static bool Equals(float x, float y) {
if (x == y) {
return !Single.IsNaN(x);
}
return x == y;
}Предупреждение PVS-Studio: V3024 An odd precise comparison: x == y. Consider using a comparison with defined precision: Math.Abs(A - B) < Epsilon. FloatOps.cs 1048
Непонятно, в чём тут смысл специальной проверки на NaN. Если условие (x == y) выполняется, то это значит, что и x, и y отличны от NaN, потому что NaN не равен никакому другому значению, в том числе и самому себе. Похоже, что проверка на NaN просто лишняя и код можно сократить до:
public static bool Equals(float x, float y) {
return x == y;
}Код проекта Mono (C#):
public bool Equals (CounterSample other)
{
return
rawValue == other.rawValue &&
baseValue == other.counterFrequency && // <=
counterFrequency == other.counterFrequency && // <=
systemFrequency == other.systemFrequency &&
timeStamp == other.timeStamp &&
timeStamp100nSec == other.timeStamp100nSec &&
counterTimeStamp == other.counterTimeStamp &&
counterType == other.counterType;
}Предупреждение PVS-Studio: V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'baseValue == other.counterFrequency'. System-net_4_x CounterSample.cs 139
Просматривая все эти ошибки, кажется удивительным, что все эти программы вообще работают. Ведь функции сравнения выполняют крайне важную и ответственную задачу в программах.
Есть несколько объяснений работоспособности программ с такими ошибками:
Я продемонстрировал, как много ошибок можно встретить в функциях сравнения. А отсюда следует, что работоспособность таких функций обязательно должна проверяться с помощью юнит-тестов.
Непременно пишите юнит-тесты для операторов сравнения, для функций Equals и так далее.
Уверен, что многим до прочтения этой статьи казалось, что такие тесты избыточны и всё равно никаких ошибок они не выявят: ведь функции сравнения на первый взгляд так просты... Что же, теперь я показал весь ужас, который может в них скрываться.
Обзоры кода и использования инструментов статического анализа тоже будут не лишними.
В этой статье упоминалось множество именитых проектов, которые разрабатываются высококлассными специалистами. Эти проекты тщательно тестируются с помощью различных методологий. И все равно, это не помешало анализатору PVS-Studio найти в них ошибки. Это говорит о том, что PVS-Studio может стать отличным дополнением к другим методологиям, используемым для повышения качества и надёжности кода.
Приходите к нам на сайт и попробуйте PVS-Studio.