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
Conversation
There was a problem hiding this 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)
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. |
@Foxar Commit message must follow OpenTTD commit message format. |
yes, we're quite picky about commit messages here. Take a look at "squashing" or "rebasing" with git. |
ff2ce1d
to
e82c32b
Compare
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. |
It was just an internal GitHub action issue, I just restarted the check |
@@ -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) { |
There was a problem hiding this comment.
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('.
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. |
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.