Download

[BUG?] Bad memory access using FTimerManager

I encountered some unexpected behaviors when I was using the timer objects by FTimerManager.
I already spotted the problem and I was able to replicate it in a very simple exampling class:


    #pragma once
    #include "GameFramework/Actor.h"
    #include "Engine/World.h"
    #include "TimerManager.h"
    #include "Test.generated.h"

    class Actor_B;

    UCLASS(NotBlueprintable, HideDropdown)
    class AActor_A : public AActor
    {
        GENERATED_BODY()

    public:
        void SetActor_B(AActor_B* InActor_B) { Actor_B = InActor_B; }
        void Foo();

    private:
        float var_A { 0.0f };
        FTimerHandle Timer_A{};
        AActor_B* Actor_B{};
    };


    UCLASS(Blueprintable, BlueprintType)
    class AActor_B : public AActor
    {
        GENERATED_BODY()
    public:
        AActor_B(const FObjectInitializer& InObjectInitializer)
        {
            RootComponent = CreateDefaultSubobject<USceneComponent>(TEXT("SceneComponent_B"));
        }

        virtual void BeginPlay() override
        {
            Super::BeginPlay();
            Actor_A = GetWorld()->SpawnActor<AActor_A>();
            Actor_A->SetActor_B(this);
            auto lambda_B = [this]()
            {
                auto* thisCopy = this;
                auto* Actor_ACopy = Actor_A;
                Actor_A->Foo();
                ensureAlways(this == thisCopy);
                var_B = 5;
                ensureAlways(thisCopy->var_B == 5);
            };
            GetWorld()->GetTimerManager().SetTimer(Timer_B, std::move(lambda_B), 10.0f, false);
        }
    private:
        int32 var_B{ 0 };
        FTimerHandle Timer_B{};
        AActor_A* Actor_A{};
    public:
        void ClearTimer() { GetWorld()->GetTimerManager().ClearTimer(Timer_B); }
    };

    void AActor_A::Foo()
    {
        Actor_B->ClearTimer();
        auto lambda_A = [this]() {};
        GetWorld()->GetTimerManager().SetTimer(Timer_A, std::move(lambda_A), 3.0f, false);
    }


In the example above we have AActor_A and AActor_B classes.
I will try to explain briefly what happens:

  • the *AActor_B *spawns a instance of AActor_A;
  • the *AActor_B *creates a new Timer_B that will be execute the lambda_B lambda function;
  • after the Timer_B has expired, during the execution of lambda_B, AActor_A::Foo() function is called;
  • AActor_A::Foo() clears the timer member of AActor_B;
  • AActor_A::Foo() creates a new timer Timer_A that will execute a lambda_A function;
  • *lambda_A *lambda function captures the this of AActor_A instance;
  • when AActor_A::Foo() has finished to execute then the execution of lambda_B continues;
  • the this captured in lambda_B is changed now! Indeed the ensure macro call (ensureAlways(this == thisCopy)) is always triggered!

At this point the execution of lambda_B is very dangerous since some bad memory access could occur. For example the assignment var_B = 5 is not executed how we expected, in fact that value is written in var_A private member of the AActor_A instance!

I suppose this could be a misusing of timer and a timer should be not cleared while its lambda is executing but I think it’s not so unlikely encounter a similar case and it could occur without our realizing it.
Indeed, in a more complex scenario, lambda_B would have been able trigger a chain of events where we can observe a similar issue.

Checking the FTimerManager implementation I noticed this case is handled, but the timer is removed immediately:


    void FTimerManager::InternalClearTimer(FTimerHandle InHandle)
    {
        SCOPE_CYCLE_COUNTER(STAT_ClearTimer);

        // not currently threadsafe
        check(IsInGameThread());

        FTimerData& Data = GetTimer(InHandle);
        switch (Data.Status)
        {
            case ETimerStatus::Pending:
                {
                    int32 NumRemoved = PendingTimerSet.Remove(InHandle);
                    check(NumRemoved == 1);
                    RemoveTimer(InHandle);
                }
                break;

            case ETimerStatus::Active:
                Data.Status = ETimerStatus::ActivePendingRemoval;
                break;

            case ETimerStatus::ActivePendingRemoval:
                // Already removed
                break;

            case ETimerStatus::Paused:
                {
                    int32 NumRemoved = PausedTimerSet.Remove(InHandle);
                    check(NumRemoved == 1);
                    RemoveTimer(InHandle);
                }
                break;

            case ETimerStatus::Executing:
                check(CurrentlyExecutingTimer == InHandle);

                // Edge case. We're currently handling this timer when it got cleared.  Clear it to prevent it firing again
                // in case it was scheduled to fire multiple times.
                CurrentlyExecutingTimer.Invalidate();
                RemoveTimer(InHandle);
                break;

            default:
                check(false);
        }
    }

I think the timer could be set into a new state, we could call it ETimerStatus::ExecutingPendingRemoval, and it will be cleared as soon as possible.
Is it a bad idea?

Any opinion?

It’s interesting, but I don’t know if I would call it a bug. The only possible way to resolve it would be to prevent removal of executing timer lambdas. Of course, I’m not an Epic engineer, so who knows if they’d call it a bug! :slight_smile:

The invalidation of “this” is a consequence of how lambdas are implemented in C++ (see https://stackoverflow.com/questions/…-it-is-running)

You’re also new’ing up an Object but you aren’t using UPROPERTY’s to track them so the GC will get them as soon as it runs (I don’t think FTimerManager keeps the objects alive). I’m not sure the object isn’t just getting deleted due to the normal flow of things and then the timer executes on a dead object.

UPROPERTY macro is not needed if the member is not a pointer to a UObject pointer. The GC tracks UObject pointers starting from root set and in this case we have not a dangling pointer refers to a deleted object.

Your link talks about std::function, but Unreal Engine doesn’t use massively the standard library. Indeed the timers of FTimerManager class use a lambda function declared as TFunction<void(void)> FuncCallback;. However thank you for the link, it is very interesting and maybe the issue can be similar.