Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect destructor order call #123

Closed
01Pollux opened this issue Apr 27, 2024 · 4 comments
Closed

Incorrect destructor order call #123

01Pollux opened this issue Apr 27, 2024 · 4 comments
Assignees
Labels
bug security/UB Undefined behavior and security concerns
Milestone

Comments

@01Pollux
Copy link

01Pollux commented Apr 27, 2024

Describe the bug
When attempting to access the resource X that we know depends on Y, it should be valid to access the Y resource during X's destructor, but from my use cases, it's solely dependant on order of instantiation of service, or KGR_KANGARU_REVERSE_DESTRUCTION which just reverse release the resources.

To Reproduce
1- Create the main.cpp with the following content:

#include <string>
#include <iostream>
#include <kangaru/kangaru.hpp>

struct Noisy
{
    std::string Name;

    Noisy(const char* Name) :
        Name(Name)
    {
        printf("Noisy::Noisy(%s);\n", Name);
    }

    ~Noisy()
    {
        printf("Noisy::~Noisy(%s);\n", Name.c_str());
    }
};

struct System1 : Noisy
{
public:
    System1() :
        Noisy("System1")
    {
    }
};

struct System2 : Noisy
{
    System2(System1& sys) :
        Noisy("System2"),
        dep(sys)
    {
    }

    System1& dep;
};

struct Subsystem1 : kgr::autowire_single_service<System1>
{
};

struct Subsystem2 : kgr::autowire_single_service<System2>
{
};

auto service_map(System1 const&) -> Subsystem1;
auto service_map(System2 const&) -> Subsystem2;

int main()
{
    printf("// 1 then 2\n");
    {
        kgr::container container;

        auto& a = container.service<Subsystem1>();
        auto& b = container.service<Subsystem2>();
    }
    printf("\n");
    printf("// 2 then 1\n");
    {
        kgr::container container;

        auto& b = container.service<Subsystem2>();
        auto& a = container.service<Subsystem1>();
    }
    printf("\n");
    printf("// 2 only\n");
    {
        kgr::container container;

        auto& b = container.service<Subsystem2>();
    }
    return 0;
}

Expected behavior
The program should output the following regardless of KANGARU_REVERSE_DESTRUCTION value

// 1 then 2
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System2);
Noisy::~Noisy(System1);

// 1 then 2
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System2);
Noisy::~Noisy(System1);

// 2 only
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System2);
Noisy::~Noisy(System1);

With KANGARU_REVERSE_DESTRUCTION enabled

// 1 then 2
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System2);
Noisy::~Noisy(System1);

// 1 then 2
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System1);
Noisy::~Noisy(System2);

// 2 only
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System1);
Noisy::~Noisy(System2);

Without KANGARU_REVERSE_DESTRUCTION

// 1 then 2
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System1);
Noisy::~Noisy(System2);

// 1 then 2
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System2);
Noisy::~Noisy(System1);

// 2 only
Noisy::Noisy(System1);
Noisy::Noisy(System2);
Noisy::~Noisy(System2);
Noisy::~Noisy(System1);

Desktop (please complete the following information):

  • OS: Windows
  • Compiler MSVC
  • Version 19.39.33523

Additional context
This is an important issue that mustn't be overlooked as it violates C++ order of destructors.

@gracicot
Copy link
Owner

Thank you for the report. I'll try to see what is possible for kangaru 4. Changing the destruction is a breaking change and cannot happen without an explicit compile time switch or a new major version.

@01Pollux
Copy link
Author

The workaround to handle such cases until a fix is implemented might not be ideal but is to use shared_ptr instead of references.

@gracicot gracicot added bug security/UB Undefined behavior and security concerns labels Apr 28, 2024
@gracicot gracicot self-assigned this Apr 28, 2024
@gracicot gracicot added this to the 4.4 milestone Apr 28, 2024
@gracicot
Copy link
Owner

I can confirm this only happens with autowire services. This is due to a two step initialization. Before autowire, all dependencies were created before insertion so it didn't matter that we inserted the service in the instance vector before constructing the class. It was indeed a bug that cause UB if using particular service types so I won't consider this a breaking change.

@gracicot
Copy link
Owner

Fixed by e941e69. You can use master if you need the fix now, but I will release a version with the fix, probably 4.3.2. The order of destruction will be the one you expect if you use KANGARU_REVERSE_DESTRUCTION, which is the default if you use vcpkg or conan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug security/UB Undefined behavior and security concerns
Projects
None yet
Development

No branches or pull requests

2 participants