Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!
Привет! Несмотря на то, что сезон конференций 2019 года ещё в самом разгаре, мы бы хотели обсудить задачи, которые ранее предлагали посетителям нашего стенда. Осень 2019 года мы начали с новым набором задач, поэтому уже можно обнародовать решение старых задачек за 2018 год, а также первую половину 2019. Тем более, многие из них были взяты из ранее опубликованных статей, а листовки с задачами содержали ссылку или QR-код с информацией о статье.
Если вы бывали на конференциях, где мы стояли со стендом, то наверняка видели или даже решали какие-то из наших задач. Это всегда фрагменты кода из реальных открытых проектов на языках программирования C, C++, C# или Java. В коде содержатся ошибки, которые мы предлагаем поискать посетителям. За решение (или просто обсуждение ошибки) мы выдаем призы — статусы на рабочий стол, брелоки и т.п.:
Хотите такие же? Приходите на наш стенд на следующих конференциях.
Кстати, в статьях "Время конференций! Подводим итоги 2018 года" и "Конференции. Промежуточные итоги по первому полугодию 2019" содержится описание нашей активности на конференциях в этом и прошлом году.
Итак, начнём игру «Найди ошибку в коде». Сначала рассмотрим более старые задачки за 2018 год, будем использовать группировку по языкам программирования.
2018
С++
Chromium bug
static const int kDaysInMonth[13] = {
0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};
bool ValidateDateTime(const DateTime& time) {
if (time.year < 1 || time.year > 9999 ||
time.month < 1 || time.month > 12 ||
time.day < 1 || time.day > 31 ||
time.hour < 0 || time.hour > 23 ||
time.minute < 0 || time.minute > 59 ||
time.second < 0 || time.second > 59) {
return false;
}
if (time.month == 2 && IsLeapYear(time.year)) {
return time.month <= kDaysInMonth[time.month] + 1;
} else {
return time.month <= kDaysInMonth[time.month];
}
}
Ответ
Пожалуй, самая «долгоиграющая» задачка из нашего набора. Эту ошибку в проекте Chromium мы предлагали найти посетителям нашего стенда в течение всего 2018 года. Также она была показана в ходе нескольких докладов.
В теле последнего блока If-else содержатся опечатки в возвращаемом значении. Вместо time.day программист дважды по ошибке указал time.month. Это привело к тому, что всегда будет возвращено значение true. Ошибка подробно разобрана в статье "31 февраля". Отличный пример ошибки, которую непросто обнаружить на code review. Также это хорошая иллюстрация использования технологии анализа потока данных.
if (time.month == 2 && IsLeapYear(time.year)) {
return time.month <= kDaysInMonth[time.month] + 1; // <= day
} else {
return time.month <= kDaysInMonth[time.month]; // <= day
}
В теле последнего блока If-else содержатся опечатки в возвращаемом значении. Вместо time.day программист дважды по ошибке указал time.month. Это привело к тому, что всегда будет возвращено значение true. Ошибка подробно разобрана в статье "31 февраля". Отличный пример ошибки, которую непросто обнаружить на code review. Также это хорошая иллюстрация использования технологии анализа потока данных.
Unreal Engine bug
bool VertInfluencedByActiveBone(
FParticleEmitterInstance* Owner,
USkeletalMeshComponent* InSkelMeshComponent,
int32 InVertexIndex,
int32* OutBoneIndex = NULL);
void UParticleModuleLocationSkelVertSurface::Spawn(....)
{
....
int32 BoneIndex1, BoneIndex2, BoneIndex3;
BoneIndex1 = BoneIndex2 = BoneIndex3 = INDEX_NONE;
if(!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[1], &BoneIndex2) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[2]) &BoneIndex3)
{
....
}
Ответ
Первым делом следует обратить внимание на то, что последний аргумент функции VertInfluencedByActiveBone() имеет значение по умолчанию и может быть не указан. Теперь взгляните на блок if. Упрощенно его можно переписать так:
Теперь видно, что допущена ошибка. Из-за опечатки третий вызов функции VertInfluencedByActiveBone() производится с тремя аргументами вместо четырех, а к результату этого вызова применяется оператор & (побитовое И, слева будет результат функции VertInfluencedByActiveBone() типа bool, справа – целочисленная переменная BoneIndex3). Код при этом компилируется. Исправленный вариант кода (добавлена запятая, закрывающая скобка перемещена в другое место):
Ошибка, которая первоначально была описана в статье "Долгожданная проверка Unreal Engine 4". В статье ошибка озаглавлена как «Самая красивая из найденных ошибок». Я согласен с данным утверждением.
if (!foo(....) && !foo(....) && !foo(....) & arg)
Теперь видно, что допущена ошибка. Из-за опечатки третий вызов функции VertInfluencedByActiveBone() производится с тремя аргументами вместо четырех, а к результату этого вызова применяется оператор & (побитовое И, слева будет результат функции VertInfluencedByActiveBone() типа bool, справа – целочисленная переменная BoneIndex3). Код при этом компилируется. Исправленный вариант кода (добавлена запятая, закрывающая скобка перемещена в другое место):
if(!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[1], &BoneIndex2) &&
!VertInfluencedByActiveBone(
Owner, SourceComponent, VertIndex[2], &BoneIndex3))
Ошибка, которая первоначально была описана в статье "Долгожданная проверка Unreal Engine 4". В статье ошибка озаглавлена как «Самая красивая из найденных ошибок». Я согласен с данным утверждением.
Android bugs
void TagMonitor::parseTagsToMonitor(String8 tagNames) {
std::lock_guard<std::mutex> lock(mMonitorMutex);
// Expand shorthands
if (ssize_t idx = tagNames.find("3a") != -1) {
ssize_t end = tagNames.find(",", idx);
char* start = tagNames.lockBuffer(tagNames.size());
start[idx] = '\0';
....
}
....
}
Ответ
В условии блока if перепутан приоритет операций. Код работает не так, как задумал программист:
Переменная idx будет получать значения 0 или 1, а выполнение условия будет зависеть от этого значения, что является ошибкой. Исправленный вариант кода:
Ошибка из статьи "Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален".
if (ssize_t idx = (tagNames.find("3a") != -1))
Переменная idx будет получать значения 0 или 1, а выполнение условия будет зависеть от этого значения, что является ошибкой. Исправленный вариант кода:
ssize_t idx = tagNames.find("3a");
if (idx != -1)
Ошибка из статьи "Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален".
И еще одна задачка на поиск нетривиальной ошибки в Android:
typedef int32_t GGLfixed;
GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
{
if ((d>>24) && ((d>>24)+1)) {
n >>= 8;
d >>= 8;
}
return gglMulx(n, gglRecip(d));
}
Ответ
Проблема в выражении (d >> 24) + 1.
Программист хотел проверить, что 8 старших бит переменной d содержат единицы, но при этом не все биты сразу. Другими словами, программист хотел проверить, что в старшем байте находится любое значение, отличное от 0x00 и 0xFF. Сначала он проверил, что старшие биты ненулевые, написав выражение (d>>24). Далее он сдвигает старшие восемь бит в младший байт. При этом он рассчитывает, что самый старший знаковый бит дублируется во всех остальных битах. То есть если переменная d равна 0b11111111'00000000'00000000'00000000, то после сдвига получится значение 0b11111111'11111111'11111111'11111111. Прибавив 1 к значению 0xFFFFFFFF типа int, программист планирует получить 0 (-1+1=0). Таким образом, выражением ((d>>24)+1) он проверяет, что не все старшие восемь бит равны 1.
Однако при сдвиге старший знаковый бит вовсе не обязательно «размазывается». Стандарт говорит: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined».
Таким образом, это пример поведения, зависящего от реализации (implementation-defined behavior). Как будет работать этот код — зависит от архитектуры микропроцессора и реализации компилятора. После сдвига в старших битах вполне могут оказаться нули, и тогда результат выражения ((d>>24)+1) всегда будет отличен от 0, то есть будет всегда истинным значением.
Действительно, непростая задачка. Эта ошибка, как и предыдущая, была описана в статье "Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален".
Программист хотел проверить, что 8 старших бит переменной d содержат единицы, но при этом не все биты сразу. Другими словами, программист хотел проверить, что в старшем байте находится любое значение, отличное от 0x00 и 0xFF. Сначала он проверил, что старшие биты ненулевые, написав выражение (d>>24). Далее он сдвигает старшие восемь бит в младший байт. При этом он рассчитывает, что самый старший знаковый бит дублируется во всех остальных битах. То есть если переменная d равна 0b11111111'00000000'00000000'00000000, то после сдвига получится значение 0b11111111'11111111'11111111'11111111. Прибавив 1 к значению 0xFFFFFFFF типа int, программист планирует получить 0 (-1+1=0). Таким образом, выражением ((d>>24)+1) он проверяет, что не все старшие восемь бит равны 1.
Однако при сдвиге старший знаковый бит вовсе не обязательно «размазывается». Стандарт говорит: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined».
Таким образом, это пример поведения, зависящего от реализации (implementation-defined behavior). Как будет работать этот код — зависит от архитектуры микропроцессора и реализации компилятора. После сдвига в старших битах вполне могут оказаться нули, и тогда результат выражения ((d>>24)+1) всегда будет отличен от 0, то есть будет всегда истинным значением.
Действительно, непростая задачка. Эта ошибка, как и предыдущая, была описана в статье "Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален".
2019
С++
«Во всём виноват GCC»
int foo(const unsigned char *s)
{
int r = 0;
while(*s) {
r += ((r * 20891 + *s *200) | *s ^ 4 | *s ^ 3) ^ (r >> 1);
s++;
}
return r & 0x7fffffff;
}
Программист утверждает, что данный код работает с ошибкой по вине компилятора GCC 8. Так ли это?
Ответ
Функция возвращает отрицательные значения. Причина в том, что компилятор не генерирует код для оператора побитового И (&). Ошибка связана с неопределённым поведением. Компилятор видит, что в переменной r считается некоторая сумма. При этом прибавляются только положительные числа. Переполнения переменной r произойти не должно, иначе это неопределённое поведение, которое компилятор никак не должен рассматривать и учитывать. Итак, компилятор считает, что раз значение в переменной r после окончания цикла не может быть отрицательным, то операция r & 0x7fffffff для сброса знакового бита является лишней и компилятор просто возвращает из функции значение переменной r.
Ошибка из статьи "Релиз PVS-Studio 6.26".
Ошибка из статьи "Релиз PVS-Studio 6.26".
QT bug
static inline const QMetaObjectPrivate *priv(const uint* data)
{ return reinterpret_cast<const QMetaObjectPrivate*>(data); }
bool QMetaEnum::isFlag() const
{
const int offset = priv(mobj->d.data)->revision >= 8 ? 2 : 1;
return mobj && mobj->d.data[handle + offset] & EnumIsFlag;
}
Ответ
С указателем mobj работают небезопасно. Сначала он разыменовывается, а далее проверяется. Классика.
Ошибка была описана в статье "Третья проверка Qt 5 с помощью PVS-Studio".
Ошибка была описана в статье "Третья проверка Qt 5 с помощью PVS-Studio".
C#
Infer.NET bug
public static void
WriteAttribute(TextWriter writer,
string name,
object defaultValue,
object value,
Func<object, string> converter = null)
{
if ( defaultValue == null && value == null
|| value.Equals(defaultValue))
{
return;
}
string stringValue = converter == null ? value.ToString() :
converter(value);
writer.Write($"{name}=\"{stringValue}\" ");
}
Ответ
В выражении value.Equals(defaultValue) возможен доступ по нулевой ссылке value. Это произойдет при таких значениях переменных, когда defaultValue != null, а value == null.
Ошибка из статьи "Какие ошибки прячутся в коде Infer.NET?"
Ошибка из статьи "Какие ошибки прячутся в коде Infer.NET?"
FastReport bug
public class FastString
{
private const int initCapacity = 32;
private void Init(int iniCapacity)
{ sb = new StringBuilder(iniCapacity); .... }
public FastString() { Init(initCapacity); }
public FastString(int iniCapacity) { Init(initCapacity); }
public StringBuilder StringBuilder => sb;
}
....
Console.WriteLine(new FastString(256).StringBuilder.Capacity);
Что будет выведено на консоль? Что не так с классом FastString?
Ответ
На консоль будет выведено 32. Причина — опечатка в имени переменной, передаваемой в метод Init в конструкторе:
Параметр конструктора iniCapacity не будет использован. Вместо этого в метод Init передают константу initCapacity.
Ошибка была описана в статье "Самые быстрые отчёты на диком западе. И горстка багов в придачу..."
public FastString(int iniCapacity){ Init(initCapacity); }
Параметр конструктора iniCapacity не будет использован. Вместо этого в метод Init передают константу initCapacity.
Ошибка была описана в статье "Самые быстрые отчёты на диком западе. И горстка багов в придачу..."
Roslyn bug
private SyntaxNode GetNode(SyntaxNode root)
{
var current = root;
....
while (current.FullSpan.Contains(....))
{
....
var nodeOrToken = current.ChildThatContainsPosition(....);
....
current = nodeOrToken.AsNode();
}
....
}
public SyntaxNode AsNode()
{
if (_token != null)
{
return null;
}
return _nodeOrParent;
}
Ответ
Возможен доступ по нулевой ссылке current в выражении current.FullSpan.Contains(....). Переменная current может получить нулевое значение как результат выполнения метода nodeOrToken.AsNode().
Ошибка из статьи "Проверяем исходный код Roslyn".
Ошибка из статьи "Проверяем исходный код Roslyn".
Unity bug
....
staticFields = packedSnapshot.typeDescriptions
.Where(t =>
t.staticFieldBytes != null &
t.staticFieldBytes.Length > 0)
.Select(t => UnpackStaticFields(t))
.ToArray()
....
Ответ
Допущена опечатка: вместо оператора && использован оператор &. Это приводит к тому, что проверка t.staticFieldBytes.Length > 0 выполняется всегда, даже в случае равенства null переменной t.staticFieldBytes, что, в свою очередь, приведет к доступу по нулевой ссылке.
Впервые эта ошибка была показана в статье "Анализируем ошибки в открытых компонентах Unity3D".
Впервые эта ошибка была показана в статье "Анализируем ошибки в открытых компонентах Unity3D".
Java
IntelliJ IDEA bug
private static boolean checkSentenceCapitalization(@NotNull String value) {
List<String> words = StringUtil.split(value, " ");
....
int capitalized = 1;
....
return capitalized / words.size() < 0.2; // allow reasonable amount of
// capitalized words
}
Предлагается определить, почему будет неправильно подсчитано число слов с заглавными буквами.
Ответ
Функция должна возвращать истину, если менее 20% слов начинаются с заглавной буквы. Но проверка не работает, так как происходит целочисленное деление, результатом которого будут только значения 0 или 1. Функция вернёт ложное значение, только если все слова будут начинаться с заглавной буквы. В остальных случаях при делении будет получаться 0, а функция будет возвращать истину.
Ошибка из статьи "PVS-Studio для Java".
Ошибка из статьи "PVS-Studio для Java".
SpotBugs bug
public static String getXMLType(@WillNotClose InputStream in) throws IOException
{
....
String s;
int count = 0;
while (count < 4) {
s = r.readLine();
if (s == null) {
break;
}
Matcher m = tag.matcher(s);
if (m.find()) {
return m.group(1);
}
}
throw new IOException("Didn't find xml tag");
....
}
Предлагается определить, в чём заключается ошибка поиска xml-тега.
Ответ
Условие count < 4 будет выполнено всегда, так как переменную count не инкрементируют внутри цикла. Предполагалось, что поиск xml-тега должен осуществляться только в первых четырёх строках файла, но из-за ошибки будет прочитан весь файл.
Эта ошибка, как и предыдущая, была описана в статье "PVS-Studio для Java".
Эта ошибка, как и предыдущая, была описана в статье "PVS-Studio для Java".
На этом – всё. Ждём вас на следующих конференциях. Ищите стенд с единорогом. Выдадим новые интересные задачки и, конечно же, задарим призы. До встречи!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Solutions to Bug-Finding Challenges Offered by the PVS-Studio Team at Conferences in 2018-2019.