Империя наносит ответный удар

Недавно вышла статья «Хакатон 2: Time lapse analysis of Unreal Engine 4», в которой рассказывается, как с помощью Klocwork можно найти множество ошибок в Unreal Engine 4. Просто не могу не прокомментировать эту статью. Дело в том, что когда-то мы исправили все ошибки, которые нашел анализатор PVS-Studio в этом проекте. Конечно, мы не работали над всеми ошибками, существующими в проекте, а только над теми, которые были обнаружены нашим анализатором. Однако статья создает впечатление, что анализатор PVS-Studio пропустил слишком много ошибок. Что ж, думаю, теперь моя очередь что-то сказать. Я также перепроверил Unreal Engine 4 и нашел множество других ошибок. Так что могу сказать, что PVS-Studio умеет находить новые баги в Unreal Engine 4. Ничья.

Справочник по истории

Все началось полтора года назад, когда я написал статью Долгожданная проверка Unreal Engine 4, которая привела к нашему сотрудничеству с компанией Epic Games, в результате которого были сняты все предупреждения, выданные PVS- Студия. За время работы мы исправили большое количество ошибок и убрали все ложные срабатывания анализатора. Наша команда предоставила компании Epic Games проект, свободный от предупреждений PVS-Studio. Вы можете прочитать эту статью «Как команда PVS-Studio улучшила код Unreal Engine», чтобы узнать больше.

Но не так давно я наткнулся на другую статью: Хакатон 2: Интервальный анализ Unreal Engine 4. И должен сказать, что эта статья качественная и очень информативная. В целом, Rogue Wave неплохо справляется с созданием такого мощного анализатора, как Klocwork, и организацией таких активностей, как проверка открытого кода. Мы также должны отдать должное Михаилу Грещищеву за проверку кода Unreal Engine и время, потраченное на написание статьи об этом. Это очень полезно для сообщества программистов. Но меня немного настораживает тот факт, что человек, мало знакомый со статическими анализаторами, может прийти к неверным выводам. Поэтому вынужден прокомментировать статью.

Непреднамеренно, но эта статья может выставить нашу команду в невыгодном свете по сравнению с Klocwork. Может показаться, что PVS-Studio находит меньше ошибок, чем Klocwork. Но правда в том, что этот мир сложнее. Оба анализатора имеют большое количество диагностик и частично эти диагностики могут пересекаться друг с другом. Но каждый анализатор имеет уникальный набор диагностик, поэтому проверив проект одним анализатором, вы всегда найдете что-то еще с помощью другого.

Еще одна маленькая деталь. Сторонние библиотеки мы не проверяли (по крайней мере, частично), а вот Михаил Грещищев это сделал, в чем мы можем убедиться, глядя на один из фрагментов кода (см. функцию HeadMountedDisplayCommon) В стороннем анализаторе PVS-Studio также легко найти много интересных недостатков. Более того, размер исходного кода ThirdParty в три раза больше, чем размер самого UE4.

Но это звучит как жалкая попытка оправдать нас :). Так что мне больше нечего делать, чтобы сравнять счет. Для этого мы скачали исходный код Unreal Engine 4 и перепроверили его с помощью PVS-Studio.

А сейчас я покажу вам, что вы всегда можете легко найти ошибки в больших быстро меняющихся проектах.

Результаты проверки PVS-Studio

Я проверил исходный код UE4 с помощью последней версии PVS-Studio. Сторонние библиотеки не были включены в процесс проверки. Иначе получился бы целый справочник, а не статья =)

Итак, у меня есть 1792 предупреждения общего анализа 1-го и 2-го уровня. Но не пугайтесь, я объясню, откуда эта цифра.

Большинство этих предупреждений (93%) выдаются из-за реализации нового диагностического правила V730, предназначенного для выявления неинициализированных членов класса. Неинициализированный член класса не всегда является ошибкой, но, тем не менее, это место в программе, на которое стоит обратить внимание. Вообще 1672 предупреждения диагностики V730 это много. Я не видел такого количества этих предупреждений в других проектах. Кроме того, анализатор пытается предугадать, вызовет ли неинициализированный член класса дальнейшие трудности или нет. Кстати, не очень благодарная работа — искать неинициализированные члены; возможно, нашим читателям было бы интересно узнать, почему. Вы можете ознакомиться с этой статьей «В поисках неинициализированных членов класса».

Но вернемся к UE4. В этой статье я не буду подробно рассказывать о предупреждениях V730. Их слишком много и я не могу сказать, что знаю проект UE4 достаточно хорошо, чтобы определить, приведут ли некоторые неинициализированные переменные к ошибке или нет. Однако я совершенно уверен, что среди этих предупреждений 1672 скрываются серьезные ошибки. Думаю, стоит их проанализировать. Даже если разработчики Epic Games посчитают эти предупреждения не более чем ложными срабатываниями, они могут легко отключить эту диагностику.

Итак, 1792–1672 = 120. Всего PVS-Studio выдало 120 предупреждений общего анализа (1 и 2 уровня) при проверке Unreal Engine. Большое количество этих предупреждений выявило настоящие ошибки. Рассмотрим подробнее наиболее интересные фрагменты кода и соответствующие предупреждения.

Интересные баги, найденные с помощью PVS-Studio

Подчеркну еще раз, что список ошибок, о которых я здесь говорю, далеко не полный. Во-первых, я мог пропустить что-то интересное, так как не имел возможности потратить приличное количество времени на просмотр всех фрагментов должным образом. Во-вторых, я не записал те ошибки, которые не очень критичны или требуют много пояснений (и фрагментов кода для объяснения).

Ошибка N1

FORCEINLINE
bool operator==(const FShapedGlyphEntryKey& Other) const
{
  return FontFace == Other.FontFace 
    && GlyphIndex == Other.GlyphIndex
    && FontSize == Other.FontSize
    && FontScale == Other.FontScale
    && GlyphIndex == Other.GlyphIndex;
}

Предупреждение PVS-Studio V501 Слева и справа от оператора && одинаковые подвыражения ‘GlyphIndex == Other.GlyphIndex’. кэш шрифтов. ч 139

GlyphIndex == Other.GlyphIndex проверяется дважды. Эффект последней строки в действии. Судя по всему, последнее сравнение должно быть: KeyHash == Other.KeyHash.

Ошибка N2

Еще один эффект последней строки, почти канонический.

bool
Compare(const FPooledRenderTargetDesc& rhs, bool bExact) const
{
  ....
  return Extent == rhs.Extent
    && Depth == rhs.Depth
    && bIsArray == rhs.bIsArray
    && ArraySize == rhs.ArraySize
    && NumMips == rhs.NumMips
    && NumSamples == rhs.NumSamples
    && Format == rhs.Format
    && LhsFlags == RhsFlags
    && TargetableFlags == rhs.TargetableFlags
    && bForceSeparateTargetAndShaderResource ==
         rhs.bForceSeparateTargetAndShaderResource
    && ClearValue == rhs.ClearValue
    && AutoWritable == AutoWritable;
}

Предупреждение PVS-Studio V501 Слева и справа от оператора == одинаковые подвыражения: AutoWritable == AutoWritable rendererinterface.h 180

В самом конце программист забыл добавить «rhs» и в результате переменная «AutoWritable» сравнивается сама с собой.

Ошибка N3

void AEQSTestingPawn::PostLoad() 
{
  ....
  UWorld* World = GetWorld();
  if (World && World->IsGameWorld() &&
      bTickDuringGame == bTickDuringGame)
  {
    PrimaryActorTick.bCanEverTick = false;
  }
}

Предупреждение PVS-Studio V501 Слева и справа от оператора == одинаковые подвыражения: bTickDuringGame == bTickDuringGame eqstestingpawn.cpp 157

Ошибка N4

int32 SRetainerWidget::OnPaint(....) const
{
  ....
  if ( RenderTargetResource->GetWidth() != 0 &&
       RenderTargetResource->GetWidth() != 0 )
  ....
}

Предупреждение PVS-Studio V501 Слева и справа от оператора && одинаковые подвыражения RenderTargetResource-›GetWidth() != 0. sretainerwidget.cpp 291

Ошибка N5, N6

Есть две похожие ошибки, расположенные близко друг к другу. Макросы ZeroMemory, которые являются простыми вызовами функции memset(), обнуляют только часть выделенной памяти.

class FD3D12BufferedGPUTiming
{
  ....
  FD3D12CLSyncPoint* StartTimestampListHandles;
  FD3D12CLSyncPoint* EndTimestampListHandles;
  ....
};
void FD3D12BufferedGPUTiming::InitDynamicRHI()
{
  ....
  StartTimestampListHandles = new FD3D12CLSyncPoint[BufferSize];
  ZeroMemory(StartTimestampListHandles,
             sizeof(StartTimestampListHandles));
  EndTimestampListHandles = new FD3D12CLSyncPoint[BufferSize];
  ZeroMemory(EndTimestampListHandles,
             sizeof(EndTimestampListHandles));
  ....
}

Предупреждения PVS-Studio:

• V512 Вызов функции memset приведет к недополнению буфера StartTimestampListHandles. d3d12query.cpp 493
• V512 Вызов функции memset приведет к недополнению буфера EndTimestampListHandles. d3d12query.cpp 495

Ошибка в том, что оператор sizeof() оценивает размер указателя, а не массива. Одним из правильных вариантов будет:

ZeroMemory(StartTimestampListHandles,
           sizeof(FD3D12CLSyncPoint) * BufferSize);
ZeroMemory(EndTimestampListHandles,
           sizeof(FD3D12CLSyncPoint) * BufferSize);

Ошибка N7

void FDeferredShadingSceneRenderer::RenderLight(....)
{
  ....
  if (bClearCoatNeeded)
  {
    SetShaderTemplLighting<false, false, false, true>(
      RHICmdList, View, *VertexShader, LightSceneInfo);
  }
  else
  {
    SetShaderTemplLighting<false, false, false, true>(
      RHICmdList, View, *VertexShader, LightSceneInfo);
  }
  ....
}

Предупреждение PVS-Studio V523 Оператор then эквивалентен оператору else. lightrendering.cpp 864

Вне зависимости от условий выполняются два аналогичных действия.

Ошибка N8

bool FBuildDataCompactifier::Compactify(....) const
{
  ....
  uint64 CurrentFileSize;
  ....
  CurrentFileSize = IFileManager::Get().FileSize(*File);
  if (CurrentFileSize >= 0)
  {
    ....
  }
  else
  {
    GLog->Logf(TEXT("Warning. ......"), *File);
  }
  ....
}

Предупреждение PVS-Studio V547 Выражение CurrentFileSize ›= 0 всегда истинно. Значение беззнакового типа всегда ›= 0. buildpatchcompactifier.cpp 135

«if (CurrentFileSize › = 0)» проверять не имеет смысла. Переменная «CurrentFileSize» имеет беззнаковый тип, поэтому ее значение всегда › = 0.

Ошибка N9

template<typename TParamRef>
void UnsetParameters(....)
{
  ....
  int32 NumOutUAVs = 0;
  FUnorderedAccessViewRHIParamRef OutUAVs[3];
  OutUAVs[NumOutUAVs++] = ObjectBuffers......;
  OutUAVs[NumOutUAVs++] = ObjectBuffers.Bounds.UAV;
  OutUAVs[NumOutUAVs++] = ObjectBuffers.Data.UAV;
  if (CulledObjectBoxBounds.IsBound())
  {
    OutUAVs[NumOutUAVs++] = ObjectBuffers.BoxBounds.UAV;
  }
  ....
}

V557 Возможно переполнение массива. Индекс «NumOutUAVs ++» указывает за границу массива. DistanceFieldlightingShared.h 388

Если выполняется условие (CulledObjectBoxBounds.IsBound()), то индекс массива выходит за границы. Обратите внимание, что массив OutUAVs состоит всего из 3 элементов.

Ошибка N10

class FSlateDrawElement
{
  ....
  FORCEINLINE void SetPosition(const FVector2D& InPosition)
  { Position = Position; }
  ....
  FVector2D Position;
  ....
};

Предупреждение PVS-Studio V570 Переменная Position присваивается самой себе. элементы рисования.h 435

На этот баг даже не стоит тратить время, это просто опечатка. Мы должны иметь:

{ Позиция = InPosition; }.

Ошибка N11

bool FOculusRiftHMD::DoesSupportPositionalTracking() const
{
  const FGameFrame* frame = GetFrame();
  const FSettings* OculusSettings = frame->GetSettings();
  return (frame && OculusSettings->Flags.bHmdPosTracking &&
          (OculusSettings->SupportedTrackingCaps &
           ovrTrackingCap_Position) != 0);
}

Предупреждение PVS-Studio V595 Указатель ‘frame’ использовался до проверки на nullptr. Проверить строки: 301, 302. oculusrifthmd.cpp 301

Мы видим, что сначала используется переменная «frame», а затем проверяется, равна ли она нулю.

Эта ошибка очень похожа на ту, что описана в статье Klocwork:

bool FHeadMountedDisplay::IsInLowPersistenceMode() const
{
    const auto frame = GetCurrentFrame();
    const auto FrameSettings = frame->Settings;
    return frame && FrameSettings->Flags.bLowPersistenceMode;
}

Как видите, оба анализатора могут идентифицировать этот тип дефекта.

Стоит отметить, что код, приведенный в статье Klocwork, относится к репозиторию ThirdParty, который мы не проверяли.

Ошибка N12 — N21

FName UKismetNodeHelperLibrary::GetEnumeratorName(
  const UEnum* Enum, uint8 EnumeratorValue)
{
  int32 EnumeratorIndex = Enum->GetIndexByValue(EnumeratorValue);
  return (NULL != Enum) ?
         Enum->GetEnum(EnumeratorIndex) : NAME_None;
}

Предупреждение PVS-Studio V595 Указатель Enum был использован до того, как он был проверен на nullptr. Проверить строки: 146, 147. kismetnodehelperlibrary.cpp 146

Снова имеем ситуацию, когда указатель сначала разыменовывается, а только потом проверяется. Смотреть на такие ошибки довольно скучно. Я просто перечислю фрагменты, на которые стоит обратить внимание:

• V595 Указатель «Класс» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 278, 282. levelactor.cpp 278
• V595 Указатель «Шаблон» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 380, 386. levelactor.cpp 380
• V595 Указатель «UpdatedComponent» использовался до того, как он был проверен на соответствие nullptr. Строки проверки: 100, 116. interptomovementcomponent.cpp 100
• V595 Указатель «SourceTexture» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 150, 178. d3d12rendertarget.cpp 150
• V595 Указатель NewRenderTarget использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 922, 924. d3d11commands.cpp 922
• V595 Указатель «RenderTarget» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 2173, 2175. d3d11commands.cpp 2173
• V595 Указатель «MyMemory» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 210, 217. bttask_moveto.cpp 210
• V595 Указатель «SkelComp» использовался до того, как он был проверен на соответствие nullptr. Контрольные строки: 79, 100. animnode_animdynamics.cpp 79
• V595 Указатель «Результат» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 1000, 1004. uobjectglobals.cpp 1000

Ошибка N22

class FD3D12Device
{
  ....
  virtual void InitD3DDevice();
  virtual void CleanupD3DDevice();
  ....
  // Destructor is not declared
  ....
};

V599 Виртуальный деструктор отсутствует, хотя класс FD3D12Device содержит виртуальные функции. d3d12device.cpp 448

В классе FD3D12Device есть виртуальные методы. Это означает, что у этого класса будут производные классы.
Однако в этом классе нет виртуального деструктора. Это очень опасно и, скорее всего, рано или поздно приведет к ошибке.

Ошибка N23 — N26

int SpawnTarget(WCHAR* CmdLine)
{
  ....
  if(!CreateProcess(....))
  {
    DWORD ErrorCode = GetLastError();
    WCHAR* Buffer = new WCHAR[wcslen(CmdLine) + 50];
    wsprintf(Buffer,
             L"Couldn't start:\n%s\nCreateProcess() returned %x.",
             CmdLine, ErrorCode);
    MessageBoxW(NULL, Buffer, NULL, MB_OK);
    delete Buffer;
    return 9005;
  }
  ....
}

Предупреждение PVS-Studio V611 Память была выделена с помощью оператора new T[], но освобождена с помощью оператора delete. Рассмотрите возможность проверки этого кода. Вероятно, лучше использовать «delete [] Buffer;». bootstrappackagedgame.cpp 110

