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

Conversation

@uvealonso
Copy link
Contributor

commented Aug 25, 2019

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

This comment has been minimized.

Copy link
Contributor

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
src/os/macosx/string_osx.cpp Outdated Show resolved Hide resolved
@uvealonso

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 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.

Done!

@nielsmh nielsmh added the Mac OS label Aug 31, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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 :)

Oh sure. I'll fix it now

@uvealonso

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Done!

src/os/macosx/string_osx.cpp Outdated Show resolved Hide resolved
@uvealonso

This comment has been minimized.

Copy link
Contributor Author

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 approved these changes Sep 1, 2019
@michicc
michicc approved these changes Sep 1, 2019
@michicc michicc merged commit ead7723 into OpenTTD:master Sep 1, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190901.1 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@nielsmh

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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.

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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.

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

This comment has been minimized.

Copy link
Contributor Author

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.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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.

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

@LordAro LordAro added the backported label Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.