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: ensure no more than the allowed number of NewGRFs are loaded from the configuration #9430

Merged
merged 1 commit into from Jul 11, 2021

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Jul 11, 2021

Motivation / Problem

When loading OpenTTD with a configuration that has more than NETWORK_MAX_GRF_COUNT NewGRFs configured bad things might happen.

The UI limits the user from entering more than that amount, but loading the configuration does not perform such sanity checks.

With older versions, including 1.11.2, this can cause an assertion or when assertions are disabled a buffer overrun when advertising to the master server.

With versions since the packet rewrite this causes only the assertion as the buffer will automatically resize if needed. However, the packets might be interpreted incorrectly by others, especially the UDP packets.

With any, but more likely for #9428, this could lead to an overflow of the number of NewGRFs in the packet, meaning the clients see 0 NewGRFs when the server actually has 256.

Description

The issue is solved by limiting the number of non-static NewGRFs that are loaded from the configuration file.

Limitations

Saving the configuration will now remove any NewGRFs over the limit from the configuration, so going back and forth between stable and post #9428 might loose NewGRFs in the configuration if you have set more than 62.

This change doesn't magically fix the problem of loading valid post #9428 settings files in 1.11. That requires this PR to be backported.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@rubidium42 rubidium42 added the backport requested This PR should be backport to current release (RC / stable) label Jul 11, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

The second issue is solved by limiting the number of NewGRFs that are loaded from the configuration using the now accessible constants.

Although strictly true, you are overselling the solution here. Assuming it won't take months to approve my PR to increase the limit, this only "solves" the issue for 1 or 2 commits :P So we only solve it if we in the future ever increase the limit again, I guess :) And of course for people manually adding more entries in the ini-files.

It still leaves the issue that if you start a server in 1.11 with a configuration from 1.12 you could crash the game. But we have had this more often, like with a zoom setting from 1.10 to 1.11. Not sure those problems are solveable without releasing a 1.11.3 .. and I am not sure that is useful :D

src/newgrf_config.cpp Outdated Show resolved Hide resolved
@rubidium42
Copy link
Contributor Author

The second issue is solved by limiting the number of NewGRFs that are loaded from the configuration using the now accessible constants.

Although strictly true, you are overselling the solution here. Assuming it won't take months to approve my PR to increase the limit, this only "solves" the issue for 1 or 2 commits :P So we only solve it if we in the future ever increase the limit again, I guess :) And of course for people manually adding more entries in the ini-files.

The issue remains after merging #9428, and there it is actually more silent. A server with 256 NewGRFs will advertise 0, while sending 256 identifiers. Clients will join the game without the appropriate NewGRFs and yuck.
Since the configuration is very human-editable, it should be considered unsafe and made safe for consumption by OpenTTD. That is exactly what this PR does.

It is true though that with #9428 it becomes easier for an end user to trigger this issue in older versions of OpenTTD, but #9428 was only the cause that triggered me to think about it.

@rubidium42 rubidium42 merged commit f6955a3 into OpenTTD:master Jul 11, 2021
@rubidium42 rubidium42 deleted the harden_newgrf_loading branch July 11, 2021 09:20
@TrueBrain TrueBrain removed the backport requested This PR should be backport to current release (RC / stable) label Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants