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

    Дарт Вейдер - Единорог
    Недавно появилась статья «Hackathon 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-Studio. В результате было поправлено множество ошибок и устранены все ложные срабатывания анализатора. Наша команда выдала компании Epic Games проект, свободный от предупреждений PVS-Studio. Как это происходило, можно узнать из статьи "Как команда PVS-Studio улучшила код Unreal Engine".

    Недавно на просторах интернета я наткнулся на статью "Hackathon 2: Time lapse analysis of Unreal Engine 4". Статья хорошая и корректная. Компания Rogue Wave молодец, что проводит такие мероприятия и делает мощный статический анализатор кода Klocwork. Michail Greshishchev тоже молодец, что проделал работу по проверке Unreal Engine и нашёл время написать про это статью. Всё хорошо. Но я обеспокоен, что сторонний человек, мало знакомый со статическими анализаторами, может сделать неправильные выводы. Поэтому я обязан прокомментировать эту статью.

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

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

    Однако всё это звучит как жалкие попытки оправдаться :). Поэтому мне не остаётся ничего другого, как сравнять счёт. С этой целью были взяты исходники Unreal Engine 4 и проверены последней версией анализатора PVS-Studio.

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

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


    Я проверил исходные коды UE4 последней версией PVS-Studio. Проверялись исходные коды UE4 без учета каталога ThirdParty. Если я их проверю, у меня получится не статья, а справочник :).

    Я получил 1792 предупреждения общего назначения первого и второго уровня. Не надо пугаться. Сейчас станет понятно, откуда взялось это число.

    Большинство предупреждений (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 There are identical sub-expressions 'GlyphIndex == Other.GlyphIndex' to the left and to the right of the '&&' operator. fontcache.h 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 There are identical sub-expressions to the left and to the right of the '==' operator: 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 There are identical sub-expressions to the left and to the right of the '==' operator: bTickDuringGame == bTickDuringGame eqstestingpawn.cpp 157

    Ошибка N4
    int32 SRetainerWidget::OnPaint(....) const
    {
      ....
      if ( RenderTargetResource->GetWidth() != 0 &&
           RenderTargetResource->GetWidth() != 0 )
      ....
    }

    Предупреждение PVS-Studio: V501 There are identical sub-expressions 'RenderTargetResource->GetWidth() != 0' to the left and to the right of the '&&' operator. 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 A call of the 'memset' function will lead to underflow of the buffer 'StartTimestampListHandles'. d3d12query.cpp 493
    • V512 A call of the 'memset' function will lead to underflow of the buffer '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 The 'then' statement is equivalent to the 'else' statement. 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 Expression 'CurrentFileSize >= 0' is always true. Unsigned type value is always >= 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 Array overrun is possible. The 'NumOutUAVs ++' index is pointing beyond array bound. distancefieldlightingshared.h 388

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

    Ошибка N10
    class FSlateDrawElement
    {
      ....
      FORCEINLINE void SetPosition(const FVector2D& InPosition)
      { Position = Position; }
      ....
      FVector2D Position;
      ....
    };

    Предупреждение PVS-Studio: V570 The 'Position' variable is assigned to itself. drawelements.h 435

    Эту ошибку даже описывать не нужно. Опечатка. Должно быть: { Position = 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 The 'frame' pointer was utilized before it was verified against nullptr. Check lines: 301, 302. oculusrifthmd.cpp 301

    Сначала переменная 'frame' используется, а затем проверяется на равенство nullptr.

    Эта ошибка очень схожа с той, что описана в статье 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 The 'Enum' pointer was utilized before it was verified against nullptr. Check lines: 146, 147. kismetnodehelperlibrary.cpp 146

    Опять ситуация, когда в начале указатель разыменован, и только затем проверен. Рассматривать такие ошибки дальше скучно. Просто приведу списком места, на которые стоит обратить внимание:
    • V595 The 'Class' pointer was utilized before it was verified against nullptr. Check lines: 278, 282. levelactor.cpp 278
    • V595 The 'Template' pointer was utilized before it was verified against nullptr. Check lines: 380, 386. levelactor.cpp 380
    • V595 The 'UpdatedComponent' pointer was utilized before it was verified against nullptr. Check lines: 100, 116. interptomovementcomponent.cpp 100
    • V595 The 'SourceTexture' pointer was utilized before it was verified against nullptr. Check lines: 150, 178. d3d12rendertarget.cpp 150
    • V595 The 'NewRenderTarget' pointer was utilized before it was verified against nullptr. Check lines: 922, 924. d3d11commands.cpp 922
    • V595 The 'RenderTarget' pointer was utilized before it was verified against nullptr. Check lines: 2173, 2175. d3d11commands.cpp 2173
    • V595 The 'MyMemory' pointer was utilized before it was verified against nullptr. Check lines: 210, 217. bttask_moveto.cpp 210
    • V595 The 'SkelComp' pointer was utilized before it was verified against nullptr. Check lines: 79, 100. animnode_animdynamics.cpp 79
    • V595 The 'Result' pointer was utilized before it was verified against nullptr. Check lines: 1000, 1004. uobjectglobals.cpp 1000

    Ошибка N22
    class FD3D12Device
    {
      ....
      virtual void InitD3DDevice();
      virtual void CleanupD3DDevice();
      ....
      // Деструктор не объявлен
      ....
    };

    V599 The virtual destructor is not present, although the 'FD3D12Device' class contains virtual functions. 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 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] Buffer;'. bootstrappackagedgame.cpp 110

    Неправильным способом освобождается выделенная память. Должно быть написано:
    delete [] Buffer;

    Есть ещё несколько таких ошибок:
    • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] ChildCmdLine;'. bootstrappackagedgame.cpp 157
    • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] ChildCmdLine;'. bootstrappackagedgame.cpp 165
    • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use '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 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. slatetextures.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 The 'ActualBackgroundColor.A' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 938, 940. widgetcomponent.cpp 940

    Здесь косвенным образом выявлен пропущенный оператор 'break'. Переменной 'ActualBackgroundColor.A' два раза подряд могут быть присвоены разные значения. Это и настораживает анализатор PVS-Studio.

    Ошибка 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 An unconditional 'break' within a loop. 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:
    1. В большом и быстро развивающемся проекте всегда можно найти ещё немножко ошибок :)
    2. Наборы диагностик PVS-Studio и Klocwork различаются, хотя и есть пересечение.
    3. Возможно, Klocwork проверял Unreal Engine 4, включая сторонние библиотеки (ThirdParty). Мы ни тогда, ни сейчас эти библиотеки не проверяли.
    4. Оба анализатора молодцы, и их использование может приносить массу пользы процессу разработки программ.

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


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The Empire Strikes Back.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Похожие публикации

    Комментарии 5

      +4
      Эффект последней строки очень интересная штука. Поидее в любом проекте если проанализировать и найти рядом повторяющиеся конструкции то есть вероятность ошибки при copy-paste. И тут даже не важно код на каком языке анализировать.
        0
        Так оно и есть. Скоро будет первая статья про проверку первого C# проекта. И там как раз будет этот эффект.

    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

    Самое читаемое