Проверяем YTsaurus. Доступность, надёжность, open source
В этой статье мы расскажем вам о результатах проверки кода проекта YTsaurus статическим анализатором PVS-Studio и разберём найденные ошибки. Уже больше полугода назад YTsaurus — мощная BigData-система — стала Open Source. Её разработка и использование направлены на расширение инфраструктуры и развитие бизнеса в сфере IT и коммуникаций. Последнее время YTsaurus — частая тема для обсуждения. А судя по статистике на GitHub, проект продолжает набирать популярность. Всё это делает его интересным объектом для исследования.
Знакомство с YTsaurus
Вообще о проекте часто и много говорят на различных конференциях, которые посещает наша команда. Как раз недавно наши ребята принимали участие в конференциях HighLoad++ и TeamLead Conf, и там темы, связанные с YTsaurus, были на слуху.
А если вам интересно, как такие мероприятия проходят с точки зрения тех, кто выступает и/или организует стенды — добро пожаловать! Последнее участие в конференции дополнительно выросло в интересную и эмоциональную статью.
Так как проект на слуху, а мне интересны подобные направления, дело дошло до довольно информативной статьи Максима Бабенко «YTsaurus: основная система для хранения и обработки данных Яндекса теперь open source». В ней подробно рассказывают историю создания YTsaurus, разработанной в Яндексе и ставшей доступной для обычного пользователя как Open Source решение. YTsaurus — результат почти десятилетнего труда, которым разработчики захотели поделится с миром! В статье вам расскажут историю возникновения, ответят на вопрос «зачем нужен YTsaurus», опишут ключевые возможности и обозначат область её применения.
Почти весь прочтённый вами выше текст я скопировал (и чуть изменил) из вступительной части статьи Максима. Так что, если стало интересно, прочтите статью полностью — дальше будет ещё интереснее. Ну, а сам я ничего нового сказать о проекте не смогу, кроме результатов проверки кода, конечно. Сам проект, кстати, доступен для всех желающих здесь.
Не успел я проанализировать весь проект, как на GitHub уже добавилось множество коммитов с различными правками. Это говорит о том, что проект активно разрабатывается.
Коммит, на котором я клонировал проект: 55cd85f.
Написание и публикация данной статьи не имеют цели обесценить труд программистов, занимающихся разработкой данного продукта. Цель — популяризация статических анализаторов кода, которые полезны даже для качественных и состоявшихся проектов.
Трудности, с которыми пришлось столкнуться
Рад всем сообщить, что трудностей не было. «Зачем же такой громкий заголовок?», — спросите вы.
Я бы хотел выразить отдельную благодарность разработчикам проекта! Насколько же подробная у проекта инструкция по сборке. И насколько же всё просто собирается. Думаю, если бы YTsaurus пытались собрать дети, то и у них без труда бы всё получилось.
Ну, а теперь, собственно, то, ради чего все затевалось.
В начале была тьма…
… всяких разных срабатываний и предупреждений.
Если проект объёмный, анализатор может выявить великое множество подозрительных участков кода. Это может испортить первое знакомство пользователя с ним: пользователь поседеет и удалит анализатор из системы.
Как же избежать подобной ситуации?
Следует сделать небольшую предварительную настройку. Например, включить наиболее интересующие вас группы диагностических правил и отключить лишние. Нет нужды включать группы диагностик MISRA/AUTOSAR, если вы не пишете проекты согласно этим стандартам.
После первого запуска анализатора можно отобразить наиболее интересные предупреждения. Это позволит ускорить и упростить ознакомление с отчетом.
Чтобы начать ощущать пользу от регулярного анализа на свежем коде, нужно что-то сделать со всеми этими срабатываниями. Решений напрашивается несколько, мы же для этой цели предоставляем механизм подавления предупреждений с помощью suppress-файлов. После подавления всех предупреждений при следующем запуске анализа в отчёте будет 0 предупреждений.
Разработчикам на своих машинах нет смысла выполнять полный анализ проекта, ведь обычно правка делается в сотнях файлов. Для этого существует инкрементальный режим — на анализ будут поставлены лишь те файлы, которые вы изменили. Анализатор будет выдавать предупреждения только для свежего кода, и их число будет невелико (единицы-десятки). Если предупреждение ложное, его можно точечно подавить.
Полный анализ проекта можно интегрировать в CI во время ночных сборок. Если вы применяете модель Pull / Merge Requests при работе с проектом, то можно интегрировать анализ и туда.
Периодически стоит возвращаться к старым предупреждениям, доставая их из suppress-файлов и исправляя.
При разовых проверках проектов мы, конечно, не проделываем все эти шаги. Но некоторую предварительную настройку всё же делаем — исключаем third-party библиотеки, включаем только группы диагностических правил общего назначения и микрооптимизаций. Анализатор порой и при таком раскладе генерирует много предупреждений, но на поиск и разбор ошибок в YTsaurus ушло не так много времени.
В процессе просмотра и разбора анализа нашлось много интересных и не очень моментов, которыми делюсь с вами.
Результаты проверки
Фрагмент N1
TAsyncSignalsHandler *SIGNALS_HANDLER = nullptr;
void SetAsyncSignalHandler(int signum, TAutoPtr handler)
{
static TAdaptiveLock lock;
if (Y_UNLIKELY(SIGNALS_HANDLER == nullptr)) // N1
{
TGuard dnd(lock);
if (SIGNALS_HANDLER == nullptr) // N2
{
// NEVERS GETS DESTROYED
SIGNALS_HANDLER = new TAsyncSignalsHandler(); // N3
}
}
SIGNALS_HANDLER->Install(signum, handler); // N4
}
Предупреждение анализатора: V547 Expression 'SIGNALS_HANDLER == nullptr' is always true. async_signals_handler.cpp:200
Несмотря на то, что здесь сработало диагностическое правило V547, был найден интересный экземпляр паттерна Double-checked locking.
Что же может пойти не так? Для начала имеем небольшую вероятность того, что компилятор может закэшировать значение переменной SIGNALS_HANDLER в регистре и не будет заново читать из неё новое значение. По крайней мере, если будет использоваться нетипичный способ блокировки. Лучшим решением будет объявить переменную как volatile или использовать std: atomic.
Дальше больше. Давайте попробуем разобраться, что здесь не очень хорошо. Будем считать, что с этим кодом оперируют поток A и поток B.
Поток A первым добирается до точки N1. Указатель нулевой, так что поток управления заходит в первый if и захватывает объект блокировки.
Поток A доходит до точки N2. Указатель также нулевой, поэтому поток управления заходит во второй if.
Дальше в точке N3 может произойти небольшая магия. Инициализацию указателя можно представить примерно в следующем виде:
auto tmp = (TAsyncSignalsHandler *) malloc(sizeof(TAsyncSignalsHandler));
new (tmp) TAsyncSignalsHandler();
SIGNALS_HANDLER = tmp;
Хитрый процессор может переупорядочить инструкции, и в итоге SIGNALS_HANDLER будет проинициализирован до того, как будет вызван placement new.
И тут в бой вступает поток B — как раз в тот момент, когда поток A еще не успел вызвать placement new. Поток управления приходит в точку N1, видит инициализированный указатель и перемещается в точку N4.
Дальше в потоке B происходит вызов функции-члена Install на объекте, у которого еще не стартовало время жизни. Неопределенное поведение…
Как поправить этот паттерн? Надо сделать так, чтобы чтение и запись в SIGNALS_HANDLER происходили атомарно:
std::atomic SIGNALS_HANDLER { nullptr };
void SetAsyncSignalHandler(int signum, TAutoPtr handler)
{
static TAdaptiveLock lock;
auto tmp = SIGNALS_HANDLER.load(std::memory_order_acquire); // <=
if (Y_UNLIKELY(tmp == nullptr))
{
TGuard dnd(lock);
tmp = SIGNALS_HANDLER.load(std::memory_order_relaxed); // <=
if (tmp == nullptr)
{
// NEVERS GETS DESTROYED
tmp = new TAsyncSignalsHandler();
SIGNALS_HANDLER.store(tmp, std::memory_order_release); // <=
}
}
tmp->Install(signum, handler);
}
Фрагмент N2
TSimpleTM& TSimpleTM::Add(EField f, i32 amount)
{
....
case F_YEAR:
{
i32 y = amount + (i32)Year;
y = ::Min(Max(y, 0), 255 /*max year*/);
// YDay may correspond to different MDay if
// it's March or greater and the years have different leap status
if (Mon > 1)
{
YDay += (i32)LeapYearAD(RealYear()) - (i32)LeapYearAD(RealYear()); // <=
}
Year = y;
IsLeap = LeapYearAD(RealYear());
return RegenerateFields();
}
....
}
Предупреждение анализатора: V501 There are identical sub-expressions '(i32) LeapYearAD (RealYear ())' to the left and to the right of the '-' operator. datetime.cpp:154:1
Очень похоже, что выражения слева и справа должны быть разными. Однако это не так, из-за чего всегда происходит YDay += 0.
Трудно сказать наверняка, не зная всего контекста, что должно происходить в этой строке и какой там код должен быть. Анализатор в этом случае отработал правильно.
У меня же появилось стойкое чувство, как будто разработчика чем-то резко отвлекли в момент написания кода.
Фрагмент N3
bool operator == (const TCompositePendingJobCount& lhs,
const TCompositePendingJobCount& rhs)
{
if (lhs.DefaultCount != rhs.DefaultCount)
{
return false;
}
if (lhs.CountByPoolTree.size() != lhs.CountByPoolTree.size()) // <=
{
return false;
}
for (const auto& [tree, lhsCount] : lhs.CountByPoolTree)
{
....
}
return true;
}
Предупреждение анализатора: V501 There are identical sub-expressions 'lhs.CountByPoolTree.size ()' to the left and to the right of the '!=' operator. structs.cpp:191
Ещё один пример ошибки, когда вас отвлекают от написания кода в самый неподходящий момент. В условии сравнивается размер одного и того же контейнера, и выражение всегда будет вычисляться как false.
Правильный код должен выглядеть следующим образом:
if (lhs.CountByPoolTree.size() != rhs.CountByPoolTree.size()){....}
Кстати, ошибки в сравнениях допускаются очень часто. В статье представлены далеко не все ошибки в сравнениях, найденные нашим анализатором. На эту тему, кстати говоря, есть интересная статья моего коллеги — «Зло живет в функциях сравнения», которую я рекомендую вам к прочтению.
Фрагмент N4
template
class TSimpleOperationCommandBase
: public virtual TTypedCommandBase
{
....
public:
TSimpleOperationCommandBase()
{
....
if (!OperationId.IsEmpty() && OperationAlias.operator bool() ||
OperationId.IsEmpty() && !OperationAlias.operator bool() )
{
....
}
....
}
};
Предупреждение анализатора: V728 — Message: An excessive check can be simplified. The '(A && ! B) || (! A && B)' expression is equivalent to the 'bool (A) != bool (B)' expression. scheduler_commands.h:36:1
Возможно, программисту, писавшему код, визуально проще воспринимать запись выше. По сути, это выражение означает операцию взаимоисключающего «ИЛИ» для двух булевых операндов. В языках C и C++ помимо побитового взаимоисключающего «ИЛИ» существует также и неявное логическое. Оно реализуется через оператор »!=», и при помощи него можно переписать выражение вот так:
if (OperationId.IsEmpty() != OperationAlias.operator bool())
Взаимоисключающее «ИЛИ» возвращает истину только когда операнды не равны. И оператор »!=» как раз отражает это. Согласитесь, такой код выглядит лаконичнее, чем исходный.
И… ещё пример.
Фрагмент N5
int TComparator::CompareKeyBounds(const TKeyBound& lhs,
const TKeyBound& rhs,
int lowerVsUpper) const
{
....
// Prefixes coincide. Check if key bounds are indeed at the same point.
{
auto lhsInclusivenessAsUpper = (lhs.IsUpper && lhs.IsInclusive) ||
(!lhs.IsUpper && !lhs.IsInclusive);
auto rhsInclusivenessAsUpper = (rhs.IsUpper && rhs.IsInclusive) ||
(!rhs.IsUpper && !rhs.IsInclusive);
if (lhsInclusivenessAsUpper != rhsInclusivenessAsUpper)
{
return lhsInclusivenessAsUpper - rhsInclusivenessAsUpper;
}
}
....
}
Предупреждение анализатора: V728 — Message: An excessive check can be simplified. The '(A && B) || (! A && ! B)' expression is equivalent to the 'bool (A) == bool (B)' expression. comparator.cpp:194:1
Как вариант можно сократить запись до:
auto lhsInclusivenessAsUpper = lhs.IsUpper == lhs.IsInclusive;
auto rhsInclusivenessAsUpper = rhs.IsUpper == rhs.IsInclusive;
Таких предупреждений может быть много в любом проекте (в данном нашлось 8), и только вы решаете, как поступить. Кому-то более читабельным покажется вариант «до», а кому-то нравится вариант после сокращения.
Вкусовщина, но наш анализатор умеет находить подобный код, что является довольно приятной опцией. Если же такого рода предупреждения вам не нужно выводить или анализировать, их можно отключить в любой момент.
Фрагмент N6, N7
void Reconfigure(TDistributedThrottlerConfigPtr config) override
{
MemberClient_->Reconfigure(config->MemberClient);
DiscoveryClient_->Reconfigure(config->DiscoveryClient);
auto oldConfig = Config_.Acquire();
if (oldConfig->LimitUpdatePeriod != config->LimitUpdatePeriod)
{
UpdateLimitsExecutor_->SetPeriod(config->LimitUpdatePeriod); // <=
}
if (oldConfig->LeaderUpdatePeriod != config->LeaderUpdatePeriod)
{
UpdateLeaderExecutor_->SetPeriod(config->LimitUpdatePeriod); // <=
}
....
Config_.Store(std::move(config));
}
Предупреждение анализатора: V778 Two similar code fragments were found. Perhaps, this is a typo and 'LeaderUpdatePeriod' variable should be used instead of 'LimitUpdatePeriod'. distributed_throttler.cpp:942, distributed_throttler.cpp:939
Очень напоминает copy-paste проблему:
Программист 1: «Можно использовать твое условие, чтобы написать свое?»
Программист 2: «Да, только не списывай точь-в-точь, чтобы не спалили.»
Программист 1: «ОК!»
В данном случае не совсем понятно, является ли использование поля LimitUpdatePeriod во втором случае корректным, или же там должно использоваться поле LeaderUpdatePeriod. Кто знает… :-/
Подобные проблемы встречаются в любом проекте, ещё один такой пример ниже:
static void xmlParseCommentComplex(xmlParserCtxtPtr ctxt, xmlChar *buf,
size_t len, size_t size)
{
....
q = CUR_CHAR(ql);
....
if (!IS_CHAR(q))
{
xmlFatalErrMsgInt(ctxt, XML_ERR_INVALID_CHAR,
"xmlParseComment: invalid xmlChar value %d\n",
q);
....
}
....
r = CUR_CHAR(rl);
....
if (!IS_CHAR(r))
{
xmlFatalErrMsgInt(ctxt, XML_ERR_INVALID_CHAR,
"xmlParseComment: invalid xmlChar value %d\n",
q); // <=
xmlFree (buf);
return;
}
....
}
Предупреждение анализатора: V778 Two similar code fragments were found. Perhaps, this is a typo and 'r' variable should be used instead of 'q'. parser.c:4771:1
Скорее всего, переменная q в теле условия if (! IS_CHAR (r)) написана ошибочно, и там должна стоять переменная r. Возможно, но это не точно. Проблему «copy-paste» среди программистов можно обсуждать часто и долго, однако невнимательность из-за усталости или отсутствия какого- либо опыта может проявляться в любой сфере деятельности и в работе с любым инструментом. Вот, например, встреча молотка с пальцем. Забиваешь ты гвоздь, и тут бах! Трехэтажный слой мата бережно усваивается всеми присутствующими рядом. Вам обидно, больно и стыдно. Классика.
Будьте внимательны и чаще отдыхайте!
Фрагмент N8, N9
....
std::vector> BlockWriters_;
....
class TPartitionMultiChunkWriter
: public TSchemalessMultiChunkWriterBase
{
....
TPartitionMultiChunkWriter(....) : ....
{
....
for (int partitionIndex = 0;
partitionIndex < partitionCount;
++partitionIndex)
{
BlockWriters_.emplace_back(
new THorizontalBlockWriter(Schema_, BlockReserveSize_)
);
....
}
....
}
}
Предупреждение анализатора: V1023 A pointer without owner is added to the 'BlockWriters_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. schemaless_chunk_writer.cpp:1374
Интересный вариант возможной потенциальной утечки памяти, и при этом она незаметна глазу. Давайте разбираться, когда она может произойти:
Всё начинается с создания объекта на куче при помощи оператора new, и затем указатель на этот объект идеально передается в функцию emplace_back.
Вектор проверяет, может ли он вставить объект без необходимости реаллокации. Допустим, что сделать это невозможно (т.е. capacity () == size ()). Тогда вектор просит аллокатор выдать ему кусок памяти большего размера.
Представим, что аллокатор по какой-либо причине не может удовлетворить такой запрос. Тогда вектор выкинет исключение, при этом его состояние останется таким же, как и до вызова emplace_back.
И здесь сюжетный твист. Как только наш объект покинет emplace_back, указатель на объект будет утерян. В итоге имеем утечку памяти.
Исправить проблему можно несколькими способами.
Способ N1. Если у объекта зовутся только публичные конструкторы, то начиная с C++14 можно воспользоваться функцией std: make_unique:
BlockWriters_.emplace_back(
std::make_unique(Schema_, BlockReserveSize_)
);
Способ N2. Если функция std: make_unique недоступна (версия стандарта, доступ к приватным конструкторам), то надо обернуть сырой указатель в умный перед передачей в вектор:
std::unique_ptr ptr {
new THorizontalBlockWriter(Schema_, BlockReserveSize_)
};
BlockWriters_.emplace_back(std::move(ptr));
Также советую заменить вызов emplace_back на push_back. Из-за того, что теперь в вектор происходит передача умного указателя, в любом случае будет происходить вызов конструктора перемещения у объекта внутри вектора. В случае с emplace_back компилятор будет проводить дополнительную работу по инстанцированию вариативного шаблона функции. С push_back компилятор будет делать меньше работы.
Вот ещё один пример такого кода, аналогичная проблема:
TBatch* GetBatch()
{
if (IsNewBatchNeeded())
{
Batches_.emplace_back(new TBatch()); // <=
}
return Batches_.back().get();
}
Предупреждение анализатора: V1023 A pointer without owner is added to the 'Batches_' container by the 'emplace_back' method. A memory leak will occur in case of an exception. tablet_request_batcher.cpp:244:1
Фрагмент N10, N11
std::vector BuildThreadSubqueries(....)
{
....
std::vector subqueries;
....
for (auto& stripe : subquery.StripeList->Stripes)
{
size_t operandIndex = stripe->GetInputStreamIndex();
YT_VERIFY(operandIndex >= 0);
....
}
....
}
Предупреждение анализатора: V547 Expression 'operandIndex >= 0' is always true. Unsigned type value is always >= 0. subquery.cpp:1061
Прежде чем продолжить, я предлагаю читателю ознакомиться с макросом YT_VERIFY:
#define YT_VERIFY(expr) \
do { \
if (Y_UNLIKELY(!(expr))) { \
YT_ASSERT_TRAP("YT_VERIFY", #expr); \
} \
} while (false)
Также для понимания добавляю макрос YT_ASSERT_TRAP:
#define YT_ASSERT_TRAP(trapType, expr) \
::NYT::NDetail::AssertTrapImpl(TStringBuf(trapType), TStringBuf(expr),
__SOURCE_FILE_IMPL__.As(),
__LINE__, TStringBuf(__FUNCTION__)); \
Y_UNREACHABLE()
Макрос YT_VERIFY — это такой assert на стероидах, который не исчезает даже под релизом.
Анализатор сообщает о том, что первая проверка YT_VERIFY (operandIndex >= 0) всегда истинна. Как видно строкой выше, переменная operandIndex имеет тип size_t, т.е. беззнаковый. А как известно, беззнаковые типы не содержат отрицательный диапазон значений. Могу предположить, что когда-то функция GetInputStreamIndex возвращала значение знакового типа, и такая проверка имела место быть.
И ещё один такой пример, но уже проверка на больше или равно нулю переменной типа unsigned long:
TChunkIndexBlock BuildChunkIndexBlock( .... )
{
....
for(int sectorIndex = 0;
sectorIndex < bestFormatDetail->GetSectorCount();
++sectorIndex)
{
....
auto paddingSize = THashTableChunkIndexFormatDetail::SectorSize –
(buffer - sectorStart) - sizeof(TChecksum);
YT_VERIFY(paddingSize >= 0);
WriteZeroes(buffer, paddingSize);
....
}
....
}
Предупреждение анализатора: V547 Expression 'paddingSize >= 0' is always true. Unsigned type value is always >= 0. chunk_index_builder.cpp:278:1
Рассмотрим декларацию paddingSize поближе:
auto paddingSize = THashTableChunkIndexFormatDetail::SectorSize –
(buffer - sectorStart) - sizeof(TChecksum);
Возможно, разработчики предполагали, что приоритет будет у знаковых чисел, и в результате paddingSize будет знакового типа. Однако выражение sizeof (TChecksum) имеет тип size_t. Вообще я советую обратить внимание на данную строку.
Фрагмент N12
size_t TParallelFileReader::DoReadWithCallback(void* ptr,
size_t size,
DoReadCallback&& callback)
{
....
std::optional curBlob;
while (curBlob = ReadNextBatch())
{
....
}
if (....)
{
return curIdx;
}
else
{
size_t prevIdx = curIdx - curBlob->Size(); // <=
Y_VERIFY(!BatchTail_);
Y_VERIFY(curBlob.has_value()); // <=
BatchTail_ = curBlob->SubBlob(size - prevIdx, curBlob->Size());
return size;
}
}
Предупреждение анализатора: V1007 The value from the potentially uninitialized optional 'curBlob' is used. Probably it is a mistake. parallel_file_reader.cpp:241
Чтобы лучше понимать этот фрагмент кода, раскрываем макрос Y_VERIFY:
#define Y_VERIFY(expr, ...) \
do{ \
if(Y_UNLIKELY(!(expr)))
{ \
::NPrivate::Panic(__SOURCE_FILE_IMPL__, __LINE__, __FUNCTION__,
#expr, " " __VA_ARGS__); \
} \
} while (false)
Давайте разбираться. В коде присутствует переменная curBlob опционального типа. Далее в цикле вызывается функция ReadNextBatch, которая инициализирует curBlob. Выход из цикла возможен в двух ситуациях:
Допустим, выход произошёл из-за второй ситуации, и поток управления попадает в else-ветку. Тогда при вычислении инициализатора переменной prevIdx произойдёт доступ к нулевому объекту, что ведёт к неопределённому поведению.
Обратите внимание на код двумя строками ниже. Можно сделать вывод, что разработчик подразумевал, что такая ситуация возможна, поэтому добавил проверку. Но добавил её слишком поздно, и почки уже отвалились.
Я бы предложил исправить код следующим образом, просто переместив проверку выше разыменования:
Y_VERIFY(!BatchTail_);
Y_VERIFY(curBlob.has_value());
size_t prevIdx = curIdx - curBlob->Size();
Фрагмент N13
template
class TInteger128 { .... };
using ui128 = TInteger128;
using i128 = TInteger128;
// specialize std templates
namespace std
{
....
constexpr bool signbit(const ui128 arg) noexcept
{
Y_UNUSED(arg);
return false;
}
constexpr bool signbit(const i128 arg) noexcept
{
return GetHigh(arg) & 0x8000000000000000;
}
constexpr ui128 abs(const ui128 arg) noexcept
{
return arg;
}
constexpr i128 abs(const i128 arg) noexcept
{
return signbit(arg) ? (-arg) : arg;
}
}
Предупреждение анализатора: V1061 Extending the 'std' namespace may result in undefined behavior. int128.h:378:1
Мы любим С++ и любим стандарты! Стандарты добавляют понимания тому, как должен работать тот или иной инструмент. В данном случае комитетом по стандартам определены правила расширения пространства имен std. В этих правилах четко говорится, что оно не может расширяться за счёт объявления там обычных функций, переменных и классов, т.к. это может привести к неопределенному поведению.
В приведённом примере видно, что разработчик хотел добавить перегрузки стандартных функций для 128-битного целого числа (TInteger128). Однако такое расширение ведёт к неопределённому поведению. Правильное место добавления таких функций — то же пространство имён, где объявлен класс. Затем в каждом месте, где потребуется стандартная функция, следует воспользоваться ее неквалифицированным именем:
template
void foo(T &arg)
{
using std::abs;
arg = abs(arg);
....
}
Ошибочные случаи расширения пространства имён std встречаются настолько часто, что у нас появился рейтинг, и возглавляют его std: swap и std: hash. Кстати, о них.
Фрагмент N14
namespace std
{
/////////////////////////////////////////////////////////////////////
template
void swap(NYT::TCompactVector& lhs,
NYT::TCompactVector& rhs)
{
lhs.swap(rhs);
}
/////////////////////////////////////////////////////////////////////
template
struct hash>
{
size_t operator()(const NYT::TCompactVector& container) const
{
size_t result = 0;
for (const auto& element : container)
{
NYT::HashCombine(result, element);
}
return result;
}
};
/////////////////////////////////////////////////////////////////////
}
Предупреждение анализатора: V1061 Extending the 'std' namespace may result in undefined behavior. compact_vector-inl.h:999:1
В случае с кастомным шаблоном функции std: swap, согласно стандарту C++, запрещено добавлять новые шаблоны в пространство имён std. Следует переместить эту функцию в пространство имён NYT.
В случае со специализацией шаблона класса std: hash, здесь всё в порядке. Однако и её можно вынести за пределы пространства имен std. Как минимум ради того, чтобы не искушать других разработчиков продолжать традицию расширять std:
template
struct std::hash> { .... };
Фрагмент N15
IUnversionedRowBatchPtr
THorizontalSchemalessRangeChunkReader::Read(
const TRowBatchReadOptions& options)
{
TCurrentTraceContextGuard traceGuard(TraceContext_);
auto readGuard = AcquireReadGuard(); // <=
....
}
Предупреждение анализатора: V1002 The 'TTimerGuard' class, containing pointers, constructor and destructor, is copied by the automatically generated copy constructor. schemaless_chunk_reader.cpp:731
В данном случае анализатор обнаружил шаблон класса TTimerGuard, который может работать не совсем правильно. Этот шаблон класса выступает в качестве RAII обертки над таймерами. Давайте посмотрим на его реализацию:
template
class TTimerGuard
{
public:
explicit TTimerGuard(TTimer* timer);
~TTimerGuard();
private:
TTimer* const Timer_;
};
template
TTimerGuard::TTimerGuard(TTimer* timer)
: Timer_(timer)
{
Timer_->Start();
}
template
TTimerGuard::~TTimerGuard()
{
Timer_->Stop();
}
Как видим, конструктор предполагает захват таймера и его старт, а в деструкторе остановку. Однако у этого шаблона класса также будет сгенерирован неявный конструктор/оператор копирования. Это может привести к тому, что объект может быть скопирован. А затем приведет к многократному вызову функции остановки у одного и того же таймера. Это необязательно приведёт к ошибке, но дизайн такого шаблона класса не должен давать такой возможности.
В случае если у этого шаблона класса должна быть семантика, схожая с std: lock_guard, будет достаточно запретить конструктор/оператор копирования:
template
class TTimerGuard
{
public:
explicit TTimerGuard(TTimer* timer);
TTimerGuard(const TTimerGuard &) = delete;
TTimerGuard& operator=(const TTimerGuard &) = delete;
~TTimerGuard();
private:
TTimer* const Timer_;
};
Если требуется, чтобы объекты можно было перемещать (например, как std: unique_lock), то придется немного изменить реализацию:
template
class TTimerGuard
{
public:
TTimerGuard() = default;
explicit TTimerGuard(TTimer* timer) noexcept(noexcept(timer->Start()));
TTimerGuard(TTimerGuard &&other) noexcept;
TTimerGuard& operator=(TTimerGuard &&other) noexcept;
~TTimerGuard();
private:
TTimer* Timer_ = {};
};
template
TTimerGuard::TTimerGuard(TTimer *timer)
noexcept(noexcept(timer->Start()))
: Timer_ { timer }
{
if (Timer_)
Timer_->Start();
}
template
TTimerGuard::TTimerGuard(TTimerGuard &&other) noexcept
: Timer_ { std::exchange(other.Timer_, {}) }
{
}
template
TTimerGuard&
TTimerGuard::operator=(TTimerGuard &&other) noexcept
{
Timer_ = std::exchange(other.Timer_, {});
return *this;
}
template
TTimerGuard::~ TTimerGuard()
{
if (Timer_)
Timer_->Stop();
}
Фрагмент N16
TDivisors GetDivisors(const TSchemaColumns& columns,
int keyIndex,
TConstExpressionPtr expr)
{
....
if (binaryOp->Opcode == EBinaryOp::Divide)
{
....
}
else if (binaryOp->Opcode == EBinaryOp::RightShift)
{
TUnversionedValue value = literal->Value;
value.Data.Uint64 = 1 << value.Data.Uint64; // <=
value.Id = 0;
return TDivisors{value};
}
....
}
Предупреждение анализатора: V629 Consider inspecting the '1 << value.Data.Uint64' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. range_inferrer.cpp:378
Такой код считается потенциально опасным. Константа 1 типа int сдвигается влево на значение value.Data.Unit64. В результате значение такого выражения может быть не определено. Если значение value.Data.Uint64 будет лежать в диапазоне [32 … 63], произойдет знаковое переполнение. Оператор побитового сдвига не поменяет тип результирующего выражения, и он по-прежнему будет типа int. Даже несмотря на то, что правый операнд уже нужного типа. Пруф.
Анализатор свидетельствует о таком диапазоне на основании того, что результирующее выражение типа затем конвертируется в выражение типа большего размера. Поправить проблему достаточно просто, нужно всего лишь привести константу 1 к нужному типу:
value.Data.Uint64 = (uint64_t) 1 << value.Data.Uint64;
Вы спросите: «А что здесь опасного? Взорвется или не взлетит?»
Отвечу: «Взлететь то взлетит…»
Скорее всего сейчас всё работает исправно и будет работать, если не допускать переполнения. Однако «взорваться» и правда может. Нет смысла гадать, как проявит себя ошибка, когда вы вступите на территорию неопределённого поведения. А если очень хочется, то предлагаю почитать очень хорошую статью на эту тему: «Ложные представления программистов о неопределённом поведении».
Заключение
К безопасности в проекте отнеслись очень серьёзно. Разработчики ввели много дополнительных проверок и заботливо оставили комментарии, уточняющие логику происходящего. Всё чётко и красиво. Уважение.
Допускать ошибки — это нормально. Хороший программист должен знать, как надо писать код и как его писать не надо. Надеюсь, среди разбора фрагментов кода, предупреждений и ошибок вам удалось получить интересный или даже полезный для себя опыт.
Традиционно предлагаю попробовать наш анализатор PVS-Studio. Для Open Source проектов у нас предоставляется* бесплатная лицензия.*
Берегите себя и всего доброго!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexey Gorshkov. A deep look into YTsaurus. Availability, reliability, open source.
