Ticket #44158

broadcast_city_info has apparently illogical code

Open Date: 2022-03-22 14:06 Last Update: 2022-04-06 11:59

Reporter:
Owner:
Type:
Status:
Closed
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
Fixed
File:
3

Details

in freeciv/freeciv master branch, citytools.c, broadcast_city_info Line 2182, https://github.com/freeciv/freeciv/blob/5bcfc01b1cd0a721764ff64b8706a037826294d1/server/citytools.c#L2182

We iterate all players to see if they can see inside the city, presumably so they can "see" the internal city data. But each time they qualify to see it, we make that info accessible to the city **owner.** So, for every pplayer who can see inside the city, we repeatedly and redundantly allow the city owner access to see inside their city. Not the pplayer who is supposedly able to see.

This ticket submitted by request... copy-paste of cazfi response: "Can you open a ticket about that? The code makes no sense, while it's less clear what exactly is wrong with it (seems to me that it has actually multiple problems, but need to confirm that with more time)"

NOTE: for the purposes FCW was able to achieve, changing powner to pplayer worked. If there are other issues when all this gets rewritten, please send me a courtesy notification. Thanks!

Ticket History (3/15 Histories)

2022-03-22 14:06 Updated by: lexxie9952
  • New Ticket "broadcast_city_info has apparently illogical code" created
2022-03-23 09:12 Updated by: cazfi
  • Milestone Update from (None) to 3.0.1 (closed)
  • Component Update from (None) to Server
2022-03-24 01:25 Updated by: cazfi
Comment

After more careful checking I think the fact that info is sent to powner instead of pplayer is the only actual bug. The code could be better arranged, and the variable 'send_city_suppressed' more accurately named (my assumption about is was causing me to think there to be other bugs)

- 'send_city_suppressed' name does not make it clear it applies to send to *owner* only. One would assume that if info is sent to anyone despite that flag, it would owner who still gets latest information
- It's a bit weird that 'send_city_suppressed' check is only inside 'can_player_see_city_internals()' block, not affecting else and 'can_player_see_city_externals()' block. This is not an actual bug simply because the city owner always 'can see city internals', so owner-specific 'send_city_suppressed' is never relevant in the else branch.

I may open a new ticket about renaming 'send_city_suppressed'. In this ticket I plan to change information to be sent to pplayer, and to move "if (!send_city_suppressed ..." check outside whole 'if (can_player_see_city_internals()) {} else {}' construct. Both for clarity and performance (no point to figure out can_player_see_city_internals() if the simpler checks are about to prevent the send anyway.

2022-03-24 01:42 Updated by: cazfi
  • Owner Update from (None) to cazfi
  • Resolution Update from None to Accepted
Comment

- Going to push also to S2_6
- Master/S3_1 patch has the refactoring I described earlier, S3_0 & S2_6 patches implement the minimum change to fix the bug.

2022-03-24 05:17 Updated by: lexxie9952
Comment

Thanks! It turned out to be almost exactly same as my fix, but I noticed your changing the "if" might be faster performance.

2022-03-31 19:22 Updated by: lexxie9952
Comment

Hello. After I implemented a small difference between my fix and your fix, a week later we noticed a bug and finally tracked it back to this patch. It turns out the "performance enhancement" of changing the order of the outer and inner IF statements, causes a bug. The bug can be found by attacking a city with killcitizen unitclass, poison city, etc. You'll see it doesn't update the client. The cause is because the "else" formerly responded to the not-true condition of the outer if, which was then made to an inner if, which caused the "else" statement to trigger itself under different logical contexts, and this bug is a "falling through the cracks" in the adjusted logic of the otherwise-case condition to the first outer if statement!

See https://github.com/Lexxie9952/fcw.org-server/commit/17bc5abe49aa435a4e1ca1ed08073611cd3ad785 After doing this fix, the bug disappeared for us.

2022-03-31 23:54 Updated by: cazfi
  • Resolution Update from Accepted to None
2022-04-02 06:49 Updated by: cazfi
Comment

Reply To lexxie9952

See https://github.com/Lexxie9952/fcw.org-server/commit/17bc5abe49aa435a4e1ca1ed08073611cd3ad785 After doing this fix, the bug disappeared for us.

That's not complete revert of the rearrangement I made. You would need to move the "else" block too, to match correct "if"

Not that I yet can see what the exact problem with my patch is.

2022-04-02 07:12 Updated by: cazfi
  • Resolution Update from None to Accepted
Comment

Reply To cazfi

That's not complete revert of the rearrangement I made. You would need to move the "else" block too, to match correct "if" Not that I yet can see what the exact problem with my patch is.

Checking the FCW code it seems that something had gone wrong when you applied the original patch, so you really had only that part of it. Which explains the issues you saw. I think my patch is ok after all.

2022-04-02 11:42 Updated by: lexxie9952
Comment

Hello. Hopefully I'm not annoying on this. But I am finding trouble with what you said. The current FCW code compared to current upstream code looks like this:

[https://gyazo.com/b51dd247004aaa9c235b14e77ea2476b

To me it seems the only difference in this is the powner was changed to pplayer in the right spots. So far, so good?

Correct me where I go wrong, please!

This is how it compares to your patch to the upstream code:

https://gyazo.com/bbe2968357dc102b8dcccc8bb8107430

If there is any action I should take please let me know. I am just confused because it was buggy and now I got it working with that change.

2022-04-02 17:37 Updated by: cazfi
Comment

Reply To lexxie9952

Hello. Hopefully I'm not annoying on this. But I am finding trouble with what you said. The current FCW code compared to current upstream code looks like this: [https://gyazo.com/b51dd247004aaa9c235b14e77ea2476b To me it seems the only difference in this is the powner was changed to pplayer in the right spots. So far, so good?

Yes, that's the situation before applying my patch to freeciv.org code.

Now, look at the patch. You had ever applied only part of it:

- if (can_player_see_city_internals(pplayer, pcity)) {
- if (!send_city_suppressed || pplayer != powner) {
+ if (!send_city_suppressed || pplayer != powner) {
+ if (can_player_see_city_internals(pplayer, pcity)) {

Having only that is wrong as it changes what "if" the "else" block is part of. The latter part of the patch begins with:

- }
- } else {
- if (player_can_see_city_externals(pplayer, pcity)) {
+ } else if (player_can_see_city_externals(pplayer, pcity)) {

The crucial bit being the very first removal of a "}"

2022-04-06 11:59 Updated by: cazfi
  • Status Update from Open to Closed
  • Resolution Update from Accepted to Fixed

Edit

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login