Permalink
Browse files

fix a danling pointer bug in CInfoConsole

-> it takes a reference on a std::string and under some conditions it may delete that string, while the reference is still used
  • Loading branch information...
jK
jK committed Jul 6, 2014
1 parent 122bfa2 commit 5f9d050fd8480401bc89f37dbb91db556910ad67
Showing with 52 additions and 61 deletions.
  1. +1 −1 rts/Game/Game.cpp
  2. +46 −52 rts/Game/UI/InfoConsole.cpp
  3. +5 −8 rts/Game/UI/InfoConsole.h
View
@@ -1155,6 +1155,7 @@ bool CGame::UpdateUnsynced(const spring_time currentTime)
assert(infoConsole);
infoConsole->PushNewLinesToEventHandler();
+ infoConsole->Update();
configHandler->Update();
mouse->Update();
@@ -1526,7 +1527,6 @@ void CGame::SimFrame() {
if (!skipping) {
// everything here is unsynced and should ideally moved to Game::Update()
- infoConsole->Update();
waitCommandsAI.Update();
geometricObjects->Update();
sound->NewFrame();
@@ -2,7 +2,6 @@
#include "Rendering/GL/myGL.h"
-
#include "InfoConsole.h"
#include "InputReceiver.h"
#include "GuiHandler.h"
@@ -11,11 +10,9 @@
#include "System/Config/ConfigHandler.h"
#include "System/Log/LogSinkHandler.h"
-#include <fstream>
-
#define border 7
-CONFIG(int, InfoMessageTime).defaultValue(400);
+CONFIG(int, InfoMessageTime).defaultValue(10).description("Timeout till old messages disappear from the ingame console.");
CONFIG(std::string, InfoConsoleGeometry).defaultValue("0.26 0.96 0.41 0.205");
//////////////////////////////////////////////////////////////////////
@@ -31,11 +28,9 @@ CInfoConsole::CInfoConsole()
, lastMsgIter(lastMsgPositions.begin())
, newLines(0)
, rawId(0)
- , lastTime(0)
, fontScale(1.0f)
+ , maxLines(1)
{
- data.clear();
-
lifetime = configHandler->GetInt("InfoMessageTime");
const std::string geo = configHandler->GetString("InfoConsoleGeometry");
@@ -48,6 +43,9 @@ CInfoConsole::CInfoConsole()
height = 0.205f;
}
+ if (width == 0.f || height == 0.f)
+ enabled = false;
+
fontSize = fontScale * smallFont->GetSize();
logSinkHandler.AddSink(this);
@@ -66,7 +64,7 @@ void CInfoConsole::Draw()
if (!smallFont) return;
if (data.empty()) return;
- boost::recursive_mutex::scoped_lock scoped_lock(infoConsoleMutex); // XXX is this really needed?
+ boost::recursive_mutex::scoped_lock scoped_lock(infoConsoleMutex);
if (guihandler && !guihandler->GetOutlineFonts()) {
// draw a black background when not using outlined font
@@ -94,10 +92,9 @@ void CInfoConsole::Draw()
if (guihandler && guihandler->GetOutlineFonts())
fontOptions |= FONT_OUTLINE;
- std::deque<InfoLine>::iterator ili;
- for (ili = data.begin(); ili != data.end(); ++ili) {
+ for (int i = 0; i < std::min(data.size(), maxLines); ++i) {
curY -= fontHeight;
- smallFont->glPrint(curX, curY, fontSize, fontOptions, ili->text);
+ smallFont->glPrint(curX, curY, fontSize, fontOptions, data[i].text);
}
smallFont->End();
@@ -107,14 +104,24 @@ void CInfoConsole::Draw()
void CInfoConsole::Update()
{
boost::recursive_mutex::scoped_lock scoped_lock(infoConsoleMutex);
- if (lastTime > 0) {
- lastTime--;
+ if (!data.empty())
+ return;
+
+ // pop old messages after timeout
+ if (data[0].timeout <= spring_gettime()) {
+ data.pop_front();
}
- if (!data.empty()) {
- data.begin()->time--;
- if (data[0].time <= 0) {
- data.pop_front();
- }
+
+ if (!smallFont) return;
+
+ // if we have more lines then we can show, remove the oldest one,
+ // and make sure the others are shown long enough
+ const float maxHeight = (height * globalRendering->viewSizeY) - (border * 2);
+ maxLines = (smallFont->GetLineHeight() > 0)
+ ? math::floor(maxHeight / (fontSize * smallFont->GetLineHeight()))
+ : 1; // this will likely be the case on HEADLESS only
+ for (size_t i = data.size(); i > maxLines; i--) {
+ data.pop_front();
}
}
@@ -146,13 +153,10 @@ void CInfoConsole::PushNewLinesToEventHandler()
int CInfoConsole::GetRawLines(std::deque<RawLine>& lines)
{
- int tmp;
- {
- boost::recursive_mutex::scoped_lock scoped_lock(infoConsoleMutex);
- lines = rawData;
- tmp = newLines;
- newLines = 0;
- }
+ boost::recursive_mutex::scoped_lock scoped_lock(infoConsoleMutex);
+ lines = rawData;
+ int tmp = newLines;
+ newLines = 0;
return tmp;
}
@@ -162,55 +166,45 @@ void CInfoConsole::RecordLogMessage(const std::string& section, int level,
{
boost::recursive_mutex::scoped_lock scoped_lock(infoConsoleMutex);
- RawLine rl(text, section, level, rawId);
- rawId++;
- rawData.push_back(rl);
if (rawData.size() > maxRawLines) {
rawData.pop_front();
- }
- if (newLines < maxRawLines) {
+ } else {
newLines++;
}
+ rawData.emplace_back(text, section, level, rawId++);
if (!smallFont) {
return;
}
- const float maxWidth = (width * globalRendering->viewSizeX) - (border * 2);
- const float maxHeight = (height * globalRendering->viewSizeY) - (border * 2);
- const unsigned int maxLines = (smallFont->GetLineHeight() > 0)
- ? math::floor(maxHeight / (fontSize * smallFont->GetLineHeight()))
- : 1; // this will likely be the case on HEADLESS only
-
+ // !!! Warning !!!
+ // We must not remove elements from `data` here
+ // cause ::Draw() iterats that container, and it's
+ // possible that it calls LOG(), which will end
+ // in here. So if we would remove stuff here it's
+ // possible that we delete a var that is used in
+ // Draw() & co, and so we might invalidate references
+ // (e.g. of std::strings) and cause SIGFAULTs!
+ const float maxWidth = (width * globalRendering->viewSizeX) - (2 * border);
std::string wrappedText = smallFont->Wrap(text, fontSize, maxWidth);
std::list<std::string> lines = smallFont->SplitIntoLines(toustring(wrappedText));
for (auto& line: lines) {
// add the line to the console
- InfoLine l;
- data.push_back(l);
- data.back().text = line;
- data.back().time = lifetime - lastTime;
- lastTime = lifetime;
- }
-
- // if we have more lines then we can show, remove the oldest one,
- // and make sure the others are shown long enough
- for (size_t i = data.size(); i > maxLines; i--) {
- data[1].time += data[0].time;
- data.pop_front();
+ data.emplace_back();
+ InfoLine& l = data.back();
+ l.text = std::move(line);
+ l.timeout = spring_gettime() + spring_secs(lifetime);
}
}
void CInfoConsole::LastMessagePosition(const float3& pos)
{
- if (lastMsgPositions.size() < maxLastMsgPos) {
- lastMsgPositions.push_front(pos);
- } else {
- lastMsgPositions.push_front(pos);
+ if (lastMsgPositions.size() >= maxLastMsgPos) {
lastMsgPositions.pop_back();
}
+ lastMsgPositions.push_front(pos);
// reset the iterator when a new msg comes in
lastMsgIter = lastMsgPositions.begin();
View
@@ -7,9 +7,9 @@
#include "System/float3.h"
#include "System/EventClient.h"
#include "System/Log/LogSinkHandler.h"
+#include "System/Misc/SpringTime.h"
#include <deque>
-#include <vector>
#include <string>
#include <list>
#include <boost/thread/recursive_mutex.hpp>
@@ -27,15 +27,13 @@ class CInfoConsole: public CInputReceiver, public CEventClient, public ILogSink
void RecordLogMessage(const std::string& section, int level,
const std::string& text);
-
bool WantsEvent(const std::string& eventName) {
return (eventName == "LastMessagePosition");
}
void LastMessagePosition(const float3& pos);
const float3& GetMsgPos(const float3& defaultPos = ZeroVector);
unsigned int GetMsgPosCount() const { return lastMsgPositions.size(); }
-
bool enabled;
public:
@@ -49,21 +47,19 @@ class CInfoConsole: public CInputReceiver, public CEventClient, public ILogSink
, section(section)
, level(level)
, id(id)
- , time(0)
{}
std::string text;
std::string section;
int level;
int id;
- boost::uint32_t time;
};
- int GetRawLines(std::deque<RawLine>& copy);
+ int GetRawLines(std::deque<RawLine>& copy);
private:
struct InfoLine {
std::string text;
- int time;
+ spring_time timeout;
};
std::list<float3> lastMsgPositions;
@@ -74,7 +70,6 @@ class CInfoConsole: public CInputReceiver, public CEventClient, public ILogSink
size_t newLines;
int rawId;
- int lastTime;
mutable boost::recursive_mutex infoConsoleMutex;
@@ -85,6 +80,8 @@ class CInfoConsole: public CInputReceiver, public CEventClient, public ILogSink
float height;
float fontScale;
float fontSize;
+
+ size_t maxLines;
};
#endif /* INFO_CONSOLE_H */

0 comments on commit 5f9d050

Please sign in to comment.