Closed abrauchli closed 6 years ago
@abrauchli Hello and thanks for the bug report.
It looks like an exception is thrown during the map loading, and subsequently RoR segfaults during stack unwinding because the code isn't exception-safe.
I'll create a development branch with more thorough exception checks and ask you to test it.
@only-a-ptr Awesome, thanks. I'd be happy to test.
Btw, here's my take at it:
From 5427903cae00f40775201e20096af8d53e66e7ca Mon Sep 17 00:00:00 2001
From: Andreas Brauchli <andreas.brauchli@sensirion.com>
Date: Fri, 31 Aug 2018 11:01:08 +0200
Subject: [PATCH] Avoid crash in GfxEnvmap Destructor
When the GfxEnvmap is destroyed before initialization with SetupEnvMap()
the destructor crashes trying to free resources that aren't allocated.
---
source/main/gfx/EnvironmentMap.cpp | 15 ++++++++++-----
source/main/gfx/EnvironmentMap.h | 1 +
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/source/main/gfx/EnvironmentMap.cpp b/source/main/gfx/EnvironmentMap.cpp
index 27bc92aa..df2b139e 100644
--- a/source/main/gfx/EnvironmentMap.cpp
+++ b/source/main/gfx/EnvironmentMap.cpp
@@ -91,6 +91,8 @@ void RoR::GfxEnvmap::SetupEnvMap()
}
}
+ m_is_set_up = true;
+
if (App::diag_envmap.GetActive())
{
// create fancy mesh for debugging the envmap
@@ -218,12 +220,15 @@ void RoR::GfxEnvmap::SetupEnvMap()
RoR::GfxEnvmap::~GfxEnvmap()
{
- for (int face = 0; face < NUM_FACES; face++)
- {
- gEnv->sceneManager->destroyCamera("EnvironmentCamera-" + TOSTRING(face));
- }
+ if (m_is_set_up) {
+ for (int face = 0; face < NUM_FACES; face++)
+ {
+ gEnv->sceneManager->destroyCamera("EnvironmentCamera-" +
+ TOSTRING(face));
+ }
- Ogre::TextureManager::getSingleton().remove(m_rtt_texture->getName());
+ Ogre::TextureManager::getSingleton().remove(m_rtt_texture->getName());
+ }
}
void RoR::GfxEnvmap::UpdateEnvMap(Ogre::Vector3 center, Actor* beam /* = 0 */)
diff --git a/source/main/gfx/EnvironmentMap.h b/source/main/gfx/EnvironmentMap.h
index 0356177b..69cb06ee 100644
--- a/source/main/gfx/EnvironmentMap.h
+++ b/source/main/gfx/EnvironmentMap.h
@@ -48,6 +48,7 @@ private:
Ogre::RenderTarget* m_render_targets[NUM_FACES];
Ogre::TexturePtr m_rtt_texture;
bool m_is_initialized;
+ bool m_is_set_up;
int m_update_round; /// Render targets are updated one-by-one; this is the index of next target to update.
bool m_is_enabled; // TODO: Use a GVar!! ~ only_a_ptr, 08/2017
};
--
2.17.1
You beat me to it 😄 However, state flags are always ugly and there is one already. Do this instead (pseudocode, I'm in a hurry):
/*in destructor: */
for each camera
{
String cam_name = /*name*/
if (gEnv->sceneManager->getCameras().find(cam_name) != /*invalid iterator*/) { /*cleanup*/ }
}
if (!m_rtt_texture.isNull()) { /*cleanup*/ }
Thanks for the pseudo-code @only-a-ptr - turns out getCameras return type is protected so I went with getCamera
and handle the exception:
From 19d2a1249a4400e2e37f6c8d15702c41de85a934 Mon Sep 17 00:00:00 2001
From: Andreas Brauchli <andreas.brauchli@sensirion.com>
Date: Fri, 31 Aug 2018 11:01:08 +0200
Subject: [PATCH] Avoid crash in GfxEnvmap Destructor
When the GfxEnvmap is destroyed before initialization with
SetupEnvMap() the destructor crashes trying to free resources that
aren't allocated.
---
source/main/gfx/EnvironmentMap.cpp | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/source/main/gfx/EnvironmentMap.cpp b/source/main/gfx/EnvironmentMap.cpp
index 27bc92aa..7364e8ae 100644
--- a/source/main/gfx/EnvironmentMap.cpp
+++ b/source/main/gfx/EnvironmentMap.cpp
@@ -220,10 +220,20 @@ RoR::GfxEnvmap::~GfxEnvmap()
{
for (int face = 0; face < NUM_FACES; face++)
{
- gEnv->sceneManager->destroyCamera("EnvironmentCamera-" + TOSTRING(face));
+ try
+ {
+ Ogre::Camera *cam = gEnv->sceneManager->getCamera("EnvironmentCamera-" + TOSTRING(face));
+ gEnv->sceneManager->destroyCamera(cam);
+ }
+ catch(...)
+ {
+ }
}
- Ogre::TextureManager::getSingleton().remove(m_rtt_texture->getName());
+ if (!m_rtt_texture.isNull())
+ {
+ Ogre::TextureManager::getSingleton().remove(m_rtt_texture->getName());
+ }
}
void RoR::GfxEnvmap::UpdateEnvMap(Ogre::Vector3 center, Actor* beam /* = 0 */)
--
2.17.1
Thanks, but catch(...){/*and do nothing*/}
is unaceptable - if it cannot be avoided at all, at least comment why it's inevitable. In this case, the try/catch
it's not necessary.
turns out getCameras return type is protected
Sure, it's const
, you're supposed to do it like this:
EDIT where did I put my eyes? We have m_cameras
in there!
for (int face = 0; face < NUM_FACES; face++)
{
if (m_cameras[face] != nullptr)
{
gEnv->sceneManager->destroyCamera(m_cameras[face]);
}
}
I submitted the fix myself, but thanks for your report and research, I look forward to working with you more!
Hi @only-a-ptr, thanks for the cpp coaching and for your work on RoR. I didn't remember there was auto
- I'm more into embedded C, C++ is too complex ;)
@abrauchli Did you test with my patch? Because it only resolves the segfault, not the original exception which caused it. It seems to be something specific to your hardware/drivers.
@only-a-ptr yes, confirmed working - many thanks. I'll leave it open for when it's merged
UI pops up, selecting any map results in a segfault when loading the resources, full stacktrace below
Built with unmodified build scripts from https://github.com/RigsOfRods/ror-linux-buildscripts Linux (Ubuntu 18.04)
Commenting out the line resolves the problem (but obviously doesn't clean up the resources)