Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #7704: [OSX] Handle malformed UTF8 strings #7705

Merged
merged 1 commit into from Sep 1, 2019
Merged

Fix #7704: [OSX] Handle malformed UTF8 strings #7705

merged 1 commit into from Sep 1, 2019

Conversation

uvealonso
Copy link
Contributor

No description provided.

@uvealonso uvealonso changed the title Fix #7704 avoiding badformed UTF8 strings Fix #7704 avoiding MacOS crash when badformed UTF8 strings Aug 25, 2019
@glx22
Copy link
Contributor

glx22 commented Aug 26, 2019

To make the commit checker happy, you'll need to fix your commit messages following https://wiki.openttd.org/Coding_style#Commit_message
I think you could also squash all your commits into one, and your latest commit adds an unneeded space in the comment.

src/os/macosx/string_osx.cpp Outdated Show resolved Hide resolved
@uvealonso
Copy link
Contributor Author

To make the commit checker happy, you'll need to fix your commit messages following https://wiki.openttd.org/Coding_style#Commit_message
I think you could also squash all your commits into one, and your latest commit adds an unneeded space in the comment.

Done!

@nielsmh nielsmh added the OS: MacOS This issue is related to a Mac OS problem label Aug 31, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Aug 31, 2019

You forgot the colon after the ticket number in the commit message.
Fix #7704: [OSX] Handle malformed UTF8 strings

@uvealonso uvealonso changed the title Fix #7704 avoiding MacOS crash when badformed UTF8 strings Fix #7704: [OSX] Handle malformed UTF8 strings Aug 31, 2019
@LordAro
Copy link
Member

LordAro commented Aug 31, 2019

Note that the commit message itself must be amended, not just the PR title

Sorry about this, we're very particular about our commit history :)

@uvealonso
Copy link
Contributor Author

Note that the commit message itself must be amended, not just the PR title

Sorry about this, we're very particular about our commit history :)

Oh sure. I'll fix it now

@uvealonso
Copy link
Contributor Author

Done!

src/os/macosx/string_osx.cpp Outdated Show resolved Hide resolved
@uvealonso
Copy link
Contributor Author

uvealonso commented Sep 1, 2019

This is a fix that prevents crash on OSX, but this doesn't fix the real origin of the problem, which is permitting servers to have badformed UTF8 strings... :-/

@michicc michicc merged commit ead7723 into OpenTTD:master Sep 1, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Sep 1, 2019

It would be impossible to prevent anyone from making a patched game that intentionally sends bad UTF8 to network clients, but the master server possibly should reject servers registering with invalid text.

@uvealonso
Copy link
Contributor Author

It would be impossible to prevent anyone from making a patched game that intentionally sends bad UTF8 to network clients, but the master server possibly should reject servers registering with invalid text.

Yeah, a patched game can intentionally send bad UTF8, that's right. But I think that the server that currently have bad UTF8 that makes my OpenTTD client to crash didn't make it intentionally. The server with which I detected the error was an czech server that has no intention of creating any issue to clients.

Btw, when is this patch going to be integrated in official compiled game? I can not play until servers stop of using bad UTF8 strings :(

@uvealonso
Copy link
Contributor Author

Btw, when is this patch going to be integrated in official compiled game? I can not play until servers stop of using bad UTF8 strings :(

@michicc , @LordAro

@nielsmh
Copy link
Contributor

nielsmh commented Sep 1, 2019

I think the best short-term solution is to patch the master server so it delists servers with invalid text in any field.

@uvealonso
Copy link
Contributor Author

It would be impossible to prevent anyone from making a patched game that intentionally sends bad UTF8 to network clients, but the master server possibly should reject servers registering with invalid text.

I think the best short-term solution is to patch the master server so it delists servers with invalid text in any field.

I've made a quick check of master server's code, and I have not seen any message that implies server-name info at the time of registration. Am I ok?

@uvealonso
Copy link
Contributor Author

It would be impossible to prevent anyone from making a patched game that intentionally sends bad UTF8 to network clients, but the master server possibly should reject servers registering with invalid text.

I think the best short-term solution is to patch the master server so it delists servers with invalid text in any field.

I've made a quick check of master server's code, and I have not seen any message that implies server-name info at the time of registration. Am I ok?

Well... I think that payload on packet PACKET_UDP_SERVER_RESPONSE is not analysed in function QueryNetworkUDPSocketHandler::Receive_SERVER_RESPONSE (masterserver/src/masterserver/udp.cpp), but info is sent.

I have to make it to compile and run, and then make some testings.

Buuuuuut, there's no possibility of creating issues on masterserver repo :(

@nielsmh
Copy link
Contributor

nielsmh commented Sep 1, 2019

This seems to be the code that receives the server data response:
https://github.com/OpenTTD/MasterServer/blob/a6be9858f385fd0e092836682a25c5142cf9923e/src/updater/udp.cpp#L21-L65

It doesn't do much validation of the data, which would need to be added.

@uvealonso
Copy link
Contributor Author

This seems to be the code that receives the server data response:
https://github.com/OpenTTD/MasterServer/blob/a6be9858f385fd0e092836682a25c5142cf9923e/src/updater/udp.cpp#L21-L65

It doesn't do much validation of the data, which would need to be added.

Yep, that's interesting. I think there's something that can be done...

@nielsmh nielsmh added the backport requested This PR should be backport to current release (RC / stable) label Sep 4, 2019
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable) OS: MacOS This issue is related to a Mac OS problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants