Don't access items in empty cache #2332
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jwrober
approved these changes
Aug 2, 2024
|
@lmoureaux Would you like to review this PR? It looks good to me and seems to fix the issue reported. |
|
In particular, I could imagine that a solution along these lines would be better, because it effectively continues to use the cache. output_type_iterate(stat_index)
{
int amount = 0;
if (pcsoutputs && !pcsoutputs->empty())
{
amount = pcsoutputs->at(sp)[stat_index];
}
if (!pcsoutputs)
{
amount = get_specialist_output(pcity, sp, stat_index);
}
output[stat_index] += count * amount;
}However, I was not sure how to interpret the empty cache. |
|
Yes, I want to check where the empty cache comes from. Need to check the code again ^^ |
|
The cache is not created when the governor settings forbid specialists. Stack trace of the crash: |
lmoureaux
added a commit
to lmoureaux/freeciv21
that referenced
this pull request
Aug 30, 2024
Governors that forbid specialists do not bother computing how much specialists would produce and end up with an empty specialist output cache. When applying solutions to evaluate them, however, we were trying to use the cached values. Only do it when there specialists are actually present in the solution. See longturn#2332.
lmoureaux
added a commit
to lmoureaux/freeciv21
that referenced
this pull request
Aug 30, 2024
Governors that forbid specialists do not bother computing how much specialists would produce and end up with an empty specialist output cache. When applying solutions to evaluate them, however, we were trying to use the cached values. Only do it when there specialists are actually present in the solution. See longturn#2332.
|
#2355 addresses the root cause but in a less defensive way. |
lmoureaux
added a commit
that referenced
this pull request
Aug 30, 2024
Governors that forbid specialists do not bother computing how much specialists would produce and end up with an empty specialist output cache. When applying solutions to evaluate them, however, we were trying to use the cached values. Only do it when there specialists are actually present in the solution. See #2332.
|
Even after #2355 we may want to keep checking that the cache isn't empty. I'm only mildly optimistic that the issue won't appear in a different code path. |
Apply the same defensive measure, we applied to specialists output cache, to the waste cache.
blabber
force-pushed
the
bugfix/empty_caches
branch
from
225b4ce
to
0daf5f5
Compare
lmoureaux
requested changes
Aug 30, 2024
common/city.cpp
Outdated
| @@ -2247,6 +2247,9 @@ void add_specialist_output( | |||
| // This is more than just an optimization. For governors that forbid | |||
| // specialists, the cache may not be filled. | |||
| if (count > 0) { | |||
| // If there is a cache it must not be empty. | |||
| fc_assert(!pcsoutputs || !pcsoutputs->empty()); | |||
There was a problem hiding this comment.
fc_ asserts aren't fatal by default. Use fc_assert_action(condition, continue) to skip this specialist, which is probably an ok fallback behavior:
Suggested change
| fc_assert(!pcsoutputs || !pcsoutputs->empty()); | |
| fc_assert_action(!pcsoutputs || !pcsoutputs->empty(), continue); |
There was a problem hiding this comment.
I implemented another solution, falling back to the old behaviour by setting the cache to nullptr if the assertion triggers. This makes sure, that the game will behave correctly. What do you think?
common/city.cpp
Outdated
| @@ -2876,6 +2879,9 @@ void set_city_production(struct city *pcity, | |||
| * on, so shield waste will include shield bonuses. */ | |||
| output_type_iterate(o) | |||
| { | |||
| // If there is a cache it must not be empty. | |||
| fc_assert(!pcwaste || !pcwaste->empty()); | |||
There was a problem hiding this comment.
This should probably go before the loop to avoid getting 6 messages every time. I think returning if the assert fails is fine, so use fc_assert_ret(condition).
Fallback to the behavior used prior to the introduction of these caches, if the assertion checking for an empty cache is triggered. While here, move an assertion outside a loop to make sure it is not checked for each iteration.
blabber
force-pushed
the
bugfix/empty_caches
branch
from
5783403
to
9ed47c0
Compare
lmoureaux
approved these changes
Sep 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While debugging an issue reported in LT85, I found that in certain circumstances an empty specialist output cache (introduced in 5c44029) is accessed, what crashed the client.
An obvious fix, contained in this PR, is limiting access to non-empty caches. There may be better ways to handle this though.
While here, I applied the same safety measure to the waste level cache, introduced in ecabf5d.