Page MenuHomeWildfire Games

Prevent lobby crash when posting unsupported characters (Christmas tree crash)
ClosedPublic

Authored by elexis on May 8 2017, 7:31 AM.

Details

Summary

As found by coincidence by Hannibal_Barca last christmas (#4433), when sending someone a special character that is not supported by our utf8 code,
a debug breakpoint will be triggered. This character can be posted in the lobby to selectively or unselectively crash players.

It was briefly discussed in the staff meeting on April 9th 2017:

20:50:29 <Itms> elexis: for the crash, I think in this low level code, a safe thing to do would be to replace the character by one inside the BMP (maybe a square) and print an error
20:51:30 <elexis> yes, but it would remove the ability debug it, for example not show a stacktrace anymore
20:53:34 <elexis> and there might be many more places where debug breakpoints could be triggered for players other than the local one
20:53:49 <elexis> so a more general question might be whether we would disable breakpoints entirely for releases
20:54:42 <Itms> elexis: there is nothing we can do here except supporting utf8 chars outside that plane (which would be great but might never happen)
20:55:03 <Itms> elexis: and for releases, I'd say not for alphas or betas ^

Test Plan

Copy that character in the ticket and send it from a chat client as a PM to a 0ad player account in the lobby.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.May 8 2017, 7:31 AM
leper requested changes to this revision.May 8 2017, 7:39 AM
leper added inline comments.
source/lib/utf8.cpp
26 ↗(On Diff #1758)

(Yes, there is a precedent for this, but lib/ shouldn't depend on things outside of lib/)

80 ↗(On Diff #1758)

Read that Coding_Style wiki page again, the part about lib/ might be relevant.

88 ↗(On Diff #1758)

I guess just using debug_printf would work too. Or whatever the Logging wiki page lists for cases like this.

This revision now requires changes to proceed.May 8 2017, 7:39 AM
elexis marked 3 inline comments as done.May 8 2017, 8:21 AM
elexis added inline comments.
source/lib/utf8.cpp
26 ↗(On Diff #1758)

Thanks. That was stupid.

80 ↗(On Diff #1758)

Would be nice to have only one code style some day. (We could start by removing differences, like adding the period to the copyright header, see rP19503.)

88 ↗(On Diff #1758)

Not having them logged visually, nor in the logfile seems unfortunate.

Alternative being to raise some std::exception and try n catch it in the XmppClient. But that means other GUI places with unchecked user input that are passed to wstring_from_utf8 would remain affected. The lobby nickname and game name also use this function. Presumably these characters could also be sent as an ingame chat message or playername

elexis updated this revision to Diff 1759.May 8 2017, 8:24 AM
elexis edited edge metadata.
elexis marked 3 inline comments as done.

RTFM and don't include pyrogenesis code in source/lib/

Looks like it might work, needs testing though.

source/lib/utf8.cpp
80 ↗(On Diff #1758)

I don't see much use in adding huge commits for something like that (same for that . thing). Keeping things consistent is nice, but lib/ mostly is just that.

88 ↗(On Diff #1758)

Exceptions create complicated control flow that is hard to reason about, so I'd prefer if we didn't do that.

As for not logging them we might want to provide some form of logging interface for lib/ which could then be used to also fix the other use of LOG*. And it would allow logging them in a user visible way.

elexis marked 5 inline comments as done.May 8 2017, 9:01 AM
In D456#18392, @leper wrote:

Looks like it might work, needs testing though.

Proposing divide & conquer for that :-)

source/lib/utf8.cpp
80 ↗(On Diff #1758)

Agree. Just had added those periods (and made (c) with (C) consistent) in that commit for the files where the year number was bumped already. Now we have the choice between a big commit and a bigger commit (diff uploaded in that commit) or convincing the auditor that this is irrelevant. :s

88 ↗(On Diff #1758)

Good idea.

(We also have #include "ps/VideoMode.h" in lib/sysdep/os/unix/x/x.cpp and #include "ps/CStr.h" in lib/self_test.h)

Hannibal_Barca accepted this revision.May 8 2017, 9:21 AM

Tested it and it works.
However, I really don't see the need of removing this extra, albeit not legitimate resource of the lobby moderators.
Since you can't type damaging characters using 0 A.D. itself, this should not be a serious problem.

leper accepted this revision.May 8 2017, 1:18 PM

I'll assume this was tested by the other reviewer and by the author.

source/lib/utf8.cpp
80 ↗(On Diff #1758)

Well, for that I'd go with keeping lib/ (or rather all MIT licensed things) consistent, and all GPL things consistent.

88 ↗(On Diff #1758)

I'd guess that's because of g_xres and g_yres, and the other one is for EscapeToPrintableASCII or something like that.

Yes, those aren't great, but the logging thing sticks out like a sore thumb.

This revision is now accepted and ready to land.May 8 2017, 1:18 PM
Vulcan added a subscriber: Vulcan.May 8 2017, 11:31 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1084/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1085/ for more details.

This revision was automatically updated to reflect the committed changes.
elexis marked 2 inline comments as done.

Thanks for the review and testing.

source/lib/utf8.cpp
88 ↗(On Diff #1758)

There are some other debug_printf messages in source/lib/ that denote errors, so this new debug_printf likely won't be forgotton in case we implement the logger interface.

elexis added a comment.May 9 2017, 3:28 AM

However, I really don't see the need of removing this extra, albeit not legitimate resource of the lobby moderators.
Since you can't type damaging characters using 0 A.D. itself, this should not be a serious problem.

Agree it is handy to have a tool to disconnect ToU violating players from lobbied games. This was planned for D116 already, but didn't get to it.
But this capability must be restricted to moderators. As we can see from that NaN cheat, once the pack knows how to break things, it be abused frequently. One malicious player alone would suffice.
So I rather have this fixed for the upcoming release while we have the chance to.