Skip to content

Commit

Permalink
Fix race condition when writing to console from UPnP thread.
Browse files Browse the repository at this point in the history 
The console and text drawing code are _not_ thread-safe, and should
never be called from side threads!

Also fixes a less serious, non-thread related race condition that would
mean text got truncated too early because the global font was sometimes
wrong for counting pixels while truncating into console buffer.

Closes ticket:4567
Closes ticket:4564
Closes ticket:4546
Actually fixes ticket:4361 and reverts useless fix in that ticket.
  • Loading branch information
@perim
perim committed Mar 26, 2017
1 parent 839e0b0 commit 03d494d
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 96 deletions.
23 changes: 10 additions & 13 deletions lib/ivis_opengl/textdraw.cpp
View file
Expand Up @@ -124,11 +124,7 @@ struct FTFace
codePoint, // the glyph_index in the font file
FT_LOAD_NO_HINTING // by default hb load fonts without hinting
);

if (error != FT_Err_Ok)
{
debug(LOG_FATAL, "unable to load glyph");
}
ASSERT(error == FT_Err_Ok, "Unable to load glyph for %u", codePoint);
return m_face->glyph->metrics.width;
}

Expand All @@ -138,10 +134,7 @@ struct FTFace
codePoint, // the glyph_index in the font file
FT_LOAD_NO_HINTING // by default hb load fonts without hinting
);
if (error != FT_Err_Ok)
{
debug(LOG_FATAL, "unable to load glyph");
}
ASSERT(error == FT_Err_Ok, "Unable to load glyph %u", codePoint);

FT_GlyphSlot slot = m_face->glyph;
FT_Render_Glyph(m_face->glyph, FT_RENDER_MODE_LCD);
Expand All @@ -153,9 +146,9 @@ struct FTFace
{
memcpy(g.buffer.get(), ftBitmap.buffer, ftBitmap.pitch * ftBitmap.rows);
}
else if (ftBitmap.pitch != 0 && ftBitmap.rows != 0)
else
{
debug(LOG_FATAL, "glyph buffer missing"); // This probably doesn't happen.
ASSERT(ftBitmap.pitch == 0 || ftBitmap.rows == 0, "Glyph buffer missing (%d and %d)", ftBitmap.pitch, ftBitmap.rows);
}
g.width = ftBitmap.width / 3;
g.height = ftBitmap.rows;
Expand Down Expand Up @@ -464,11 +457,15 @@ void iV_SetFont(enum iV_fonts FontID)
s_FondID = FontID;
}

unsigned int iV_GetTextWidth(const char *string)
unsigned int iV_GetTextWidth(const char *string, iV_fonts FontID)
{
uint32_t width;
if (FontID == font_count) // support legacy API with global font selection for a while
{
FontID = s_FondID;
}
TextRun tr(string, "en", HB_SCRIPT_COMMON, HB_DIRECTION_LTR);
std::tie(width, std::ignore) = getShaper().getTextMetrics(tr, getFTFace(s_FondID));
std::tie(width, std::ignore) = getShaper().getTextMetrics(tr, getFTFace(FontID));
return width;
}

Expand Down
25 changes: 12 additions & 13 deletions lib/ivis_opengl/textdraw.h
View file
Expand Up @@ -34,17 +34,17 @@ enum iV_fonts
font_count
};

extern void iV_TextInit(void);
extern void iV_TextShutdown(void);
extern void iV_font(const char *fontName, const char *fontFace, const char *fontFaceBold);
void iV_TextInit();
void iV_TextShutdown();
void iV_font(const char *fontName, const char *fontFace, const char *fontFaceBold);

extern void iV_SetFont(enum iV_fonts FontID);
extern int iV_GetTextAboveBase(void);
extern int iV_GetTextBelowBase(void);
extern int iV_GetTextLineSize(void);
extern unsigned int iV_GetTextWidth(const char *String);
extern unsigned int iV_GetCountedTextWidth(const char *string, size_t string_length);
extern unsigned int iV_GetCharWidth(uint32_t charCode);
void iV_SetFont(enum iV_fonts FontID);
int iV_GetTextAboveBase();
int iV_GetTextBelowBase();
int iV_GetTextLineSize();
unsigned int iV_GetTextWidth(const char *String, iV_fonts = font_count);
unsigned int iV_GetCountedTextWidth(const char *string, size_t string_length);
unsigned int iV_GetCharWidth(uint32_t charCode);

extern unsigned int iV_GetTextHeight(const char *string);
extern void iV_SetTextColour(PIELIGHT colour);
Expand All @@ -57,9 +57,8 @@ enum
FTEXT_RIGHTJUSTIFY, // Right justify.
};

extern int iV_DrawFormattedText(const char *String, UDWORD x, UDWORD y, UDWORD Width, UDWORD Justify);

extern void iV_DrawTextRotated(const char *string, float x, float y, float rotation);
int iV_DrawFormattedText(const char *String, UDWORD x, UDWORD y, UDWORD Width, UDWORD Justify);
void iV_DrawTextRotated(const char *string, float x, float y, float rotation);

/** Draws text with a printf syntax to the screen.
*/
Expand Down
145 changes: 82 additions & 63 deletions lib/netplay/netplay.cpp
View file
Expand Up @@ -29,13 +29,16 @@
#include "lib/framework/crc.h"
#include "lib/framework/file.h"
#include "lib/gamelib/gtime.h"
#include "lib/exceptionhandler/dumpinfo.h"
#include "src/console.h"
#include "src/component.h" // FIXME: we need to handle this better
#include "src/modding.h" // FIXME: we need to handle this better

#include <time.h> // for stats
#include <physfs.h>
#include <string.h>
#include <memory>
#include <thread>

#include "netplay.h"
#include "netlog.h"
Expand All @@ -44,7 +47,6 @@
#include <miniupnpc/miniwget.h>
#include <miniupnpc/miniupnpc.h>
#include <miniupnpc/upnpcommands.h>
#include "lib/exceptionhandler/dumpinfo.h"

#include "src/multistat.h"
#include "src/multijoin.h"
Expand Down Expand Up @@ -78,6 +80,10 @@ static unsigned int masterserver_port = 0, gameserver_port = 0;
*/
#define NET_BUFFER_SIZE (MaxMsgSize) // Would be 16K

#define UPNP_SUCCESS 1
#define UPNP_ERROR_DEVICE_NOT_FOUND -1
#define UPNP_ERROR_CONTROL_NOT_AVAILABLE -2

// ////////////////////////////////////////////////////////////////////////
// Function prototypes
static void NETplayerLeaving(UDWORD player); // Cleanup sockets on player leaving (nicely)
Expand All @@ -95,6 +101,8 @@ SYNC_COUNTER sync_counter; // keeps track on how well we are in sync
// ////////////////////////////////////////////////////////////////////////
// Types

std::atomic_int upnp_status;

struct Statistic
{
unsigned sent;
Expand Down Expand Up @@ -931,86 +939,64 @@ static bool NETrecvGAMESTRUCT(GAMESTRUCT *ourgamestruct)
return true;
}