Выделенная память освобождается неправильным образом. Это должно быть так:

delete [] Buffer;

Еще пара подобных ошибок:

• V611 Память была выделена с помощью оператора «новый T[]», но освобождена с помощью оператора «удалить». Рассмотрите возможность проверки этого кода. Вероятно, лучше использовать «delete [] ChildCmdLine;». bootstrappackagedgame.cpp 157
• V611 Память была выделена с помощью оператора «новый T[]», но освобождена с помощью оператора «удалить». Рассмотрите возможность проверки этого кода. Вероятно, лучше использовать «delete [] ChildCmdLine;». bootstrappackagedgame.cpp 165
• V611 Память была выделена с помощью оператора «новый T[]», но освобождена с помощью оператора «удалить». Рассмотрите возможность проверки этого кода. Вероятно, лучше использовать «delete [] ChildCmdLine;». bootstrappackagedgame.cpp 169

Ошибка N27

void FSlateTexture2DRHIRef::InitDynamicRHI()
{
  ....
  checkf(GPixelFormats[PixelFormat].BlockSizeX ==
         GPixelFormats[PixelFormat].BlockSizeY ==
         GPixelFormats[PixelFormat].BlockSizeZ == 1,
         TEXT("Tried to use compressed format?"));
  ....
}

Предупреждение PVS-Studio V709 Обнаружено подозрительное сравнение: ‘a == b == c’. Помните, что «a == b == c» не равно «a == b && b == c». сланцевые текстуры.cpp 67

Проверка работает не так, как хотел программист. Вместо этого мы должны написать:

GPixelFormats[PixelFormat].BlockSizeX == 1 &&
GPixelFormats[PixelFormat].BlockSizeY == 1 &&
GPixelFormats[PixelFormat].BlockSizeZ == 1

Ошибка N28

void UWidgetComponent::UpdateRenderTarget()
{
  ....
  FLinearColor ActualBackgroundColor = BackgroundColor;
  switch ( BlendMode )
  {
  case EWidgetBlendMode::Opaque:
    ActualBackgroundColor.A = 1.0f;
  case EWidgetBlendMode::Masked:
    ActualBackgroundColor.A = 0.0f;
  }
  ....
}

V519 Переменной ActualBackgroundColor.A дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 938, 940. widgetcomponent.cpp 940

Здесь мы видим, что пропущенный оператор break обнаружен. Переменной ActualBackgroundColor.A можно присвоить два разных значения два раза подряд. Это и вызывает подозрения у анализатора.

Ошибка N29

void FProfilerManager::TrackDefaultStats()
{
  // Find StatId for the game thread.
  for( auto It = GetProfilerInstancesIterator(); It; ++It )
  {
    FProfilerSessionRef ProfilerSession = It.Value();
    if( ProfilerSession->GetMetaData()->IsReady() )
    {
      ....;
    }
    break;
  }
}

Предупреждение PVS-Studio V612 Безусловный «разрыв» внутри цикла. profilermanager.cpp 717

Это очень подозрительный фрагмент кода. Кажется, что оператор break не на своем месте. Я не совсем уверен, но, возможно, это должно быть написано так:

for( auto It = GetProfilerInstancesIterator(); It; ++It )
{
  FProfilerSessionRef ProfilerSession = It.Value();
  if( ProfilerSession->GetMetaData()->IsReady() )
  {
    ....;
    break;
  }
}

Общие результаты

Как минимум 29 из 120 предупреждений, выданных PVS-Studio, указывали на реальные ошибки (24%). Еще 50% — код, который воняет. Остальные - ложные срабатывания. Общее количество времени, затраченного на проверку проекта и написание статьи, составляет около 10 часов.

Выводы, которые можно сделать по результатам проверки анализатора PVS-Studio и Klocwork:

• В большом и быстроразвивающемся проекте всегда можно найти больше багов :)
• Наборы диагностик в PVS-Studio и Klocwork разные, но некоторые диагностики похожи.
• Возможно, Klocwork проверял Unreal Engine 4, включая сторонние библиотеки (ThirdParty). Мы их вообще не проверяли.
• Оба анализатора отлично поработали. Их использование может быть очень полезным для разработки программы.

Спасибо за внимание.

Статья опубликована с разрешения автора.