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

GL: fix segfault in Context destructor if constructed with NoCreate #331

Closed
wants to merge 2 commits into from

Conversation

Projects
3 participants
@xqms
Copy link
Contributor

commented Apr 4, 2019

If we use the GL::Context(NoCreate, ...) constructor and never actually create it, the destructor will segfault, since it unconditionally deletes _state.

In my code, it happens like this:

Platform::GLContext context{NoCreate, argc, argv.data()};
...
if(somethingFailed)
   return; // segfault here

context.tryCreate();
@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

If you would like a unit test for this, let me know. I have no idea where to put it though - GL or Platform?

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

... and maybe a cleaner solution would be Corrade::Pointer instead of a raw pointer?

@mosra

This comment has been minimized.

Copy link
Owner

commented Apr 4, 2019

Thanks! 👍 💯

Yes, Corrade::Pointer would be very nice to have here -- originally I hesitated using std::unique_ptr because of the compile-time overhead, but since there's a lightweight type now, it makes sense to use it.

The unit test for this could go into GL/Test/ContextTest.cpp (not in ContextGLTest) -- simply construct a NoCreate instance of Platform::GLContext and then let it go out of scope, similarly how's that done here for Buffer. Actually I'm not sure why this test wasn't there already, hopefully there's no unsolvable design issue preventing this from being tested :)

@mosra mosra added this to TODO in GL via automation Apr 4, 2019

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Thanks for the suggestions, will update the PR 👍

By the way, your response time is awesome ;-)

GL: Use Corrade::Containers::Pointer for Context _state
The old raw pointer could cause a segfault if the context is never
created, since the destructor unconditionally deleted _state.

@xqms xqms force-pushed the xqms:gl_context_state branch from 39e4f5e to 4d44781 Apr 4, 2019

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

I'm seeing a linking problem now in the unit test, it seems MagnumGL does not link to flextGL and I would need to link to some Platform library?

contrib/magnum/src/Magnum/GL/Test/CMakeFiles/GLContextTest.dir/ContextTest.cpp.o: In function `Magnum::Platform::GLContext::GLContext(Magnum::NoCreateT, int, char const**)':
contrib/magnum/src/Magnum/Platform/GLContext.h:100: undefined reference to `flextGLInit'

The reason that I want to test with Platform::GLContext rather than GL::Context is that the NoCreate constructor is private - maybe it would make sense to have a local friended class in the test file and avoid the Platform::GLContext that way?

@mosra

This comment has been minimized.

Copy link
Owner

commented Apr 4, 2019

Yeah I know, it's private for a reason :) Can you try linking it to the MagnumOpenGLTester library (like the *GLTest tests)? So it doesn't attempt to create a context but still picks up all needed libraries. That should do the trick.

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Can you try linking it to the MagnumOpenGLTester library (like the *GLTest tests)?

That works, but needs WITH_OPENGLTESTER. Should we simply add the test function to ContextGLTest.cpp?

@xqms xqms force-pushed the xqms:gl_context_state branch from 4d44781 to 43397b8 Apr 4, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Apr 4, 2019

Codecov Report

Merging #331 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   71.66%   71.67%   +<.01%     
==========================================
  Files         346      346              
  Lines       17564    17562       -2     
==========================================
- Hits        12588    12587       -1     
+ Misses       4976     4975       -1
Impacted Files Coverage Δ
src/Magnum/GL/Context.h 91.3% <ø> (ø) ⬆️
src/Magnum/GL/Context.cpp 68% <50%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff94725...43397b8. Read the comment docs.

@mosra

This comment has been minimized.

Copy link
Owner

commented Apr 4, 2019

Uh oh. That might be the "unsolvable design issue preventing this from being tested" I was talking about :D I'll try to figure out a solution. How fast do you need this to be merged?

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

No rush, this only happens in my error path and leads to a crash instead of an exit with error code. It doesn't impact my work at all.

In any case, I pushed the last version with the test in ContextGLTest.cpp, that at least compiles and runs. I understand that it would be better to have the test in the non-GL testsuite, though.

@mosra mosra added this to the 2019.0b milestone Apr 5, 2019

@mosra

This comment has been minimized.

Copy link
Owner

commented Apr 5, 2019

Merged in c38319e and f58c26c, thank you! In the end I made the relevant methods of GL::Context protected instead of private, which allowed me to both have it testable in ContextTest.cpp and removed the need for friending Platform::GLContext.

@mosra mosra closed this Apr 5, 2019

GL automation moved this from TODO to Done Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.