Есть бесконечное количество способов ошибиться при написании кода. Однако иногда можно заметить явные интересные закономерности, как и где ошибаются программисты. Поговорим о коде, который "притягивает" опечатки.
С целью тестирования и продвижения статического анализатора кода PVS-Studio мы проверяем различные открытые проекты. Найдя ошибки, мы сообщаем о них авторам проектов, коллекционируем их и пишем статьи про наиболее интересные случаи.
Рассматривая все эти ошибки, я постепенно замечаю различные повторяющиеся паттерны опечаток. За редким исключением они не зависят от языка программирования. По крайней мере, они одновременно свойственны коду, написанному на C, C++, C#, Java. В этой статье я опишу 7 паттернов, которые заметил к настоящему моменту:
Заметность закономерностей в ошибках свидетельствует о том, что они крайне распространены. Полезно знать о них, чтобы избегать написания потенциально опасного кода или более эффективно находить их в процессе обзоров кода. Другим словами, вы узнаете, какой код притягивает ошибки, и будете более внимательно его проверять. Конечно, PVS-Studio способен выявить многие подобные ошибки, но не все. Поэтому дополнительное внимание не повредит.
Самый первый замеченный мною паттерн. Впервые я описывал его в статье ещё в 2014 году. Формулировка: при написании однотипных блоков кода наибольшая вероятность допустить ошибку будет в последнем блоке.
Поясню на примере кода из проекта Godot Engine (C++).
String SoftBody::get_configuration_warning() const {
....
Transform t = get_transform();
if ((ABS(t.basis.get_axis(0).length() - 1.0) > 0.05 ||
ABS(t.basis.get_axis(1).length() - 1.0) > 0.05 ||
ABS(t.basis.get_axis(0).length() - 1.0) > 0.05)) {
if (!warning.empty())
....
}
Перед нами классическая Copy-Paste ошибка. Программисту не хочется заново набирать похожие строчки кода. Поэтому он берёт и дважды дублирует строчку кода:
ABS(t.basis.get_axis(0).length() - 1.0) > 0.05
После чего он заменяет во второй строке 0 на 1, а вот в последней строке заменить 0 на 2 он забывает. Когда отдельно рассматривается такой фрагмент кода, кажется удивительным, что допущена такая простая ошибка. Но именно так и выглядит этот паттерн.
Другой пример ошибки, найденный в юнит-тестах проекта LLVM (C++).
TEST(RegisterContextMinidump, ConvertMinidumpContext_x86_64) {
MinidumpContext_x86_64 Context;
....
Context.rax = 0x0001020304050607;
Context.rbx = 0x08090a0b0c0d0e0f;
....
Context.eflags = 0x88898a8b;
Context.cs = 0x8c8d;
Context.fs = 0x8e8f;
Context.gs = 0x9091;
Context.ss = 0x9293; // <=
Context.ds = 0x9495;
Context.ss = 0x9697; // <=
llvm::ArrayRef<uint8_t> ContextRef(reinterpret_cast<uint8_t *>(&Context),
sizeof(Context));
....
}
Обратите внимание на строчки кода, выделенные комментариями. Этот код вряд ли писался с помощью Copy-Paste, но ошибка всё равно в последней строке. Программист поспешил и по невнимательности допустил опечатку, повторно записав значение в регистр ss, а не в es.
Примечание. Ошибка интересна ещё и тем, что показывает, как статический анализатор дополняет методологию юнит-тестирования. В юнит-тестах тоже могут быть ошибки, из-за которых какие-то случаи не тестируются или тестируются не полностью. Но ведь никто не будет писать юнит-тесты на юнит-тесты. Здесь помогут анализаторы кода.
Чтобы не создавалось впечатления, что подобные баги свойственны только языкам C и C++, приведу код из мира Java. Следующую опечатка замечена мною в IntelliJ IDEA Community Edition (Java).
private static boolean isBeforeOrAfterKeyword(String str, boolean trimKeyword) {
return (trimKeyword ? LoadingOrder.BEFORE_STR.trim() :
LoadingOrder.BEFORE_STR).equalsIgnoreCase(str) ||
(trimKeyword ? LoadingOrder.AFTER_STR.trim() :
LoadingOrder.AFTER_STR).equalsIgnoreCase(str) ||
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) || // <=
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str); // <=
}
Перед нами явная закономерность. С другими схожими примерами вы можете познакомиться, открыв старую статью "Эффект последней строки".
Почему этому паттерну я дал такое название? Он навеян ассоциацией с "эффектом последних метров" из мира альпинизма. Я читал про наблюдение, что альпинисты часто допускают ошибку в конце подъема. Причём причина не в усталости, а в том, что они становятся менее сосредоточенными. Кажется, что дело уже сделано, скоро можно будет отдохнуть и порадоваться. Альпинист теряет внимательность, спешит закончить подъём и делает что-то не так.
При написании кода происходит что-то похожее. Писать однотипные блоки кода скучно. Мы найдём подтверждение этому ниже, когда будем говорить о функциях сравнения. Поэтому человек торопится закончить монотонную работу. При написании последней строки кода программист радуется, что заканчивает с рутиной, и, возможно, уже начинает думать том, что напишет далее.
В любом случае внимание ослабевает, и программист допускает ошибку. Конечно, он может ошибиться в другом месте, но наблюдения говорят, что вероятнее всего, это произойдёт в последнем блоке однотипного кода.
С обзором кода происходит аналогичная история. Скучно проверять однотипный код. Поэтому внимание быстро снижается, и последние строчки кода будут проверены поверхностно или вообще не будут проверены. Вот так в коде возникают и остаются незамеченными ошибки, которые удивляют своей простотой, когда обнаруживаются.
Помимо универсальных паттернов, существуют паттерны, свойственные определённому языку. Например, огромное количество ошибок связано с использованием функции memset. В основном она используется в программах на C, но её нередко можно встретить и в C++ коде.
void* memset( void* dest, int ch, size_t count ); // С
void* std::memset( void* dest, int ch, std::size_t count ); // С++
Функция заполняет count первых байтов буфера, на который указывает dest, значением (unsigned char) ch. Описание:
На мой взгляд, эта функция — чемпион С и C++ мира по притягиванию к себе багов. Чаще всего ошибка связана с неправильным вычислением размера заполняемого буфера.
Опечатки заключаются в использовании лишних операторов разыменования * или, наоборот, в их отсутствии там, где надо. Или в лишних операторах взятия адреса &. Сейчас мы рассмотрим примеры таких ошибок.
Забыли разыменовать указатель *. Фрагмент кода FAR Manager v2 for Linux (Far2l, C++). Из-за опечатки оператор sizeof вычисляет не размер структуры, а размер указателя.
int64_t FileList::VMProcess(int OpCode,
void *vParam,
int64_t iParam)
{
....
PluginInfo *PInfo = (PluginInfo *)vParam;
memset(PInfo, 0, sizeof(PInfo));
....
}
Правильный вариант:
memset(PInfo, 0, sizeof(*PInfo));
Лишний оператор взятия адреса &. Код из проекта Energy Checker SDK (С). Оператор sizeof вычисляет не размер структуры WIN32_FIND_DATA, а размер указателя. В результате, обнулены будут только первые 4 или 8 байт структуры в зависимости от того, собирается Win32 или Win64 приложение.
int plh_read_pl_folder(PPLH_PL_FOLDER_INFO pconfig) {
....
WIN32_FIND_DATA file_data;
....
memset(
&file_data,
0,
sizeof(&file_data)
);
....
}
Должно быть написано: sizeof(file_data). Ещё лучше вообще избавиться от вызова функции memset, инициализируя объект нулями ещё на этапе объявления:
WIN32_FIND_DATA file_data = { };
Перепутали & и *. Проект Wolfenstein 3D (С). Здесь вместо взятия адреса наоборот нужно было разыменовать указатель, чтобы правильно вычислить размер структуры.
void CG_RegisterItemVisuals( int itemNum ) {
....
itemInfo_t *itemInfo;
....
memset( itemInfo, 0, sizeof( &itemInfo ) );
....
}
Переполнение буфера. В проекте Open source emulator PS4 (C++), автор кода случайно спутал sizeof c countof. Он подумал, что sizeof вычислит количество элементов и поэтому умножил это значение на размер одного элемента.
struct GnmCmdPSShader
{
....
uint32_t reserved[27];
};
int PS4API sceGnmSetPsShader350(....)
{
....
memset(param->reserved, 0, sizeof(param->reserved) * sizeof(uint32_t));
return SCE_OK;
}
Правильный вариант кода:
memset(param->reserved, 0, sizeof(param->reserved));
Примечание. Кстати, функция memset притягивает к себе не только опечатки, но и другие ошибки. Можете взглянуть на них в статье "Самая опасная функция в мире С/С++". Например, встречаются потенциальные уязвимости, когда функцию memset используют в конце, чтобы затереть приватные данные. Пример из проекта WebRTC (C++).
void AsyncSocksProxySocket::SendAuth() {
....
char * sensitive = new char[len];
pass_.CopyTo(sensitive, true);
request.WriteString(sensitive); // Password
memset(sensitive, 0, len);
delete [] sensitive;
....
}
Дело в том, что в целях оптимизации компиляторы удалят этот вызов функции memset. С точки зрения компилятора, раз буфер освобождается, то его можно не заполнять. В результате приватные данные останутся в памяти. Эта тема выходит за рамки статьи про опечатки, поэтому если хочется подробностей, то предлагаю познакомиться со следующими ссылками:
MyStruct foo;
memset(foo, 0, sizeof(foo));
Проще и безопаснее обнулять структуры сразу при их объявлении:
MyStruct foo = { };
Если следует обнулить массив в C++, то лучше прибегнуть к функции std::fill. Поясню это на примере ошибки, найденной мною в проекте Notepad++ (C++).
#define CONT_MAP_MAX 50
int _iContMap[CONT_MAP_MAX];
....
DockingManager:
ockingManager()
{
....
memset(_iContMap, -1, CONT_MAP_MAX);
....
}
Программист думает, что инициализирует массив из 50 элементов типа int, значением -1. Но функция memset работает с байтами. На самом деле первые 12 элементов будут проинициализированы значением 0xFFFFFFFF. Фактически они будут заполнены правильным значением -1, но это просто везение. Ещё один элемент получит значение 0x0000FFFF. Остальные элементы массива останутся неинициализированными.
В случае использования std::fill какую-то ошибку совершить гораздо сложнее:
// since C++11
std::fill(std::begin(_iContMap), std::end(_iContMap), -1);
// since C++20
std::ranges::fill(_iContMap, -1);
Людям скучно писать функции сравнения. По факту это вспомогательный код, который прост и неинтересен. Как говорится, там нет места творчеству. Нужно просто сравнить все нужные переменные между собой.
Из-за этого программисты торопятся при написании таких функций. Торопятся они и при их проверке на обзорах кода. Тяжело сосредоточить внимание на таком однотипном простом коде. Подобные функции на обзоре кода просматриваются "по диагонали". Результатом этого является большое количество незамеченных опечаток.
Начнём с классической опечатки в большом блоке однотипных сравнений. Проект Apache Flink (Java).
@Override
public boolean equals(Object o)
{
....
CheckpointStatistics that = (CheckpointStatistics) o;
return id == that.id &&
savepoint == that.savepoint &&
triggerTimestamp == that.triggerTimestamp &&
latestAckTimestamp == that.latestAckTimestamp &&
stateSize == that.stateSize &&
duration == that.duration &&
alignmentBuffered == that.alignmentBuffered &&
processedData == processedData &&
persistedData == that.persistedData &&
numSubtasks == that.numSubtasks &&
numAckSubtasks == that.numAckSubtasks &&
status == that.status &&
Objects.equals(checkpointType, that.checkpointType) &&
Objects.equals(
checkpointStatisticsPerTask,
that.checkpointStatisticsPerTask);
}
Нашли ошибку? Думаю, нет. Такой код скучно и писать, и читать. Скорее всего и вы поленились его внимательно изучить. Вот строчка с опечаткой:
processedData == processedData
Переменная сравнивается сама с собой. Правильный вариант:
processedData == that.processedData
Аналогичные опечатки можно встреть в куда более маленьких функциях. Проект eShopOnContainers (C#).
private bool CheckSameOrigin(string urlHook, string url)
{
var firstUrl = new Uri(urlHook, UriKind.Absolute);
var secondUrl = new Uri(url, UriKind.Absolute);
return firstUrl.Scheme == secondUrl.Scheme &&
firstUrl.Port == secondUrl.Port &&
firstUrl.Host == firstUrl.Host;
}
Обратите внимание на последнюю строчку. Поле класса сравнивается само с собой. Кстати, здесь двойное притяжение ошибки. Во-первых, это функция сравнения. Во-вторых, мы имеем дело с эффектом последней строки. Бинго. Видимо, благодаря этому опечатка не была замечена в весьма простой короткой функции. Простота функции и то, что она скучная — это серьезное препятствие, чтобы уделить ей внимание на обзоре кода.
Но это ещё не предел того, в насколько короткой функции сравнения можно допустить опечатку. Проект Skia Graphics Engine (C++).
inline bool operator==(const SkPDFCanon::BitmapGlyphKey& u,
const SkPDFCanon::BitmapGlyphKey& v)
{
return memcmp(&u, &u, sizeof(SkPDFCanon::BitmapGlyphKey)) == 0;
}
Объект u побайтово сравнивается сам с собой.
Однако опечатки в функциях сравнения не ограничиваются чем-то, что сравнивается само с собой. Ошибки бывают разнообразнейшие. Например, интересный случай был найден в проекте Azure Service Fabric (С++).
template <typename TK, typename TV>
static bool MapCompare(const std::map<TK, TV>& lhs,
const std::map<TK, TV>& rhs)
{
if (lhs.size() != rhs.size()) { false; }
return std::equal(lhs.begin(), lhs.end(), rhs.begin());
}
Обратите внимание на одинокое слово false. Код компилируется, но не имеет смысла. Здесь забыли написать return. Правильный вариант:
if (lhs.size() != rhs.size()) { return false; }
Проект 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 &&
....
}
Здесь возможно возникновение NullReferenceException. На неравенство null нужно проверять ссылку otherResourceString, а не other.
С другими примерами ошибок вы можете познакомиться в статье "Зло живёт в функциях сравнения".
Если я скажу: "уделите функциям сравнения больше внимания на обзорах кода", то это вряд ли сработает. Поэтому подойду к этому вопросу более утилитарно.
Ещё стоит помнить, что, начиная с С++20, появился способ запросить компилятор сгенерировать код операторов сравнения: равенства, неравенства и отношения. Лишь в редких ситуациях вам придется писать собственные функции сравнения, следовательно, уменьшается вероятность допустить ошибку. Пользуйтесь современными возможностями языка C++!
Если функции сравнения программистам писать скучно, то к реализации функций копирования они подходят с излишним энтузиазмом. Я время от времени встречаю в таких функциях ошибки и, что интересно, текст функций кажется мне переусложнённым. Собственно, эта избыточная сложность и является источником ошибок.
Собственная реализация функции strdup в проекте Zephyr RTOS (С).
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
memcpy(cpy_mntpt, mntpt, strlen(mntpt));
}
return cpy_mntpt;
}
Функция memcpy копирует строчку, но не копирует терминальный ноль. Поэтому для копирования терминального нуля написан дополнительный код:
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
Но он не работает! Здесь опечатка, из-за которой терминальный ноль копируется сам в себя. Обратите внимание, что запись происходит в массив mntpt, а не в cpy_mntpt. В итоге функция mntpt_prepare возвращает строку, незавершенную терминальным нулём.
Правильный вариант:
((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';
Непонятно только, зачем код написан так запутанно и нестандартно. Его можно упростить до следующего варианта:
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
strcpy(cpy_mntpt, mntpt);
}
return cpy_mntpt;
}
Проект LFortran (С). Ошибка в функции конкатенации двух строк в новом буфере.
void _lfortran_strcat(char** s1, char** s2, char** dest)
{
int cntr = 0;
char trmn = '\0';
int s1_len = strlen(*s1);
int s2_len = strlen(*s2);
int trmn_size = strlen(&trmn);
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
for (int i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1);
cntr++;
}
for (int i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2);
cntr++;
}
dest_char[cntr] = trmn;
*dest = &(dest_char[0]);
}
Про некоторые ошибки сложно сказать, являются они опечатками или нет. Перед нами как раз такой случай.
Пожалуй, я всё-таки буду считать, что здесь опечатка. Программист использовал strlen вместо sizeof, чтобы определить размер терминального нуля в байтах.
Пояснение:
char trmn = '\0';
int trmn_size = strlen(&trmn);
Здесь символ trmn интерпретируется как пустая строка. Её длина нулевая. Соответственно, переменная trmn_size, название которой означает размер терминального нуля, всегда будет равна 0.
Нужно не считать длину пустой строки, а посчитать с помощью оператора sizeof, сколько байт занимает терминальный символ. Правильный код:
int trmn_size = sizeof(trmn);
Код рассмотренной функции можно улучшить, но это выходит за рамки этой статьи. По поводу других улучшений отсылаю к статье "Красивая ошибка в реализации функции конкатенации строк".
И последний пример ошибки возьму из проекта QuantConnect Lean (C#).
/// <summary>
/// Copy contents of the portfolio collection to a new destination.
/// </summary>
/// <remarks>
/// IDictionary implementation calling the underlying Securities collection
/// </remarks>
/// <param name="array">Destination array</param>
/// <param name="index">Position in array to start copying</param>
public void CopyTo(KeyValuePair<Symbol, SecurityHolding>[] array, int index)
{
array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];
var i = 0;
foreach (var asset in Securities)
{
if (i >= index)
{
array = new KeyValuePair<Symbol,SecurityHolding>(asset.Key,
asset.Value.Holdings);
}
i++;
}
}
Метод принимает коллекцию и сразу же перезаписывает ее значение. По комментарию и названию метода становится понятно, что в переданный массив должен скопироваться какой-то другой. Однако этого не произойдёт, и значение у array вне текущего метода останется неизмененным.
Это происходит из-за того, что аргумент array передан в метод по значению, а не по ссылке. Таким образом, после выполнения операции присвоения ссылку на новый объект будет хранить переменная array, доступная внутри метода. Значение переменной, переданной в метод, останется неизменным.
Это вариант опечатки, заключающийся в том, что забыли модификатор out. Правильный код:
public void CopyTo(out KeyValuePair<Symbol, SecurityHolding>[] array,
int index)
{
array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];
....
}
Например, я не стал упоминать про юнит-тесты, когда мы говорили о функциях сравнения. Проверять их юнит-тестами — неблагодарное занятие:
Функции копирования — другое дело. Посмотрите ещё раз на приведённые выше ошибки. Все они будут выявлены даже простейшим набором юнит-тестов, которые покажут, что на выходе функции возвращают не то, что надо.
Много опечаток встречается в коде, оперирующем со временем и датами. Эти ошибки очень разнообразны по своей природе, и мне трудно сформулировать, почему такой код притягивает баги. Поэтому я просто приведу несколько примеров. Предлагаю читателям самим сделать выводы.
Проект Umbraco (C#). Неправильно вызываются конструкторы класса DateTime.
public static DateTime TruncateTo(this DateTime dt,
DateTruncate truncateTo)
{
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 0, 0);
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 0);
....
}
Программист опечатался из-за того, что слишком привык всё нумеровать с нуля. Например, индексы массива. Но перед нами не массив, а даты. Первый день или месяц имеет номер 1, а не 0. Вызов конструкторов в рассмотренном коде приведёт к выбросу исключения ArgumentOutOfRangeException. Правильный вариант:
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 1, 1);
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 1);
Проект MPC-HC (C++). Может использоваться неинициализированная переменная.
void CSyncAP::RenderThread()
{
....
REFERENCE_TIME rtRefClockTimeNow;
if (m_pRefClock) {
m_pRefClock->GetTime(&rtRefClockTimeNow);
}
LONG lLastVsyncTime =
(LONG)((m_llEstVBlankTime - rtRefClockTimeNow) / 10000);
....
}
Если условие не выполняется, то переменная rtRefClockTimeNow остаётся неинициализированной. При этом она всё равно дальше будет использоваться в вычислениях. Видимо, забыли инициализировать переменную при объявлении дефолтным значением.
Проект Tizen (С). Запутались в представлении данных. Красивая опечатка.
static void preview_down_cb(....)
{
....
int delay = 0.5;
double fdelay;
fdelay = ((double)delay / 1000.0f);
DbgPrint("Long press: %lf\n", fdelay);
....
}
Записывать в переменную типа int значение 0.5 — плохая идея. На самом деле задержка отсчитывается в миллисекундах. Правильное дефолтное значение задержки в 500 миллисекунд должно быть записано так:
int delay = 500;
Проект MSBuild (C#). Неправильное приведение типа.
internal static void Trace(....)
{
....
long now = DateTime.UtcNow.Ticks;
float millisecondsSinceLastLog =
(float)((now - s_lastLoggedTicks)/10000L);
....
}
Происходит целочисленное деление, и только затем выполняется явное приведение к типу float. Приведение типа должно выполняться до деления, а не после. Опечатка в том, что запутались в скобках. Правильный код:
float millisecondsSinceLastLog = (float)(now - s_lastLoggedTicks)/10000L;
Проект Linux Kernel (С). Забыли запятую.
static ssize_t lp8788_show_eoc_time(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
"20min", "25min", "30min" "No timeout" };
....
}
Между предпоследним и последним строковым литералом нет запятой. Поэтому они слипаются, и получается "30minNo timeout".
Пожалуй, достаточно. Примеры некоторых других ошибок вы можете посмотреть в статье "31 февраля" и "Обработка дат притягивает ошибки или 77 дефектов в Qt 6".
Я продемонстрировал, сколь разнообразны ошибки, связанные с датами и временем. В ошибках нет какой-то закономерности. По крайней мере, я её не вижу.
Числа 0, 1, 2 очень часто используются в программировании в качестве индексов массивов или как часть имён переменных. Наличие таких чисел говорит об однотипности кода.
Раз код однотипен, то на нём сложно сосредоточиться. Как следствие, такой код притягивает различные опечатки. Это роднит рассматриваемый паттерн ошибок с эффектом последней строки и неверными сравнениями.
Проект LibreOffice (C++). Классическая Copy-Paste ошибка.
Sequence< OUString > FirebirdDriver::
getSupportedServiceNames_Static() throw (RuntimeException)
{
Sequence< OUString > aSNS( 2 );
aSNS[0] = "com.sun.star.sdbc.Driver";
aSNS[0] = "com.sun.star.sdbcx.Driver";
return aSNS;
}
Чтобы повторно не набирать скучную строчку, её скопировали. В продублированной строке заменили sdbc на sdbcx, но забыли поправить индекс массива.
Проект Quake III Arena (С). Ещё одна Copy-Paste ошибка.
int VL_FindAdjacentSurface(....)
{
....
if (fabs(dir[0]) > test->radius ||
fabs(dir[1]) > test->radius ||
fabs(dir[1]) > test->radius)
{
....
}
Здесь ещё проявляет себя и эффект последней строки. Комбо. Подобный код как магнит тянет к себе баги.
Проект OpenCOLLADA (C++). Более неожиданная опечатка (2 вместо 1).
struct short2
{
short values[2];
short2(short s1, short s2)
{
values[0] = s1;
values[2] = s2;
}
....
};
В этот раз есть пересечение с "ошибкой на единицу" (off-by-one error), которую мы рассмотрим в конце статьи.
Примечание для самого себя. Мы в очередной раз наблюдаем, как встречаясь, паттерны, усиливают вероятность появления ошибки. Это интересная тема. Возможно, стоит развить её в одной из будущих статей.
Проект .NET Core SDK (C#). Другой вариант опечатки.
public bool MatchesXmlType(IList<XPathItem> seq, int indexType)
{
....
typBase = typBase.Prime;
for (int i = 0; i < seq.Count; i++)
{
if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase))
return false;
}
return true;
}
В цикле всё время обрабатывается первый элемент массива seq. Правильный вариант:
if (!CreateXmlType(seq).IsSubtypeOf(typBase))
Проект Doom (C++).
uint AltOp::fixedLength()
{
uint l1 = exp1->fixedLength();
uint l2 = exp1->fixedLength();
if (l1 != l2 || l1 == ~0u)
return ~0;
return l1;
}
Скорее всего код писался с помощью Copy-Paste и во второй строке забыли заменить exp1 на exp2.
Проект Vangers (C++).
const char* iGetJoyBtnNameText(int vkey, int lang)
{
....
if (vkey >= VK_STICK_SWITCH_1 && vkey <= VK_STICK_SWITCH_9)
{
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1];
return ret;
}
....
}
Обратный случай. Правильный вариант:
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch1[vkey - VK_STICK_SWITCH_1];
Проект Boost (C++).
point3D operator/(const point3D &p1, const point3D &p2)
{
return point3D(p1.x/p2.x, p1.y/p2.y, p1.z/p1.z);
}
Опечатка здесь: p1.z/p1.z.
Проект Azure PowerShell (C#). Ошибка форматирования.
protected override void ProcessRecordInternal()
{
....
if (this.ShouldProcess(this.Name,
string.Format("Creating Log Alert Rule '{0}' in resource group {0}",
this.Name, this.ResourceGroupName)))
....
}
Правильный вариант:
string.Format("Creating Log Alert Rule '{0}' in resource group {1}",
this.Name, this.ResourceGroupName))
Думаю, мы рассмотрели достаточно примеров. Если хочется ещё больше, то предлагаю заглянуть в статью "Ноль, один, два, Фредди заберёт тебя".
Если приходится использовать числа в именах переменных или для индексации массивов, то, скорее всего, такой код будет казаться скучным. Конечно, этот код нужен и поэтому в любом случае будет создан программистом. Но, к сожалению, сложно сосредотачиваться на его написании и проверке.
Давайте ещё раз взглянем вот на это:
point3D operator/(const point3D &p1, const point3D &p2)
{
return point3D(p1.x/p2.x, p1.y/p2.y, p1.z/p1.z);
}
Нельзя сказать, что код сложен. Как раз наоборот, он слишком прост. Не получается педантично подойти к его созданию. Вместо этого хочется думать об алгоритмах и о том, как написать красивее какой-то класс. Результат перед нами.
Это давно известный паттерн ошибок. Поэтому не буду приписывать себе его открытие. Он давно описан:
Чаще всего ошибки проявляются в выходе на один элемент за границу массивов. Думаю, каждый сталкивался с ними, начиная изучать программирование. Это те самые случаи, когда вас сбивало с толку, что элементы в массиве нумеруются с нуля, а не с единицы.
Итак, ошибки этого типа давно всем известны. Однако, даже не зная заранее про этот паттерн ошибок, думаю, я смог его обнаружить. К сожалению, даже опытные программисты продолжают совершать такие ошибки, и я регулярно встречаю их в коде проектов. Приведу пару примеров.
Проект Umbraco (C#). Опечатка в индексе.
protected virtual string VisitMethodCall(MethodCallExpression m)
{
....
if (m.Arguments.Count == 2)
{
var n1 = Visit(m.Arguments[0]);
var f = m.Arguments[2];
....
}
Правильный код:
var f = m.Arguments[1];
Кстати, этот код можно было отнести и к главе про числа 0, 1, 2. Думаю, вас это уже не удивляет. Взаимное притяжение разных паттернов ошибок.
Проект CMake (С). Опечатка при проверке диапазона значений.
static int64_t
expand(struct archive_read *a, int64_t end)
{
....
if ((lensymbol = read_next_symbol(a, &rar->lengthcode)) < 0)
goto bad_data;
if (lensymbol > (int)(sizeof(lengthbases)/sizeof(lengthbases[0])))
goto bad_data;
....
len = lengthbases[lensymbol] + 2;
....
}
При обращении к массиву lengthbases возможен выход за границу массива, так как выше написали оператор '>' вместо '>='. Правильный вариант проверки:
if (lensymbol >= (int)(sizeof(lengthbases)/sizeof(lengthbases[0])))
Проект ELKI (Java). Опечатка в индексе.
public List<double[]> points;
@Override
public double[] computeMean() {
// Not supported except for singletons.
return points.size() == 1 ? points.get(1) : null;
}
Правильный код:
return points.size() == 1 ? points.get(0) : null;
Спасибо за внимание. Если вы знаете какие-то аналогичные ошибочные паттерны, прошу рассказать про них в комментариях. Во-первых, вы предупредите коллег по цеху. Во-вторых, возможно, мы сможем придумать новые правила для статического анализатора PVS-Studio, чтобы помочь выявлять такие ошибки.
Возможно, я тоже замечу какие-то новые паттерны и опишу их будущих статьях. Чтобы не попустить что-то интересное, приглашаю подписаться на ежемесячную рассылку.
Все баги, описанные в этой статье, были найдены с помощью PVS-Studio. Этот инструмент поможет вам быстро выявлять в новом коде опечатки и другие дефекты, тем самым сокращая стоимость их исправления. Попробуйте бесплатную триальную версию: скачать PVS-Studio.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Common patterns of typos in programming.
habr.com
На чём основаны наблюдения
С целью тестирования и продвижения статического анализатора кода PVS-Studio мы проверяем различные открытые проекты. Найдя ошибки, мы сообщаем о них авторам проектов, коллекционируем их и пишем статьи про наиболее интересные случаи.
Рассматривая все эти ошибки, я постепенно замечаю различные повторяющиеся паттерны опечаток. За редким исключением они не зависят от языка программирования. По крайней мере, они одновременно свойственны коду, написанному на C, C++, C#, Java. В этой статье я опишу 7 паттернов, которые заметил к настоящему моменту:
- Эффект последней строки.
- Злополучная функция memset.
- Неверные функции сравнения.
- Неверные функции копирования.
- Ошибки работы с датами и временем.
- Несчастливые числа: 0, 1, 2.
- Ошибка на единицу (off-by-one error).
Заметность закономерностей в ошибках свидетельствует о том, что они крайне распространены. Полезно знать о них, чтобы избегать написания потенциально опасного кода или более эффективно находить их в процессе обзоров кода. Другим словами, вы узнаете, какой код притягивает ошибки, и будете более внимательно его проверять. Конечно, PVS-Studio способен выявить многие подобные ошибки, но не все. Поэтому дополнительное внимание не повредит.
Эффект последней строки
Самый первый замеченный мною паттерн. Впервые я описывал его в статье ещё в 2014 году. Формулировка: при написании однотипных блоков кода наибольшая вероятность допустить ошибку будет в последнем блоке.
Поясню на примере кода из проекта Godot Engine (C++).
String SoftBody::get_configuration_warning() const {
....
Transform t = get_transform();
if ((ABS(t.basis.get_axis(0).length() - 1.0) > 0.05 ||
ABS(t.basis.get_axis(1).length() - 1.0) > 0.05 ||
ABS(t.basis.get_axis(0).length() - 1.0) > 0.05)) {
if (!warning.empty())
....
}
Перед нами классическая Copy-Paste ошибка. Программисту не хочется заново набирать похожие строчки кода. Поэтому он берёт и дважды дублирует строчку кода:
ABS(t.basis.get_axis(0).length() - 1.0) > 0.05
После чего он заменяет во второй строке 0 на 1, а вот в последней строке заменить 0 на 2 он забывает. Когда отдельно рассматривается такой фрагмент кода, кажется удивительным, что допущена такая простая ошибка. Но именно так и выглядит этот паттерн.
Другой пример ошибки, найденный в юнит-тестах проекта LLVM (C++).
TEST(RegisterContextMinidump, ConvertMinidumpContext_x86_64) {
MinidumpContext_x86_64 Context;
....
Context.rax = 0x0001020304050607;
Context.rbx = 0x08090a0b0c0d0e0f;
....
Context.eflags = 0x88898a8b;
Context.cs = 0x8c8d;
Context.fs = 0x8e8f;
Context.gs = 0x9091;
Context.ss = 0x9293; // <=
Context.ds = 0x9495;
Context.ss = 0x9697; // <=
llvm::ArrayRef<uint8_t> ContextRef(reinterpret_cast<uint8_t *>(&Context),
sizeof(Context));
....
}
Обратите внимание на строчки кода, выделенные комментариями. Этот код вряд ли писался с помощью Copy-Paste, но ошибка всё равно в последней строке. Программист поспешил и по невнимательности допустил опечатку, повторно записав значение в регистр ss, а не в es.
Примечание. Ошибка интересна ещё и тем, что показывает, как статический анализатор дополняет методологию юнит-тестирования. В юнит-тестах тоже могут быть ошибки, из-за которых какие-то случаи не тестируются или тестируются не полностью. Но ведь никто не будет писать юнит-тесты на юнит-тесты. Здесь помогут анализаторы кода.
Чтобы не создавалось впечатления, что подобные баги свойственны только языкам C и C++, приведу код из мира Java. Следующую опечатка замечена мною в IntelliJ IDEA Community Edition (Java).
private static boolean isBeforeOrAfterKeyword(String str, boolean trimKeyword) {
return (trimKeyword ? LoadingOrder.BEFORE_STR.trim() :
LoadingOrder.BEFORE_STR).equalsIgnoreCase(str) ||
(trimKeyword ? LoadingOrder.AFTER_STR.trim() :
LoadingOrder.AFTER_STR).equalsIgnoreCase(str) ||
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) || // <=
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str); // <=
}
Перед нами явная закономерность. С другими схожими примерами вы можете познакомиться, открыв старую статью "Эффект последней строки".
Почему этому паттерну я дал такое название? Он навеян ассоциацией с "эффектом последних метров" из мира альпинизма. Я читал про наблюдение, что альпинисты часто допускают ошибку в конце подъема. Причём причина не в усталости, а в том, что они становятся менее сосредоточенными. Кажется, что дело уже сделано, скоро можно будет отдохнуть и порадоваться. Альпинист теряет внимательность, спешит закончить подъём и делает что-то не так.
При написании кода происходит что-то похожее. Писать однотипные блоки кода скучно. Мы найдём подтверждение этому ниже, когда будем говорить о функциях сравнения. Поэтому человек торопится закончить монотонную работу. При написании последней строки кода программист радуется, что заканчивает с рутиной, и, возможно, уже начинает думать том, что напишет далее.
В любом случае внимание ослабевает, и программист допускает ошибку. Конечно, он может ошибиться в другом месте, но наблюдения говорят, что вероятнее всего, это произойдёт в последнем блоке однотипного кода.
С обзором кода происходит аналогичная история. Скучно проверять однотипный код. Поэтому внимание быстро снижается, и последние строчки кода будут проверены поверхностно или вообще не будут проверены. Вот так в коде возникают и остаются незамеченными ошибки, которые удивляют своей простотой, когда обнаруживаются.
Злополучная функция memset
Помимо универсальных паттернов, существуют паттерны, свойственные определённому языку. Например, огромное количество ошибок связано с использованием функции memset. В основном она используется в программах на C, но её нередко можно встретить и в C++ коде.
void* memset( void* dest, int ch, size_t count ); // С
void* std::memset( void* dest, int ch, std::size_t count ); // С++
Функция заполняет count первых байтов буфера, на который указывает dest, значением (unsigned char) ch. Описание:
- C вариант — memset.
- C++ вариант — std::memset.
На мой взгляд, эта функция — чемпион С и C++ мира по притягиванию к себе багов. Чаще всего ошибка связана с неправильным вычислением размера заполняемого буфера.
Опечатки заключаются в использовании лишних операторов разыменования * или, наоборот, в их отсутствии там, где надо. Или в лишних операторах взятия адреса &. Сейчас мы рассмотрим примеры таких ошибок.
Забыли разыменовать указатель *. Фрагмент кода FAR Manager v2 for Linux (Far2l, C++). Из-за опечатки оператор sizeof вычисляет не размер структуры, а размер указателя.
int64_t FileList::VMProcess(int OpCode,
void *vParam,
int64_t iParam)
{
....
PluginInfo *PInfo = (PluginInfo *)vParam;
memset(PInfo, 0, sizeof(PInfo));
....
}
Правильный вариант:
memset(PInfo, 0, sizeof(*PInfo));
Лишний оператор взятия адреса &. Код из проекта Energy Checker SDK (С). Оператор sizeof вычисляет не размер структуры WIN32_FIND_DATA, а размер указателя. В результате, обнулены будут только первые 4 или 8 байт структуры в зависимости от того, собирается Win32 или Win64 приложение.
int plh_read_pl_folder(PPLH_PL_FOLDER_INFO pconfig) {
....
WIN32_FIND_DATA file_data;
....
memset(
&file_data,
0,
sizeof(&file_data)
);
....
}
Должно быть написано: sizeof(file_data). Ещё лучше вообще избавиться от вызова функции memset, инициализируя объект нулями ещё на этапе объявления:
WIN32_FIND_DATA file_data = { };
Перепутали & и *. Проект Wolfenstein 3D (С). Здесь вместо взятия адреса наоборот нужно было разыменовать указатель, чтобы правильно вычислить размер структуры.
void CG_RegisterItemVisuals( int itemNum ) {
....
itemInfo_t *itemInfo;
....
memset( itemInfo, 0, sizeof( &itemInfo ) );
....
}
Переполнение буфера. В проекте Open source emulator PS4 (C++), автор кода случайно спутал sizeof c countof. Он подумал, что sizeof вычислит количество элементов и поэтому умножил это значение на размер одного элемента.
struct GnmCmdPSShader
{
....
uint32_t reserved[27];
};
int PS4API sceGnmSetPsShader350(....)
{
....
memset(param->reserved, 0, sizeof(param->reserved) * sizeof(uint32_t));
return SCE_OK;
}
Правильный вариант кода:
memset(param->reserved, 0, sizeof(param->reserved));
Примечание. Кстати, функция memset притягивает к себе не только опечатки, но и другие ошибки. Можете взглянуть на них в статье "Самая опасная функция в мире С/С++". Например, встречаются потенциальные уязвимости, когда функцию memset используют в конце, чтобы затереть приватные данные. Пример из проекта WebRTC (C++).
void AsyncSocksProxySocket::SendAuth() {
....
char * sensitive = new char[len];
pass_.CopyTo(sensitive, true);
request.WriteString(sensitive); // Password
memset(sensitive, 0, len);
delete [] sensitive;
....
}
Дело в том, что в целях оптимизации компиляторы удалят этот вызов функции memset. С точки зрения компилятора, раз буфер освобождается, то его можно не заполнять. В результате приватные данные останутся в памяти. Эта тема выходит за рамки статьи про опечатки, поэтому если хочется подробностей, то предлагаю познакомиться со следующими ссылками:
- V597: диагностика PVS-Studio.
- CWE-14: Compiler Removal of Code to Clear Buffers.
- Безопасная очистка приватных данных.
Очень часто функция memset используется при инициализации для заполнения всех полей структуры нулевыми значениями. Абстрактный пример:
MyStruct foo;
memset(foo, 0, sizeof(foo));
Проще и безопаснее обнулять структуры сразу при их объявлении:
MyStruct foo = { };
Если следует обнулить массив в C++, то лучше прибегнуть к функции std::fill. Поясню это на примере ошибки, найденной мною в проекте Notepad++ (C++).
#define CONT_MAP_MAX 50
int _iContMap[CONT_MAP_MAX];
....
DockingManager:
ockingManager(){
....
memset(_iContMap, -1, CONT_MAP_MAX);
....
}
Программист думает, что инициализирует массив из 50 элементов типа int, значением -1. Но функция memset работает с байтами. На самом деле первые 12 элементов будут проинициализированы значением 0xFFFFFFFF. Фактически они будут заполнены правильным значением -1, но это просто везение. Ещё один элемент получит значение 0x0000FFFF. Остальные элементы массива останутся неинициализированными.
В случае использования std::fill какую-то ошибку совершить гораздо сложнее:
// since C++11
std::fill(std::begin(_iContMap), std::end(_iContMap), -1);
// since C++20
std::ranges::fill(_iContMap, -1);
Неверные функции сравнения
Людям скучно писать функции сравнения. По факту это вспомогательный код, который прост и неинтересен. Как говорится, там нет места творчеству. Нужно просто сравнить все нужные переменные между собой.
Из-за этого программисты торопятся при написании таких функций. Торопятся они и при их проверке на обзорах кода. Тяжело сосредоточить внимание на таком однотипном простом коде. Подобные функции на обзоре кода просматриваются "по диагонали". Результатом этого является большое количество незамеченных опечаток.
Начнём с классической опечатки в большом блоке однотипных сравнений. Проект Apache Flink (Java).
@Override
public boolean equals(Object o)
{
....
CheckpointStatistics that = (CheckpointStatistics) o;
return id == that.id &&
savepoint == that.savepoint &&
triggerTimestamp == that.triggerTimestamp &&
latestAckTimestamp == that.latestAckTimestamp &&
stateSize == that.stateSize &&
duration == that.duration &&
alignmentBuffered == that.alignmentBuffered &&
processedData == processedData &&
persistedData == that.persistedData &&
numSubtasks == that.numSubtasks &&
numAckSubtasks == that.numAckSubtasks &&
status == that.status &&
Objects.equals(checkpointType, that.checkpointType) &&
Objects.equals(
checkpointStatisticsPerTask,
that.checkpointStatisticsPerTask);
}
Нашли ошибку? Думаю, нет. Такой код скучно и писать, и читать. Скорее всего и вы поленились его внимательно изучить. Вот строчка с опечаткой:
processedData == processedData
Переменная сравнивается сама с собой. Правильный вариант:
processedData == that.processedData
Аналогичные опечатки можно встреть в куда более маленьких функциях. Проект eShopOnContainers (C#).
private bool CheckSameOrigin(string urlHook, string url)
{
var firstUrl = new Uri(urlHook, UriKind.Absolute);
var secondUrl = new Uri(url, UriKind.Absolute);
return firstUrl.Scheme == secondUrl.Scheme &&
firstUrl.Port == secondUrl.Port &&
firstUrl.Host == firstUrl.Host;
}
Обратите внимание на последнюю строчку. Поле класса сравнивается само с собой. Кстати, здесь двойное притяжение ошибки. Во-первых, это функция сравнения. Во-вторых, мы имеем дело с эффектом последней строки. Бинго. Видимо, благодаря этому опечатка не была замечена в весьма простой короткой функции. Простота функции и то, что она скучная — это серьезное препятствие, чтобы уделить ей внимание на обзоре кода.
Но это ещё не предел того, в насколько короткой функции сравнения можно допустить опечатку. Проект Skia Graphics Engine (C++).
inline bool operator==(const SkPDFCanon::BitmapGlyphKey& u,
const SkPDFCanon::BitmapGlyphKey& v)
{
return memcmp(&u, &u, sizeof(SkPDFCanon::BitmapGlyphKey)) == 0;
}
Объект u побайтово сравнивается сам с собой.
Однако опечатки в функциях сравнения не ограничиваются чем-то, что сравнивается само с собой. Ошибки бывают разнообразнейшие. Например, интересный случай был найден в проекте Azure Service Fabric (С++).
template <typename TK, typename TV>
static bool MapCompare(const std::map<TK, TV>& lhs,
const std::map<TK, TV>& rhs)
{
if (lhs.size() != rhs.size()) { false; }
return std::equal(lhs.begin(), lhs.end(), rhs.begin());
}
Обратите внимание на одинокое слово false. Код компилируется, но не имеет смысла. Здесь забыли написать return. Правильный вариант:
if (lhs.size() != rhs.size()) { return false; }
Проект 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 &&
....
}
Здесь возможно возникновение NullReferenceException. На неравенство null нужно проверять ссылку otherResourceString, а не other.
С другими примерами ошибок вы можете познакомиться в статье "Зло живёт в функциях сравнения".
Если я скажу: "уделите функциям сравнения больше внимания на обзорах кода", то это вряд ли сработает. Поэтому подойду к этому вопросу более утилитарно.
Про табличное форматирование кода я подробно писал в мини-книге "60 антипаттернов для С++ программиста". Смотрите двадцатый вредный совет "Компактный код". Там вначале написано, как не надо делать, а потом как надо. Табличное форматирование не гарантирует отсутствия опечаток, но однозначно сокращает их количество.
Ещё стоит помнить, что, начиная с С++20, появился способ запросить компилятор сгенерировать код операторов сравнения: равенства, неравенства и отношения. Лишь в редких ситуациях вам придется писать собственные функции сравнения, следовательно, уменьшается вероятность допустить ошибку. Пользуйтесь современными возможностями языка C++!
Неверные функции копирования
Если функции сравнения программистам писать скучно, то к реализации функций копирования они подходят с излишним энтузиазмом. Я время от времени встречаю в таких функциях ошибки и, что интересно, текст функций кажется мне переусложнённым. Собственно, эта избыточная сложность и является источником ошибок.
Собственная реализация функции strdup в проекте Zephyr RTOS (С).
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
memcpy(cpy_mntpt, mntpt, strlen(mntpt));
}
return cpy_mntpt;
}
Функция memcpy копирует строчку, но не копирует терминальный ноль. Поэтому для копирования терминального нуля написан дополнительный код:
((u8_t *)mntpt)[strlen(mntpt)] = '\0';
Но он не работает! Здесь опечатка, из-за которой терминальный ноль копируется сам в себя. Обратите внимание, что запись происходит в массив mntpt, а не в cpy_mntpt. В итоге функция mntpt_prepare возвращает строку, незавершенную терминальным нулём.
Правильный вариант:
((u8_t *)cpy_mntpt)[strlen(mntpt)] = '\0';
Непонятно только, зачем код написан так запутанно и нестандартно. Его можно упростить до следующего варианта:
static char *mntpt_prepare(char *mntpt)
{
char *cpy_mntpt;
cpy_mntpt = k_malloc(strlen(mntpt) + 1);
if (cpy_mntpt) {
strcpy(cpy_mntpt, mntpt);
}
return cpy_mntpt;
}
Проект LFortran (С). Ошибка в функции конкатенации двух строк в новом буфере.
void _lfortran_strcat(char** s1, char** s2, char** dest)
{
int cntr = 0;
char trmn = '\0';
int s1_len = strlen(*s1);
int s2_len = strlen(*s2);
int trmn_size = strlen(&trmn);
char* dest_char = (char*)malloc(s1_len+s2_len+trmn_size);
for (int i = 0; i < s1_len; i++) {
dest_char[cntr] = (*s1);
cntr++;
}
for (int i = 0; i < s2_len; i++) {
dest_char[cntr] = (*s2);
cntr++;
}
dest_char[cntr] = trmn;
*dest = &(dest_char[0]);
}
Про некоторые ошибки сложно сказать, являются они опечатками или нет. Перед нами как раз такой случай.
Пожалуй, я всё-таки буду считать, что здесь опечатка. Программист использовал strlen вместо sizeof, чтобы определить размер терминального нуля в байтах.
Пояснение:
char trmn = '\0';
int trmn_size = strlen(&trmn);
Здесь символ trmn интерпретируется как пустая строка. Её длина нулевая. Соответственно, переменная trmn_size, название которой означает размер терминального нуля, всегда будет равна 0.
Нужно не считать длину пустой строки, а посчитать с помощью оператора sizeof, сколько байт занимает терминальный символ. Правильный код:
int trmn_size = sizeof(trmn);
Код рассмотренной функции можно улучшить, но это выходит за рамки этой статьи. По поводу других улучшений отсылаю к статье "Красивая ошибка в реализации функции конкатенации строк".
И последний пример ошибки возьму из проекта QuantConnect Lean (C#).
/// <summary>
/// Copy contents of the portfolio collection to a new destination.
/// </summary>
/// <remarks>
/// IDictionary implementation calling the underlying Securities collection
/// </remarks>
/// <param name="array">Destination array</param>
/// <param name="index">Position in array to start copying</param>
public void CopyTo(KeyValuePair<Symbol, SecurityHolding>[] array, int index)
{
array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];
var i = 0;
foreach (var asset in Securities)
{
if (i >= index)
{
array = new KeyValuePair<Symbol,SecurityHolding>(asset.Key,
asset.Value.Holdings);
}
i++;
}
}
Метод принимает коллекцию и сразу же перезаписывает ее значение. По комментарию и названию метода становится понятно, что в переданный массив должен скопироваться какой-то другой. Однако этого не произойдёт, и значение у array вне текущего метода останется неизмененным.
Это происходит из-за того, что аргумент array передан в метод по значению, а не по ссылке. Таким образом, после выполнения операции присвоения ссылку на новый объект будет хранить переменная array, доступная внутри метода. Значение переменной, переданной в метод, останется неизменным.
Это вариант опечатки, заключающийся в том, что забыли модификатор out. Правильный код:
public void CopyTo(out KeyValuePair<Symbol, SecurityHolding>[] array,
int index)
{
array = new KeyValuePair<Symbol, SecurityHolding>[Securities.Count];
....
}
Конечно, юнит-тесты полезно писать и для нахождения ошибок других разновидностей. Однако здесь они более уместны.
Например, я не стал упоминать про юнит-тесты, когда мы говорили о функциях сравнения. Проверять их юнит-тестами — неблагодарное занятие:
- Скучно писать юнит-тесты для функций сравнения. А мы уже знаем, что скучный код редко бывает удачным .
- В юнит-тестах потребуется рассмотреть множество частных случаев. Например, нужно отдельно проверить корректность сравнения каждого члена класса по отдельности. Код юнит-тестов будет в десятки раз больше, чем функций сравнения. Таким образом, покрытие тестами может оказаться непрактичным с точки зрения соотношения польза/затраченные ресурсы.
Функции копирования — другое дело. Посмотрите ещё раз на приведённые выше ошибки. Все они будут выявлены даже простейшим набором юнит-тестов, которые покажут, что на выходе функции возвращают не то, что надо.
Ошибки работы с датами и временем
Много опечаток встречается в коде, оперирующем со временем и датами. Эти ошибки очень разнообразны по своей природе, и мне трудно сформулировать, почему такой код притягивает баги. Поэтому я просто приведу несколько примеров. Предлагаю читателям самим сделать выводы.
Проект Umbraco (C#). Неправильно вызываются конструкторы класса DateTime.
public static DateTime TruncateTo(this DateTime dt,
DateTruncate truncateTo)
{
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 0, 0);
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 0);
....
}
Программист опечатался из-за того, что слишком привык всё нумеровать с нуля. Например, индексы массива. Но перед нами не массив, а даты. Первый день или месяц имеет номер 1, а не 0. Вызов конструкторов в рассмотренном коде приведёт к выбросу исключения ArgumentOutOfRangeException. Правильный вариант:
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 1, 1);
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 1);
Проект MPC-HC (C++). Может использоваться неинициализированная переменная.
void CSyncAP::RenderThread()
{
....
REFERENCE_TIME rtRefClockTimeNow;
if (m_pRefClock) {
m_pRefClock->GetTime(&rtRefClockTimeNow);
}
LONG lLastVsyncTime =
(LONG)((m_llEstVBlankTime - rtRefClockTimeNow) / 10000);
....
}
Если условие не выполняется, то переменная rtRefClockTimeNow остаётся неинициализированной. При этом она всё равно дальше будет использоваться в вычислениях. Видимо, забыли инициализировать переменную при объявлении дефолтным значением.
Проект Tizen (С). Запутались в представлении данных. Красивая опечатка.
static void preview_down_cb(....)
{
....
int delay = 0.5;
double fdelay;
fdelay = ((double)delay / 1000.0f);
DbgPrint("Long press: %lf\n", fdelay);
....
}
Записывать в переменную типа int значение 0.5 — плохая идея. На самом деле задержка отсчитывается в миллисекундах. Правильное дефолтное значение задержки в 500 миллисекунд должно быть записано так:
int delay = 500;
Проект MSBuild (C#). Неправильное приведение типа.
internal static void Trace(....)
{
....
long now = DateTime.UtcNow.Ticks;
float millisecondsSinceLastLog =
(float)((now - s_lastLoggedTicks)/10000L);
....
}
Происходит целочисленное деление, и только затем выполняется явное приведение к типу float. Приведение типа должно выполняться до деления, а не после. Опечатка в том, что запутались в скобках. Правильный код:
float millisecondsSinceLastLog = (float)(now - s_lastLoggedTicks)/10000L;
Проект Linux Kernel (С). Забыли запятую.
static ssize_t lp8788_show_eoc_time(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
"20min", "25min", "30min" "No timeout" };
....
}
Между предпоследним и последним строковым литералом нет запятой. Поэтому они слипаются, и получается "30minNo timeout".
Пожалуй, достаточно. Примеры некоторых других ошибок вы можете посмотреть в статье "31 февраля" и "Обработка дат притягивает ошибки или 77 дефектов в Qt 6".
Я продемонстрировал, сколь разнообразны ошибки, связанные с датами и временем. В ошибках нет какой-то закономерности. По крайней мере, я её не вижу.
Несчастливые числа: 0, 1, 2
Числа 0, 1, 2 очень часто используются в программировании в качестве индексов массивов или как часть имён переменных. Наличие таких чисел говорит об однотипности кода.
Раз код однотипен, то на нём сложно сосредоточиться. Как следствие, такой код притягивает различные опечатки. Это роднит рассматриваемый паттерн ошибок с эффектом последней строки и неверными сравнениями.
Начнём с опечаток в индексах
Проект LibreOffice (C++). Классическая Copy-Paste ошибка.
Sequence< OUString > FirebirdDriver::
getSupportedServiceNames_Static() throw (RuntimeException)
{
Sequence< OUString > aSNS( 2 );
aSNS[0] = "com.sun.star.sdbc.Driver";
aSNS[0] = "com.sun.star.sdbcx.Driver";
return aSNS;
}
Чтобы повторно не набирать скучную строчку, её скопировали. В продублированной строке заменили sdbc на sdbcx, но забыли поправить индекс массива.
Проект Quake III Arena (С). Ещё одна Copy-Paste ошибка.
int VL_FindAdjacentSurface(....)
{
....
if (fabs(dir[0]) > test->radius ||
fabs(dir[1]) > test->radius ||
fabs(dir[1]) > test->radius)
{
....
}
Здесь ещё проявляет себя и эффект последней строки. Комбо. Подобный код как магнит тянет к себе баги.
Проект OpenCOLLADA (C++). Более неожиданная опечатка (2 вместо 1).
struct short2
{
short values[2];
short2(short s1, short s2)
{
values[0] = s1;
values[2] = s2;
}
....
};
В этот раз есть пересечение с "ошибкой на единицу" (off-by-one error), которую мы рассмотрим в конце статьи.
Примечание для самого себя
. Мы в очередной раз наблюдаем, как встречаясь, паттерны, усиливают вероятность появления ошибки. Это интересная тема. Возможно, стоит развить её в одной из будущих статей.Проект .NET Core SDK (C#). Другой вариант опечатки.
public bool MatchesXmlType(IList<XPathItem> seq, int indexType)
{
....
typBase = typBase.Prime;
for (int i = 0; i < seq.Count; i++)
{
if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase))
return false;
}
return true;
}
В цикле всё время обрабатывается первый элемент массива seq. Правильный вариант:
if (!CreateXmlType(seq).IsSubtypeOf(typBase))
Теперь посмотрим на опечатки в именах переменных
Проект Doom (C++).
uint AltOp::fixedLength()
{
uint l1 = exp1->fixedLength();
uint l2 = exp1->fixedLength();
if (l1 != l2 || l1 == ~0u)
return ~0;
return l1;
}
Скорее всего код писался с помощью Copy-Paste и во второй строке забыли заменить exp1 на exp2.
Проект Vangers (C++).
const char* iGetJoyBtnNameText(int vkey, int lang)
{
....
if (vkey >= VK_STICK_SWITCH_1 && vkey <= VK_STICK_SWITCH_9)
{
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1];
return ret;
}
....
}
Обратный случай. Правильный вариант:
ret = (lang)
? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1]
: iJoystickStickSwitch1[vkey - VK_STICK_SWITCH_1];
Проект Boost (C++).
point3D operator/(const point3D &p1, const point3D &p2)
{
return point3D(p1.x/p2.x, p1.y/p2.y, p1.z/p1.z);
}
Опечатка здесь: p1.z/p1.z.
Другие варианты опечаток
Проект Azure PowerShell (C#). Ошибка форматирования.
protected override void ProcessRecordInternal()
{
....
if (this.ShouldProcess(this.Name,
string.Format("Creating Log Alert Rule '{0}' in resource group {0}",
this.Name, this.ResourceGroupName)))
....
}
Правильный вариант:
string.Format("Creating Log Alert Rule '{0}' in resource group {1}",
this.Name, this.ResourceGroupName))
Думаю, мы рассмотрели достаточно примеров. Если хочется ещё больше, то предлагаю заглянуть в статью "Ноль, один, два, Фредди заберёт тебя".
Если приходится использовать числа в именах переменных или для индексации массивов, то, скорее всего, такой код будет казаться скучным. Конечно, этот код нужен и поэтому в любом случае будет создан программистом. Но, к сожалению, сложно сосредотачиваться на его написании и проверке.
Давайте ещё раз взглянем вот на это:
point3D operator/(const point3D &p1, const point3D &p2)
{
return point3D(p1.x/p2.x, p1.y/p2.y, p1.z/p1.z);
}
Нельзя сказать, что код сложен. Как раз наоборот, он слишком прост. Не получается педантично подойти к его созданию. Вместо этого хочется думать об алгоритмах и о том, как написать красивее какой-то класс. Результат перед нами.
Ошибка на единицу (off-by-one error)
Это давно известный паттерн ошибок. Поэтому не буду приписывать себе его открытие. Он давно описан:
- Wikipedia. Ошибка на единицу.
- Common Weakness Enumeration. CWE-193: Off-by-one Error.
Чаще всего ошибки проявляются в выходе на один элемент за границу массивов. Думаю, каждый сталкивался с ними, начиная изучать программирование. Это те самые случаи, когда вас сбивало с толку, что элементы в массиве нумеруются с нуля, а не с единицы.
Итак, ошибки этого типа давно всем известны. Однако, даже не зная заранее про этот паттерн ошибок, думаю, я смог его обнаружить. К сожалению, даже опытные программисты продолжают совершать такие ошибки, и я регулярно встречаю их в коде проектов. Приведу пару примеров.
Проект Umbraco (C#). Опечатка в индексе.
protected virtual string VisitMethodCall(MethodCallExpression m)
{
....
if (m.Arguments.Count == 2)
{
var n1 = Visit(m.Arguments[0]);
var f = m.Arguments[2];
....
}
Правильный код:
var f = m.Arguments[1];
Кстати, этот код можно было отнести и к главе про числа 0, 1, 2. Думаю, вас это уже не удивляет. Взаимное притяжение разных паттернов ошибок.
Проект CMake (С). Опечатка при проверке диапазона значений.
static int64_t
expand(struct archive_read *a, int64_t end)
{
....
if ((lensymbol = read_next_symbol(a, &rar->lengthcode)) < 0)
goto bad_data;
if (lensymbol > (int)(sizeof(lengthbases)/sizeof(lengthbases[0])))
goto bad_data;
....
len = lengthbases[lensymbol] + 2;
....
}
При обращении к массиву lengthbases возможен выход за границу массива, так как выше написали оператор '>' вместо '>='. Правильный вариант проверки:
if (lensymbol >= (int)(sizeof(lengthbases)/sizeof(lengthbases[0])))
Проект ELKI (Java). Опечатка в индексе.
public List<double[]> points;
@Override
public double[] computeMean() {
// Not supported except for singletons.
return points.size() == 1 ? points.get(1) : null;
}
Правильный код:
return points.size() == 1 ? points.get(0) : null;
Поделитесь своими наблюдениями!
Спасибо за внимание. Если вы знаете какие-то аналогичные ошибочные паттерны, прошу рассказать про них в комментариях. Во-первых, вы предупредите коллег по цеху. Во-вторых, возможно, мы сможем придумать новые правила для статического анализатора PVS-Studio, чтобы помочь выявлять такие ошибки.
Возможно, я тоже замечу какие-то новые паттерны и опишу их будущих статьях. Чтобы не попустить что-то интересное, приглашаю подписаться на ежемесячную рассылку.
PVS-Studio
Все баги, описанные в этой статье, были найдены с помощью PVS-Studio. Этот инструмент поможет вам быстро выявлять в новом коде опечатки и другие дефекты, тем самым сокращая стоимость их исправления. Попробуйте бесплатную триальную версию: скачать PVS-Studio.
Дополнительные ссылки
- Коллекция ошибок.
- Эффект последней строки.
- Объяснение эффекта последней строки.
- Самая опасная функция в мире С/С++.
- Зло живёт в функциях сравнения.
- Начало коллекционирования ошибок в функциях копирования.
- Красивая ошибка в реализации функции конкатенации строк.
- 31 февраля.
- Обработка дат притягивает ошибки или 77 дефектов в Qt 6.
- Ноль, один, два, Фредди заберёт тебя.
- 60 антипаттернов для С++ программиста.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Common patterns of typos in programming.
Распространённые паттерны опечаток при программировании
Есть бесконечное количество способов ошибиться при написании кода. Однако иногда можно заметить явные интересные закономерности, как и где ошибаются программисты. Поговорим о коде, который...
habr.com