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

osgViewer::Viewer derefencing null pointer after being returned from a function. #1217

Closed
xlirate opened this issue Apr 10, 2023 · 5 comments

Comments

@xlirate
Copy link

xlirate commented Apr 10, 2023

The following code works

// some #includes

int main(int argc, char** argv)
{
    osg::ArgumentParser arguments{&argc, argv};
    osgViewer::Viewer viewer(arguments);
    // code continues on
}

but this code fails an address sanitizer check by dereferencing the pointer 0x000000000020

// some #includes

[[nodiscard]] osgViewer::Viewer createOsgViewer(osg::ArgumentParser& arguments)
{
    osgViewer::Viewer viewer(arguments);
    return viewer;
}

int main(int argc, char** argv)
{
    osg::ArgumentParser arguments{&argc, argv};
    osgViewer::Viewer viewer(createOsgViewer(arguments));
    // code continues on
}

I am using osg_3.6.5_x64-windows and osgearth_3.3_x64-windows both from vcpkg,
MSVC 19.35.32216.1
-std=C++17 and -std=C++20 both have this issue

I think that something is up with a copy constructor/move constructor with side effects being elided by the compiler as it is required to do.

Am I making an obvious error, or is there something to this?

@robertosfield
Copy link
Collaborator

It's best not to try to copy a Viewer, it has lots of resources associated with it and is not intended to be copyied. If you want to create a Viewer then create it on the heap and return a ref_ptr<> to it. i,e

ref_ptr createViewer(..)
{
ref_ptr viewer = new Viewer;
return viewer;
}

auto viewer = createViewer(..)

@xlirate
Copy link
Author

xlirate commented Apr 10, 2023

The catch here is that the code that I posted above does not actually copy the Viewer at all in C++17 and above. The Viewer is constructed in the calling scope at the location that is would have been moved/copied to if a normal move/copy constructor was called.

My concern is less about the resource usage and more about how there is an object that might have value semantics but instead commits memory access violations. I think that the fact that the above code compiles and does not work is a bug. Don't you agree?

@robertosfield
Copy link
Collaborator

The OSG is not written for modern C++ usage, it's written to be compatible with older compilers.

Mostly the OSG forces creation of scene graph objecst on the heap by having a protected destructor, but it some cases objects that are best managed on the heap are allowed to be constructed on the stack simply for convenience on the understanding that users won't abuse the class and misuse it in ways that it wasn't designed for.

The usage case you are wanting to use is misuse of the class, it was never written to be copied like you are attempting, so for C++17 or later not to be happy with the misuse is good in my book. Stop misusing the OSG and follow my suggestion.

@xlirate
Copy link
Author

xlirate commented Apr 10, 2023

If you want to have those sorts of semantics, that can be done with factory methods and protected/private constructors.

If you have a type that can be given automatic lifetime that does not play by the automatic lifetimes rules, it is an invalid type in C++, and has been since the Cfront days. Move and Copy elision has been a thing forever, It only became mandatory in 17. To borrow a phrase, "Stop misusing the C++ type system and consider what you are doing".

At the very least please consider doing something to make the compiler tell other well meaning developers off if you get the chance.

@robertosfield
Copy link
Collaborator

The OSG is in maintenance mode, it's being maintained for legacy applications using older C++ compilers. As I've said you're abusing the intended usage of osgVIewer::Viewer, if telling you is not enough then I'm sorry.

If you want a modern scene graph that properly supports C++17 then please stop looking at the OSG, please look at VulkanSceneGraph, or other modern projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants