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

Open
wants to merge 4 commits into
base: master
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

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 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 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 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 force-pushed the Foxar:Fixed-concmd-connect branch from ff2ce1d to e82c32b Dec 3, 2020
Foxar added 2 commits Dec 3, 2020
@Foxar Foxar requested a review from LordAro Dec 3, 2020
src/console_cmds.cpp Outdated Show resolved Hide resolved
@Foxar Foxar requested a review from LordAro Dec 4, 2020
@Foxar
Copy link
Author

@Foxar 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 glx22 commented Dec 4, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants