Empire strikes back

    Darth Vader - Unicorn
    Recently, an article “Hackathon 2: Time lapse analysis of Unreal Engine 4” appeared, which tells how, using the Klocwork tool, you can find many errors in the Unreal Engine 4 project. I can’t get past this article. The fact is that in due time we fixed all the errors that the PVS-Studio analyzer found in this project. It is clear that not all errors were corrected at all, but only detected by the analyzer. However, the article gives the impression that the PVS-Studio analyzer missed too much. Well, now it's my turn. I also rechecked Unreal Engine 4 and found a ton of errors. Thus, I can say that PVS-Studio can also find new errors in Unreal Engine 4 now. Draw.

    Historical reference


    It all started a year and a half ago when I wrote the article "The Long-awaited Test of Unreal Engine 4 ". Then there was a collaboration with Epic Games, the result of which was the elimination of all warnings issued by PVS-Studio. As a result, many errors were corrected and all false positives of the analyzer were eliminated. Our team issued a project to Epic Games that is free of PVS-Studio alerts. How this happened can be found in the article “ How the PVS-Studio Team Improved Unreal Engine Code ”.

    Recently, on the Internet, I came across an article " Hackathon 2: Time lapse analysis of Unreal Engine 4". The article is good and correct. Rogue Wave is great for hosting such events and making Klocwork a powerful static code analyzer. Michail Greshishchev is also great for doing the Unreal Engine verification work and taking the time to write an article about it. That's all right. But I'm worried that an outsider who is not familiar with static analyzers can draw the wrong conclusions, so I must comment on this article.

    Unintentionally, but to an uninitiated reader, this article shows us in a disadvantageous light compared to Klocwork. PVS-Studio seems to find fewer errors than Klocwork. In fact, the world is more complex. Both analyzers have different sets of diagnostics. Partially these sets intersect. However, each of the analyzers has a unique set of diagnostics. Thus, having checked a large project with one analyzer, you will always find something with another analyzer.

    Another nuance. We did not check third-party libraries (the ThirdParty directory), and Michail Greshishchev checked (at least partially), as evidenced by one of the code examples (see the HeadMountedDisplayCommon function). In ThirdParty, the PVS-Studio analyzer can also easily find many interesting defects. Moreover, the size of ThirdParty sources is 3 times larger than UE4 itself.

    However, all this sounds like pathetic attempts to justify :). Therefore, I have no choice but to level the score. For this purpose, the sources of Unreal Engine 4 were taken and checked by the latest version of the PVS-Studio analyzer.

    And now I will demonstrate that you can always easily find errors in a large fast-paced project.

    Project Validation Results Using PVS-Studio


    I checked the source code of UE4 with the latest version of PVS-Studio. The source codes of UE4 were checked without regard to the ThirdParty catalog. If I check them, I will not get an article, but a reference :).

    I received 1792 first and second level general purpose warnings. Do not be scared. Now it will become clear where this number came from.

    Most warnings (93%) are associated with the new V730 diagnostic rule, designed to identify uninitialized class members. An uninitialized member of a class is not always a mistake, but nevertheless this is the place in the program that is worth checking. In general, 1672 warnings of the V730 are many. On other projects, I have not seen a similar amount of these warnings. Moreover, if possible, the analyzer tries to predict when an uninitialized member is not scary. By the way, the search for uninitialized members is an ungrateful job, and perhaps readers will be interested in reading the article “ Search for uninitialized class members ”.

    But back to the UE4 project. In the article I will not touch on the warning V730. There are too many of them, and I know too little the UE4 project to judge whether this or that uninitialized variable can lead to an error in the program. However, I am sure that among these 1672 warnings there are quite a few serious serious errors. I think the developers at Epic Games should analyze them. If they consider that these are continuous false positives, then this diagnosis can simply be turned off.

    So, 1792 - 1672 = 120. In total, PVS-Studio issued 120 general-purpose warnings (level 1 and 2) when checking the Unreal Engine. Many of these warnings revealed real errors. Consider the most interesting pieces of code and their corresponding warnings.

    Interesting errors found with PVS-Studio


    I emphasize once again that in the article I will not give all the sections of code that deserve attention and editing. Firstly, I watched the report fluently and could miss something interesting. Secondly, I did not write out frivolous errors or those that are difficult to explain (many code fragments are required).

    Error 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 Warning: V501 There are identical sub-expressions 'GlyphIndex == Other.GlyphIndex' to the left and to the right of the '&&' operator. fontcache.h 139

    The “GlyphIndex == Other.GlyphIndex” check is repeated twice. The effect of the last line in action. Apparently, the last comparison should be: KeyHash == Other.KeyHash.

    Error N2

    Another effect of the last line. Directly canonical.
    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 warning: V501 There are identical sub-expressions to the left and to the right of the '==' operator: AutoWritable == AutoWritable rendererinterface.h 180

    At the very end, they forgot to add “rhs.” And as a result the variable “AutoWritable 'compared to itself.

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

    PVS-Studio Warning: V501 There are identical sub-expressions to the left and to the right of the '==' operator: bTickDuringGame == bTickDuringGame eqstestingpawn.cpp 157

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

    PVS-Studio warning: V501 There are identical sub-expressions 'RenderTargetResource-> GetWidth ()! = 0' to the left and to the right of the '&&' operator. sretainerwidget.cpp 291

    Error N5, N6

    Two identical errors are located in the neighborhood. ZeroMemory macros, which are nothing more than calls to the memset () function, only zero out part of the allocated memory.
    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 warnings:
    • 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

    The error is that the sizeof () operator calculates the size of the pointer, not the array. One of the correct options would be to write this:
    ZeroMemory(StartTimestampListHandles,
               sizeof(FD3D12CLSyncPoint) * BufferSize);
    ZeroMemory(EndTimestampListHandles,
               sizeof(FD3D12CLSyncPoint) * BufferSize);

    Error N7
    void FDeferredShadingSceneRenderer::RenderLight(....)
    {
      ....
      if (bClearCoatNeeded)
      {
        SetShaderTemplLighting(
          RHICmdList, View, *VertexShader, LightSceneInfo);
      }
      else
      {
        SetShaderTemplLighting(
          RHICmdList, View, *VertexShader, LightSceneInfo);
      }
      ....
    }

    PVS-Studio Warning: V523 The 'then' statement is equivalent to the 'else' statement. lightrendering.cpp 864

    Regardless of the condition, the same actions are performed.

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

    PVS-Studio warning: V547 Expression 'CurrentFileSize> = 0' is always true. Unsigned type value is always> = 0. buildpatchcompactifier.cpp 135

    Checking "if (CurrentFileSize> = 0)" does not make sense. The variable 'CurrentFileSize' has an unsigned type, which means its value is always> = 0.

    Error N9
    template
    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

    If the condition (CulledObjectBoxBounds.IsBound ()) is fulfilled, then the array will be exceeded. Note that the array 'OutUAVs' consists of only 3 elements.

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

    PVS-Studio Warning: V570 The 'Position' variable is assigned to itself. drawelements.h 435

    This error does not even need to be described. Typo. It should be: {Position = InPosition; }.

    Error 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 Warning: V595 The 'frame' pointer was utilized before it was verified against nullptr. Check lines: 301, 302. oculusrifthmd.cpp 301

    First, the variable 'frame' is used, and then it is checked for nullptr equality.

    This error is very similar to the one described in the Klocwork article :
    bool FHeadMountedDisplay::IsInLowPersistenceMode() const
    {
        const auto frame = GetCurrentFrame();
        const auto FrameSettings = frame->Settings;
        return frame && FrameSettings->Flags.bLowPersistenceMode;
    }

    As you can see, both analyzers can detect this type of defect.

    It is worth noting that the code provided in the Klocwork article refers to the ThirdParty directory, from which we did not check the projects.

    Error 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 Warning: V595 The 'Enum' pointer was utilized before it was verified against nullptr. Check lines: 146, 147. kismetnodehelperlibrary.cpp 146

    Again, the situation is when at the beginning the pointer is dereferenced, and only then checked. To consider such errors further is boring. Just a list of places worth paying attention to:
    • 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

    Error 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

    There are virtual methods in the FD3D12Device class. This means that it is assumed that this class will have heirs. However, there is no virtual destructor in this class. This is very dangerous and most likely will lead to an error sooner or later.

    Error 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 warning: 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 The

    allocated memory is freed in the wrong way. It should be written:
    delete [] Buffer;

    There are a few more such errors:
    • 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

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

    PVS-Studio warning: V709 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. slatetextures.cpp 67

    Verification does not work at all as the programmer intended. Apparently you should write:
    GPixelFormats[PixelFormat].BlockSizeX == 1 &&
    GPixelFormats[PixelFormat].BlockSizeY == 1 &&
    GPixelFormats[PixelFormat].BlockSizeZ == 1

    Error 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

    Here, the missing 'break' operator is indirectly detected. The variable 'ActualBackgroundColor.A' can be assigned different values ​​two times in a row. This is what worries the PVS-Studio analyzer.

    Error 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 Warning: V612 An unconditional 'break' within a loop. profilermanager.cpp 717

    Very suspicious code. The 'break' statement seems to be out of place. I'm not sure, but maybe it was planned to write like this:
    for( auto It = GetProfilerInstancesIterator(); It; ++It )
    {
      FProfilerSessionRef ProfilerSession = It.Value();
      if( ProfilerSession->GetMetaData()->IsReady() )
      {
        ....;
        break;
      }
    }

    Summary


    At least 29 out of 120 warnings issued by PVS-Studio indicated real errors (24%). Another 50% is a code that smells. The rest is false positives. The total time I spent on checking the project and writing this article is about 10 hours.

    What conclusions can be drawn from the results of the PVS-Studio and Klocwork analyzers:
    1. In a large and rapidly developing project you can always find a little more errors :)
    2. PVS-Studio and Klocwork diagnostic sets are different, although there is an intersection.
    3. Klocwork may have tested Unreal Engine 4, including third-party libraries (ThirdParty). We did not check these libraries either then or now.
    4. Both analyzers are great, and their use can bring a lot of benefits to the program development process.

    Thank you all for your attention.


    If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. The Empire Strikes Back .

    Have you read the article and have a question?
    Often our articles are asked the same questions. We collected the answers here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please see the list.

    Also popular now: