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 #6598: Added company id check for connect console command. #8346

Closed
wants to merge 4 commits into from

Conversation

Foxar
Copy link

@Foxar Foxar commented Nov 29, 2020

I think I managed to stop the crashing mentioned in #6598 by adding a Company::IsValidID check to the connect console command, similiarly how it's handled in the join command.

It's my first time contributing so please notify me if I did something incorrectly, thank you.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An excellent patch, I believe this solves the issue well.

I am confused about what the numbers are doing though. The reconnect command above is explicitly +1 (whereas normally they're 0 indexed), but the helptext for all join/move/reconnect implies that they are 1-indexed, though it's rather hit and miss whether they're doing +1 to the company id or not. Probably needs checking all of them to make sure you can actually join as the first and last company. Error messages would need adjusting, certainly (MAX_COMPANIES + 1, etc)

src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Show resolved Hide resolved
@ldpl
Copy link
Contributor

ldpl commented Dec 3, 2020

I'd just like to point out that the original issue includes reports of the game crash, invalid game state and ai crash. And while adding preliminary id check in the command is alright it doesn't really change much as that company id is actually used much later and can become invalid. So it can't really be considered a definite fix for any of the aforementioned issues. At least not without a good explanation of what causes that and why.

@James103
Copy link
Contributor

James103 commented Dec 3, 2020

@Foxar Commit message must follow OpenTTD commit message format.
In this case, your recent commit message should begin with "Fix: ".

@LordAro
Copy link
Member

LordAro commented Dec 3, 2020

yes, we're quite picky about commit messages here. Take a look at "squashing" or "rebasing" with git.

@Foxar Foxar requested a review from LordAro December 3, 2020 18:41
src/console_cmds.cpp Outdated Show resolved Hide resolved
@Foxar Foxar requested a review from LordAro December 4, 2020 08:57
@Foxar
Copy link
Author

Foxar commented Dec 4, 2020

Sorry to be a bother, but could someone explain why the commit checker fails with 'git fatal: error reading section header 'acknowledgments'' ? I'm just a beginner in git, and googling yields no results.

@glx22
Copy link
Contributor

glx22 commented Dec 4, 2020

It was just an internal GitHub action issue, I just restarted the check

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: trivial This Pull Request is trivial labels Dec 14, 2020
@@ -919,6 +919,12 @@ DEF_CONSOLE_CMD(ConNetworkConnect)
IConsolePrintF(CC_DEFAULT, "Connecting to %s...", ip);
if (company != nullptr) {
join_as = (CompanyID)atoi(company);
IConsolePrintF(CC_DEFAULT, "Connecting as company id %d ...", join_as);
/* Check if we have a valid company id! */
if(!Company::IsValidID(join_as) && join_as != COMPANY_SPECTATOR) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation is broken in a subtle manner due to line 934 below (join_as--), which has all to do with the showing the users number 1 to 15 whereas the IDs internally are 0 to 14 (with 15 being INVALID_COMPANY).
connect localhost#0 will make you join as spectator (join_as is a byte, so 0 - 1 = 255 which is equal to COMPANY_SPECTATOR).
connect localhost#15 will give you an error message instead of joining company 15
Coding style wise there should be a space between the 'if' and '(', i.e. 'if (' instead of 'if('.

@rubidium42
Copy link
Contributor

Foxar, upon further investigation of the original issue you are trying to solve I think you are trying to solve something that is not the problem. Have said that, the many commenters seem to not have realized that either so it is indeed something difficult to wrap your head around.
You are guarding against using an invalid company number, however there is already a guard against that a few lines below where you are looking.
The whole point of the underlying issue seems to be that if/when that validation fails, the client is in some weird state. This is due to the disconnect at the begin of the function. So even with your patch the original issue remains, we just added a duplicate check.
Thanks for your effort and attempt, but I'll close this PR as it is sadly not the solution to the problem.

@rubidium42 rubidium42 closed this May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: trivial This Pull Request is trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants