Using dependency injection and vsg::Inherited memory gets corrupted

[sbrkopac]

Describe the bug When using dependency injection and vsg::Inherited memory gets corrupted

To Reproduce

#include <iostream>
#include <unordered_map>
#include <vsg/core/Inherit.h>

class Foo
{
public:

    Foo() = default;
    ~Foo() = default;

    void addValue(std::string key, int value)
    {
        intMap[key] = value;
    }

    void dump()
    {
        std::cout << "\nintMap contains the following key and value pairs: \n";
        for (const auto& entry : intMap)
        {
            std::cout << entry.first << ", " << entry.second << "\n";
        }
        std::cout << std::endl;
    }

private:

    std::unordered_map<std::string, int> intMap;
};

class Bar : public vsg::Inherit<vsg::Object, Bar>
{
public:

    Bar(Foo& foo) : foo(foo) { }

    void dump()
    {
        foo.dump();
    }

private:

    Foo& foo;
};

int main(int argc, char* argv[])
{
    // setup test
    Foo foo;
    foo.addValue("firstKey", 1);
    foo.dump();

    // use a naked pointer
    Bar* bar = new Bar(foo);
    bar->dump();
    delete bar;

    // try to use a vsg object
    bar = Bar::create(foo);
    bar->dump();
}

Expected behavior I would expect my call to bar->dump() to print the same output as the prior two dump calls but I get a crash instead.

Desktop:

• OS: Windows 10
• Version: master

[vsg-dev]

I am surprised you get a crash but I find the code pretty hard to read, not because it's complicated but because it's such a bad example of how to write C++.

The VSG has smart pointers, and Inherit provides a create() method that helps create objects and return ref_ptr<> for them to help enforce use of ref_ptr<> - this is what the vast majority of VSG examples will use.

Second up passing a reference to an object on the stack (Foo) to an object on the heap Bar that can outlive the scope of Foo but still have a reference is really bad practice.

Third calling delete explicitly on ref counted object designed to be used and shared using smart pointers is really prone to errors and is another bad practice. For your own classes I would recommend following the idiom of putting the destructor into private or protected scope to prevent users from making this mistake.

So my body recoils from such problematic C++ code. However, in principle I can't see why it wouldn't work and can't see why use of Inherit would have any baring on you getting a crash. I will build the example here and see what my Linux system makes of it.

[vsg-dev]

I have just tested under Linux and it runs fine and doesn't crash but valgrind reports a memory access error. I've just spotted the cause of the crash:

bar = Bar::create(foo); 

The Bar::create(foo) creates a Bar object and returns a ref_ptr<> but you don't assign it to C pointer so bar goes out of scope right after the pointer assignment and deletes the object.

So it's the automatic conversion of ref_ptr<> to C pointer for convenience is what is causing problems here, it's not related to Inherit. I even commented on this when I wrote vsg::ref_ptr<>:

    // potentially dangerous automatic type conversion, could cause dangling pointer if ref_ptr<> assigned to C pointer, if ref_ptr<> destruction cause an object delete.
    operator T*() const noexcept { return _ptr; }

It's really convenient though, as adding a .get() everywhere just clogs code up and makes it less expressive. We could remove the method above and prevent problem usage like the above and loose this convenience, the VSG itself right now doesn't build with this ref_ptr<> method removed so it would need a range of updates.

The question is how common will users accidentally go and write bad code?

[sbrkopac]

I am surprised you get a crash but I find the code pretty hard to read, not because it's complicated but because it's such a bad example of how to write C++.

I don't disagree. This is obviously a super stripped down minimal example.

Second up passing a reference to an object on the stack (Foo) to an object on the heap Bar that can outlive the scope of Foo but still have a reference is really bad practice.

I don't disagree with this either.

Third calling delete explicitly on ref counted object designed to be used and shared using smart pointers is really prone to errors and is another bad practice. For your own classes I would recommend following the idiom of putting the destructor into private or protected scope to prevent users from making this mistake.

Agreed. In the non-minimal stripped example the destructor is private and that is never called. I added it to the example for completeness.

So my body recoils from such problematic C++ code.

Would you prefer more complex examples that had less recoil factor? The goal was to illustrate an issue and not have to you sift through a large example.

So it's the automatic conversion of ref_ptr<> to C pointer for convenience is what is causing problems here, it's not related to Inherit. I even commented on this when I wrote vsg::ref_ptr<>:

Does that mean something like the below should work?

int main(int argc, char* argv[])
{
    // setup test
    Foo foo;
    foo.addValue("firstKey", 1);
    foo.dump();

    // try to use a vsg object
    vsg::ref_ptr<Bar> bar = Bar::create(foo);
    bar->dump();
}

This still crashes out for me.

It's really convenient though, as adding a .get() everywhere just clogs code up and makes it less expressive. We could remove the method above and prevent problem usage like the above and loose this convenience, the VSG itself right now doesn't build with this ref_ptr<> method removed so it would need a range of updates.

The question is how common will users accidentally go and write bad code?

I'm sure if either of us had the answer to that we'd be on an island somewhere :smiley:

[vsg-dev]

I have just tried your more simplified version that use ref_ptr<> and it works fine for me, valgrind can't find any issues either. One should pass objects via reference like this unless in really controlled manner so it's still dangerous code, but should work fine. Below is the valigrind output.

==7223== Memcheck, a memory error detector ==7223== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==7223== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info ==7223== Command: vsgtypes ==7223==

intMap contains the following key and value pairs: firstKey, 1

intMap contains the following key and value pairs:

==7223== ==7223== HEAP SUMMARY: ==7223== in use at exit: 0 bytes in 0 blocks ==7223== total heap usage: 7 allocs, 7 frees, 73,920 bytes allocated ==7223== ==7223== All heap blocks were freed -- no leaks are possible ==7223== ==7223== For counts of detected and suppressed errors, rerun with: -v ==7223== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[vsg-dev]

To protect against users writing bad code one thing can do is put all the constructors for scene graph objects into protected scope to force users to use vsg::T::create() and remove the ref_ptr<> to C pointer automatic cast.

However, it would require restricting users a bit and quite a few mods to the VSG. I did consider these topics when I wrote ref_ptr<> and Inherit and decided then to give users the benefit of doubt and give them flexibility. Might have to revisit this. I am now keen on making the code less readable and more verbose, which is one of the reasons I decided to go the more flexible approach.

For your persisting crash, you'll need to dig into the compiler, it could be compile bug, or some build issue at your end.

[sbrkopac]

To protect against users writing bad code one thing can do is put all the constructors for scene graph objects into protected scope to force users to use vsg::T::create() and remove the ref_ptr<> to C pointer automatic cast.

Perhaps the 24 hour rule would apply here? Let it sit for a bit before making a decision like that.

For your persisting crash, you'll need to dig into the compiler, it could be compile bug, or some build issue at your end.

The output you provided you isn't what I'd be expecting from the program. It fails to iterate the intMap in the second dump() call.

On windows this crashes with the following Exception thrown: read access violation. std::basic_string<char,std::char_traits<char>,std::allocator<char> >::_Get_data(...) returned 0xFFFFFFFFFFFFFFE7.. My debugger is telling me that the size of the map is 1 but the data inside the map is corrupt.

I ran this through a linux VM and I don't get the crash with the minimal exampl but. I don't see the output of the intMap though in my linux VM.

@tomhog Tom, could you please run this minimal example and let me know if you get a crash since you are running Windows as well. My tool chain is Visual Studio 2019 16.2.2.

Thank you for taking the time to look at this.

[vsg-dev]

Try putting in a Foo constructor and destructor to report the this pointer, and report the this pointer in the dump to see what it's doing.

You are right about the console output I'm getting, we should get the same value for each call to foo.dump. For some reason this isn't happening. Perhaps there is some odd rule about how compilers are handling references.

Robert.

[vsg-dev]

I think something odd is happening with the forwarding of the reference within Inherit::create(), I've just added then following debug output to track whether we have multiple Foos:

class Foo { public:

Foo()
{
    std::cout<<"Foo::Foo() "<<this<<std::endl;
}

~Foo()
{
    std::cout<<"Foo::~Foo() "<<this<<std::endl;
}

void addValue(std::string key, int value)
{
    intMap[key] = value;
}

void dump()
{
    std::cout << "\nintMap contains the following key and value pairs: "<<this<<" "<<intMap.size()<<" \n";
    for (const auto& entry : intMap)
    {
        std::cout << entry.first << ", " << entry.second << "\n";
    }
    std::cout << std::endl;
}

private:

std::unordered_map<std::string, int> intMap;

};

And the resulting output is: Foo::Foo() 0x7ffdde5eb080

intMap contains the following key and value pairs: 0x7ffdde5eb080 1 firstKey, 1

Foo::~Foo() 0x7ffdde5eb0c0

intMap contains the following key and value pairs: 0x7ffdde5eb0c0 0

Foo::~Foo() 0x7ffdde5eb080

Which tells me that there is extra copies of Foo being made, but not via the default Foo constructor. My best guess is that the Foo reference isn't being passed by the Inherit::create template method as intended.

[vsg-dev]

Chaging from Bar::create(foo) to new Bar(foo) fixes the problem with multiple copies:

int main(int argc, char* argv[]) { // setup test Foo foo; foo.addValue("firstKey", 1); foo.dump();

// try to use a vsg object
vsg::ref_ptr<Bar> bar(new Bar(foo));
bar->dump();

}

Produces the result (that looks correct to me):

Foo::Foo() 0x7fff779a36c0

intMap contains the following key and value pairs: 0x7fff779a36c0 1 firstKey, 1

intMap contains the following key and value pairs: 0x7fff779a36c0 1 firstKey, 1

Foo::~Foo() 0x7fff779a36c0

Which all points to the Inhert::create() not working quite as intended when passing parameters.

[vsg-dev]

Explicitly deleting the Foo(const Foo&) constructor via:

Foo(const Foo&) = delete;

Creates the compile error:

/home/robert/Dev/vsgExamples/Core/vsgtypes/vsgtypes.cpp: In function ‘int main(int, char**)’: /home/robert/Dev/vsgExamples/Core/vsgtypes/vsgtypes.cpp:80:44: error: use of deleted function ‘Foo::Foo(const Foo&)’ vsg::ref_ptr bar = Bar::create(foo); ^ /home/robert/Dev/vsgExamples/Core/vsgtypes/vsgtypes.cpp:19:5: note: declared here Foo(const Foo&) = delete; ^~~ In file included from /home/robert/Dev/vsgExamples/Core/vsgtypes/vsgtypes.cpp:3:0: /home/robert/Install/include/vsg/core/Inherit.h:65:34: note: initializing argument 1 of ‘static vsg::ref_ptr vsg::Inherit<ParentClass, Subclass>::create(Args ...) [with Args = {Foo}; ParentClass = vsg::Object; Subclass = Bar]’ static ref_ptr create(Args... args) ^~

So this confirms that problem lies with the use of template Args expansion automatically creating a copy and then a reference to it.

[vsg-dev]

One solution for the template args automatically creating a copy is to pass the foo by std::ref(foo), the follow works for me with the correct output:

int main(int argc, char* argv[]) { // setup test Foo foo; foo.addValue("firstKey", 1); foo.dump();

// try to use a vsg object
vsg::ref_ptr<Bar> bar = Bar::create(std::ref(foo));
bar->dump();

}

Note the std::ref(foo) in the Bar::create(..) line.

I will now look to see if there a tweak to the Args usage in Inherit::create(..) to see if there is a solution on that front.

Robert.

[vsg-dev]

I've changed the template argument passing in Inherit::create(..) to use Args&&.... rather than Args... and this enables the code to work correctly without the std::ref(), this is now checked into master as commit:

https://github.com/vsg-dev/VulkanSceneGraph/commit/e1aad3a61db63b0487ddceff7cd3118ecef2887f

Could you please update to master and test then let me know how it's working under Windows.

[sbrkopac]

Could you please update to master and test then let me know how it's working under Windows.

No more crash and I get the expected output. Thanks for the fix!

Robert, I'll leave the issue up to you to close, in case you want to have any more discussion regarding the automatic type conversion or anything else in this thread.