static int upnp_init(void *)
// This function is run in its own thread! Do not call any non-threadsafe functions!
static void upnp_init(std::atomic_int &retval)
{
struct UPNPDev *devlist;
struct UPNPDev *dev;
char *descXML;
int result = 0;
int descXMLsize = 0;
char buf[255];
int result;
memset(&urls, 0, sizeof(struct UPNPUrls));
memset(&data, 0, sizeof(struct IGDdatas));

if (NetPlay.isUPNP)
debug(LOG_NET, "Searching for UPnP devices for automatic port forwarding...");
devlist = upnpDiscover(3000, NULL, NULL, 0, 0, 2, &result);
debug(LOG_NET, "UPnP device search finished.");

if (devlist)
{
debug(LOG_NET, "Searching for UPnP devices for automatic port forwarding...");
devlist = upnpDiscover(3000, NULL, NULL, 0, 0, 2, &result);
debug(LOG_NET, "UPnP device search finished.");
if (devlist)
dev = devlist;
while (dev)
{
dev = devlist;
while (dev)
{
if (strstr(dev->st, "InternetGatewayDevice"))
{
break;
}
dev = dev->pNext;
}
if (!dev)
if (strstr(dev->st, "InternetGatewayDevice"))
{
dev = devlist; /* defaulting to first device */
break;
}
dev = dev->pNext;
}
if (!dev)
{
dev = devlist; /* defaulting to first device */
}

debug(LOG_NET, "UPnP device found: %s %s\n", dev->descURL, dev->st);
debug(LOG_NET, "UPnP device found: %s %s\n", dev->descURL, dev->st);

descXML = (char *)miniwget_getaddr(dev->descURL, &descXMLsize, lanaddr, sizeof(lanaddr), dev->scope_id);
debug(LOG_NET, "LAN address: %s", lanaddr);
if (descXML)
{
parserootdesc(descXML, descXMLsize, &data);
free(descXML); descXML = 0;
GetUPNPUrls(&urls, &data, dev->descURL, dev->scope_id);
}
ssprintf(buf, "UPnP device found: %s %s LAN address %s", dev->descURL, dev->st, lanaddr);
addDumpInfo(buf);
freeUPNPDevlist(devlist);
descXML = (char *)miniwget_getaddr(dev->descURL, &descXMLsize, lanaddr, sizeof(lanaddr), dev->scope_id);
debug(LOG_NET, "LAN address: %s", lanaddr);
if (descXML)
{
parserootdesc(descXML, descXMLsize, &data);
free(descXML); descXML = 0;
GetUPNPUrls(&urls, &data, dev->descURL, dev->scope_id);
}

if (!urls.controlURL || urls.controlURL[0] == '\0')
{
ssprintf(buf, "controlURL not available, UPnP disabled");
addDumpInfo(buf);
// beware of writing a line too long, it screws up console line count. \n is safe for line split
ssprintf(buf, _("Your router doesn't support UPnP, you must manually configure your router & firewall to\nopen port 2100 before you can host a game."));
addConsoleMessage(buf, DEFAULT_JUSTIFY, NOTIFY_MESSAGE);
NetPlay.isUPNP_CONFIGURED = false;
NetPlay.isUPNP_ERROR = true;
return false;
}
debug(LOG_NET, "UPnP device found: %s %s LAN address %s", dev->descURL, dev->st, lanaddr);
freeUPNPDevlist(devlist);

NETaddRedirects();
return true;
if (!urls.controlURL || urls.controlURL[0] == '\0')
{
retval = UPNP_ERROR_CONTROL_NOT_AVAILABLE;
}
else
{
retval = UPNP_SUCCESS;
}
ssprintf(buf, "UPnP device not found.");
addDumpInfo(buf);
debug(LOG_NET, "No UPnP devices found.");
// beware of writing a line too long, it screws up console line count. \n is safe for line split
ssprintf(buf, _("No UPnP device was found. You must manually configure your router & firewall to\nopen port 2100 before you can host a game."));
addConsoleMessage(buf, DEFAULT_JUSTIFY, NOTIFY_MESSAGE);
NetPlay.isUPNP_CONFIGURED = false;
NetPlay.isUPNP_ERROR = true;
return false;
}
else
{
ssprintf(buf, "UPnP detection routine disabled by user.");
addDumpInfo(buf);
debug(LOG_NET, "UPnP detection routine disabled by user.");
return false;
retval = UPNP_ERROR_DEVICE_NOT_FOUND;
}

}

static bool upnp_add_redirect(int port)
Expand Down Expand Up @@ -1088,10 +1074,14 @@ void NETremRedirects(void)

void NETdiscoverUPnPDevices(void)
{
if (!NetPlay.isUPNP_CONFIGURED)
if (!NetPlay.isUPNP_CONFIGURED && NetPlay.isUPNP)
{
upnpdiscover = wzThreadCreate(&upnp_init, NULL);
wzThreadStart(upnpdiscover);
std::thread t(upnp_init, std::ref(upnp_status));
t.detach();
}
else if (!NetPlay.isUPNP)
{
debug(LOG_INFO, "UPnP detection disabled by user.");
}
}

