Bug #2179
open
Server segfault when city cannot provide goods
Added by Dean Brown about 2 months ago.
Updated 2 days ago.
Description
Supplied rulesets are OK, but a clumsy custom ruleset author can make a game.ruleset goods section such that it may sometimes be impossible for a city to provide any goods. In that case, routine goods_from_city_to_unit() returns NULL, and some places in the code down the line that depend on it are not checking.
This patch prevents crashing, but I'm not entirely happy about the resulting situation. That poor good-less trade unit will likely never be able to "Enter Marketplace" nor "Establish Trade Route", and its owner (human or AI) will have no clue why. Could still do "Help Wonder" or "Disband Unit Recover". I thought of an alternate solution - instead of returning NULL, have goods_from_city_to_unit() return a pointer to a static struct goods_type called default_goods, but that is problematic. That good would not be in the ruleset but might have the same name, the struct's id would have to be outside the legal range, and I don't know any way to construct a struct requirement_vector for it.
Another alternate solution that I think would not be workable is to check at ruleset loading time that cities can always provide goods.
Files
Related issues
1 (1 open — 0 closed)
- File 2179_Main.patch added
- File 2179_S3_3.patch added
- Category set to General
- Target version set to 3.3.0-beta1
From the patch it seems that the problem is always with accessing replace_priority. That's a new S3_3 feature.
- I haven't checked in detail, but am guessing that passing priority lower than "normal" == 1 would work better, by not ending recommending replacing existing trade route with one without goods. Should be possible to go to 0 at least, maybe -1
- That seems like a lot of duplication. I would add an helper function unit_current_good_replace_priority()
- File deleted (
2179_Main.patch)
- File deleted (
2179_S3_3.patch)
Your suggestion of helper routine results in a more elegant solution. I went with -1, replace_priority is signed int so that's valid value. Prefer that to 0 because a crazy 0-based C coder might use replace_priority=0 in their game.ruleset.
- Do those functions need to figure out unit_current_goods() for other reasons too, or could that part too be incorporated/hidden in the unit_current_good_replace_priority(), like its name currently suggests. If not, maybe rename the function.
- Use 'nullptr' instead of 'NULL' in comments too.
Also available in: Atom
PDF