Prevent DSCM-Users from crashing others

[Chronial]

There are two ways to do this:

1. Change the name in memory
2. When we detect an invalid name, close DSCM and display a popup that explains the situation and how to fix it.

We should implement (2) soon and change over to (1) if somebody has time and energy.

[Chronial]

@Jellybaby34 Do you have knowledge of the problem? Is it enough to check that the name is < 16 chars and does not contain #?

[Wulf2k]

Do you have any evidence that this is happening at any time other than summoning/invasions?

The post would suggest that it is, but if that were the case then I'd expect far more reports of DSCM crashing people. One person with a bad name would poison the network and nuke everybody in that level range / area.

I was actually going to tackle (1) this weekend.

[Wulf2k]

Minor note, I believe it's actually a "#c" that can't exist, as that should indicate a colour code. I haven't tested myself but I believe a lone "#" is fine.

Edit: This is reversed. a lone # will crash it, but at least when examining your own sign a #c is silently removed.

[Chronial]

I linked the wrong thread. I meant this one: http://steamcommunity.com/app/211420/discussions/0/358415103477070691/

[Wulf2k]

Ah, k. That one makes sense.

Yes, no surprises there but we should definitely fix that one for people.

[Jellybaby34]

The best approach is shortening names to 16 characters maximum and replacing any #'s with another character e.g. "!".

Shortening the name to 16 characters fixes one of the crashes that occur when touching summon signs which was due to a buffer overflow. It also fixes an issue where if the invaders name was longer than 16 characters, rather than just copy the first 16 bytes it would just leave the copy function and the invader would be using whatever name the previous invader had.

Stripping any instance of the # in player names fixes the other type of crashing due to incorrectly formatted text tagging. The game enters a routine when it reads a # in the string its about to display and if it isn't formatted like it expects i.e. #c[FFFFFF]txthere#c it just dies.

Furthermore the name used when you touch your own summon sign is queried directly through the steam api so even if you purged any #'s in a persons name they would still crash if they touched their own sign.

TL;DR Best fix is to replace any #'s in the persons name with another character and limit it to 16 characters. Only issue then is crashing when touching your own summon sign due to it getting your steam name through the steam api

[Chronial]

@Wulf2k Did you test that your fix actually prevents crashing of other players in the various scenarios?

[Jellybaby34]

Had a quick look and it works at removing #'s and shortening names so that should prevent people inadvertently crashing those who aren't running the appropriate fixes. Only nitpick is that it shortens names to 15 characters rather than 16 which is the maximum that can be used before it crashes others.

[Wulf2k]

Doesn't the 16th character have to be a 0x00, or else it will just blindly
keep reading whatever's in the 17th slot and beyond?

I had nobody on to test with last night. Still, easy test and fix at some
point.
On Jun 18, 2016 16:05, "Jellybaby34" notifications@github.com wrote:

Had a quick look and it works at removing #'s and shortening names so that
should prevent people inadvertently crashing those who aren't running the
appropriate fixes. Only nitpick is that it shortens names to 15 characters
rather than 16 which is the maximum that can be used before it crashes
others.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#108 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AOMBrjV4-ziCvaCpuQkhNSEmsWFnpb_tks5qNF2LgaJpZM4I2dS1
.

[Wulf2k]

Chronial, I tested as much as I could by checking my own summoning sign but
that's a different function than reading others, so not perfect.
On Jun 18, 2016 16:52, "Lane Hatland" wulf2k@gmail.com wrote:

Doesn't the 16th character have to be a 0x00, or else it will just blindly
keep reading whatever's in the 17th slot and beyond?

I had nobody on to test with last night. Still, easy test and fix at some
point.
On Jun 18, 2016 16:05, "Jellybaby34" notifications@github.com wrote:

Had a quick look and it works at removing #'s and shortening names so
that should prevent people inadvertently crashing those who aren't running
the appropriate fixes. Only nitpick is that it shortens names to 15
characters rather than 16 which is the maximum that can be used before it
crashes others.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#108 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AOMBrjV4-ziCvaCpuQkhNSEmsWFnpb_tks5qNF2LgaJpZM4I2dS1
.

[Jellybaby34]

You're right. Its 15 then the null byte or else it overruns the stack buffer. Sorry for the wrong information, I thought it was 16 characters.

[Wulf2k]

Doh, i just took your word for it and upped it to 16 last night. I'll put
it back later.

Got a code for another copy of Dark Souls last night so these things should
be easier to test now.

You're right. Its 15 then the null byte or else it overruns the stack
buffer. Sorry for the wrong information, I thought it was 16 characters.

[Chronial]

@Wulf2k I had a look at what the game does, and a big portion of the locations the namecrash fix patches are places in which the name was just directly retrieved from the steam api. So it doesn't matter what's reported via the game's mechanic.

Given that information I think we should check for the player's name and if it violates the name rules, DSCM should refuse to work and display a big message instead that describes the problem and tells the user how to fix it.

If we allow bad-name users to use DSCM, we help them crash as many machines as possible, which I don't think we should do ^^.

Are you fine with my proposal or do you disagree?

[Wulf2k]

I'm ok with it in theory, but how many calls are there?

Would it be reasonable to just patch each call so it does an extra "jmp 0xOURCODE, mov al, 00, mov [edi+15], al, jmp 0xORIGCODE"?

Edit: Er, that fixes the local crash but not the outbound crash.

Yeah, we should be forceful.