Expand All @@ -1100,6 +1090,7 @@ void NETdiscoverUPnPDevices(void)
int NETinit(bool bFirstCall)
{
debug(LOG_NET, "NETinit");
upnp_status = 0;
NETlogEntry("NETinit!", SYNC_FLAG, selectedPlayer);
NET_InitPlayers(true);

Expand Down Expand Up @@ -1791,6 +1782,34 @@ static void NETcheckPlayers(void)
// We should not block here.
bool NETrecvNet(NETQUEUE *queue, uint8_t *type)
{
switch (upnp_status)
{
case UPNP_ERROR_CONTROL_NOT_AVAILABLE:
case UPNP_ERROR_DEVICE_NOT_FOUND:
if (upnp_status == UPNP_ERROR_DEVICE_NOT_FOUND)
{
debug(LOG_NET, "UPnP device not found");
}
else if (upnp_status == UPNP_ERROR_CONTROL_NOT_AVAILABLE)
{
debug(LOG_NET, "controlURL not available, UPnP disabled");
}
// beware of writing a line too long, it screws up console line count. \n is safe for line split
addConsoleMessage(_("No UPnP device found. Configure your router/firewall to open port 2100!"), DEFAULT_JUSTIFY, NOTIFY_MESSAGE);
NetPlay.isUPNP_CONFIGURED = false;
NetPlay.isUPNP_ERROR = true;
upnp_status = 0;
break;
case UPNP_SUCCESS:
NETaddRedirects();
upnp_status = 0;
break;
default:
case 0:
ASSERT(upnp_status == 0, "bad value");
break;
}

uint32_t current;

if (!NetPlay.bComms)
Expand Down
9 changes: 2 additions & 7 deletions src/console.cpp
View file
Expand Up @@ -41,7 +41,6 @@
#include <string>
#include <istream>
#include <deque>
#include <mutex>

// FIXME: When we switch over to full JS, use class version of this file

Expand Down Expand Up @@ -71,7 +70,6 @@ struct CONSOLE_MESSAGE
bool team; // team message or not
CONSOLE_MESSAGE() : timeAdded(0), JustifyType(0), player(0), team(false) {}
};
wz::mutex mtx; // prevent adding messages when we are not expecting them
std::deque<CONSOLE_MESSAGE> ActiveMessages; // we add all messages to this container
std::deque<CONSOLE_MESSAGE> TeamMessages; // history of team/private communications
std::deque<CONSOLE_MESSAGE> HistoryMessages; // history of all other communications
Expand Down Expand Up @@ -148,7 +146,7 @@ bool addConsoleMessage(const char *Text, CONSOLE_TEXT_JUSTIFICATION jusType, SDW

if (!allowNewMessages)
{
return false ; // Don't allow it to be added if we've disabled adding of new messages
return false; // Don't allow it to be added if we've disabled adding of new messages
}

std::istringstream stream(Text);
Expand All @@ -163,7 +161,7 @@ bool addConsoleMessage(const char *Text, CONSOLE_TEXT_JUSTIFICATION jusType, SDW
std::string FitText(lines);
while (!FitText.empty())
{
int pixelWidth = iV_GetTextWidth(FitText.c_str());
int pixelWidth = iV_GetTextWidth(FitText.c_str(), font_regular);
if (pixelWidth <= mainConsole.width)
{
break;
Expand Down Expand Up @@ -197,7 +195,6 @@ bool addConsoleMessage(const char *Text, CONSOLE_TEXT_JUSTIFICATION jusType, SDW

consoleStorage.text = messageText;
consoleStorage.timeAdded = realTime; // Store the time when it was added
std::lock_guard<wz::mutex> lck(mtx); // Don't add messages unless locked!
if (player == INFO_MESSAGE)
{
InfoMessages.push_back(consoleStorage);
Expand Down Expand Up @@ -231,7 +228,6 @@ void updateConsoleMessages(void)
{
return;
}
std::lock_guard<wz::mutex> lck(mtx); // Don't remove messages unless locked.
for (auto i = InfoMessages.begin(); i != InfoMessages.end();)
{
if (realTime - i->timeAdded > messageDuration)
Expand Down Expand Up @@ -421,7 +417,6 @@ void displayConsoleMessages(void)
displayOldMessages(HistoryMode);
}

std::lock_guard<wz::mutex> lck(mtx); // Don't iterate without a lock.
if (InfoMessages.size())
{
auto i = InfoMessages.end() - 1; // we can only show the last one...
Expand Down

0 comments on commit 03d494d

Please sign in to comment.