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

Possible for client to be in a nonexistent company on a server; but then the client crashes. #6598

Open
DorpsGek opened this issue Aug 4, 2017 · 5 comments

Comments

@DorpsGek
Copy link

@DorpsGek DorpsGek commented Aug 4, 2017

james1101 opened the ticket and wrote:

To Reproduce this Crash:
Start OTTD server.
Remember it's ip address.
Start OTTD client.
Client: Connect to server.
Client: Execute 'join 255'.
-> Client joins spectators.
Client: Execute 'join N', where N is greater than 15 and less than 255.
-> Client throws: 'Error: Company does not exist.'
Client: Execute 'connect # N', where N is greater than 15 and less than 255.
-> Client: Connect should be successful.
Client: Open company list.
-> Client crashes. Crash files are attached.

Client version 1.7.1 on Windows 7 Ultimate; Server version 1.7.1 on Windows 7 Ultimate

Suggested Fix: Make the company id part of the connect command's argument follow the same rules as the join command's company id argument, ie must be between 0 and max_companies, or 255 (spectator).

Attachments

Reported version: 1.7.1
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/6598
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Aug 15, 2017

Wolf01 wrote:

I can't reproduce it on Win 10.
Tried with local server both with IPv4 and IPv6.
Tried logging in as a company 1 and spectator.
Tried connecting from titlescreen.

The command already checks for join_as > MAX_COMPANIES:

 if (join_as != COMPANY_SPECTATOR) {
 	if (join_as > MAX_COMPANIES) return false;
 	join_as--;
 }

This comment was imported from FlySpray: https://bugs.openttd.org/task/6598#comment14523
@DorpsGek

This comment has been minimized.

Copy link
Author

@DorpsGek DorpsGek commented Aug 15, 2017

dp wrote:

Ok, I didn't get any crashes, but I think it's close enough. Thing is connect cmd sometimes leaves the game in a weird half-disconnected state. It's not very noticable at first but it's clearly not a valid state. E.g. you can build rails but can't leave company or use join command. Or you can bring up mp lobby while still being in game.
There are several ways to get in that state:

  1. connect ::1# 254 (or anything 16-254)
  2. connect ::1# 1 and cancelling password prompt (company 1 has to be passworded)
  3. connect ::1:0

I think I got all such cases in my patch but can't be 100% sure.
Also rearranged console output a bit so it doesn't say "connecting ..." before failing on arg parsing.
And added check for negative company # to prevent possible bugs with it.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6598#comment14526
@TrueBrain

This comment has been minimized.

Copy link
Member

@TrueBrain TrueBrain commented Apr 12, 2018

Patch contains many things I am unsure are part of the solution. But it is a good start for solving this issue :)

@andythenorth

This comment has been minimized.

Copy link
Contributor

@andythenorth andythenorth commented Jan 12, 2019

Thanks for this. Valid issue, but there's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. I'm closing it as we try to keep the issue count low for OpenTTD, it helps us focus on things that are important and fun. Feel free to discuss in irc or request re-opening if you disagree. Thanks for contributing!

@nikolas

This comment has been minimized.

Copy link
Member

@nikolas nikolas commented Aug 6, 2019

Here's a crash.log triggered by a related scenario:

  • Enable an AI player to start in the game immediately. I used AdmiralAI.
  • Host a game, and put a password on it
  • From a second client, join the game, and join that company. (connect ::1#1 from the console.)
  • In the console of the second client, do: connect ::1#1 (effectively trying to join the same company). Don't type in the server password yet.
  • In this state, where the server password is being prompted, you can crash the second client in any number of ways. The first ones I've found: Open up the Company window, and try and change the company name, or president's name. Cancelling the password dialog box causes a crash as well.

Additionally, writing the crash savegame fails here (resolved by #7684):

Writing crash savegame...
Error: Assertion failed at line 286 of /home/nik/src/OpenTTD/src/ai/ai_core.cpp: c != nullptr && c->ai_instance != nullptr
Aborted

nikolas added a commit to nikolas/OpenTTD that referenced this issue Aug 6, 2019
This fixes an assertion failure created by the scenario
outlined here:
OpenTTD#6598 (comment)

When a client gets disconnected from a network game that has an AI, and
then crashes, it tries to write a crash.sav. Attempting to save the AI
instance here, which doesn't exist on the client, causes the assertion
failure.

This change allows openttd to crash properly in this awkward scenario :P

crash.sav and crash.png are now written:

    Writing crash log to disk...
    Crash log written to /home/nik/.openttd/crash.log. Please add this file
    to any bug reports.

    Writing crash savegame...
    Crash savegame written to /home/nik/.openttd/crash.sav. Please add this
    file and the last (auto)save to any bug reports.

    Writing crash screenshot...
    Crash screenshot written to /home/nik/.openttd/crash.png. Please add
    this file to any bug reports.

    Aborted
@TrueBrain TrueBrain added the pinned label Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.