[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Jacob Nevins]

Follow-up Comment #32, bug #18776 (project freeciv):

Further eve-of-2.3.1 analysis of the issues and patches surrounding this
ticket. (Probably shortly to be split to several tickets.)



== Vision cleanup when player removed ==

file #14580: 0006-fix-bug-in-player_map_free.patch
(basically the same as file #14319 tested by akfaew in comment #14)

This is the first patched area from this bug.

_Bug:_ if I understand correctly, if a player is removed mid-game, various
consequences of its vision / tile knowledge aren't cleaned up properly,
including in data structures for remaining players. Consequences apparently
include segfaults in running games (e.g., comment #1).

Cannot have any effect if players are removed pre-game (rather than in-game),
since players haven't got any vision/knowledge at that point? Therefore, if
we're not going to fix it for 2.3.1, temporarily reverting in-game-remove
(i.e. reverting bug #18641) is a reasonable reaction, so that people don't run
into this.

_Risk/benefit:_ it touches player_map_free(). This is called by
server_remove_player(), but also from server_player_init(), which has ten call
sites, mostly to do with initial creation of players rather than removal. Call
me paranoid, but without more analysis than I have time for, I can't convince
myself that there's no risk of it breaking one of these cases, obscure (civil
war, barbarian creation) or otherwise (savegame loading), say due to tickling
a latent bug in the vision code by calling it from a new place.

*Conclusion:* Don't apply this patch for 2.3.1. Revert fix for bug #18641
(and deal with i18n consequences of doing that). Apply immediately after 2.3.1
release, to get confidence in fix.



== Player slots miscounted when loading/saving known tiles ==

file #14579:  0005-basic-fix-known-map-in-savegame2.patch
(which is basically the same as the state of play in comment #20)

_Bug:_ if the usage of player slots is sparse, then depending on luck and
alignment, you can see either:
* Segfaults or other misbehaviour, due to indexing off the end of a malloc'd
array.
* The known-tiles status of higher-indexed players is not saved or loaded.
Such players will probably forget all the map they knew on reload. (I don't
know if that has knock-on effects elsewhere, since their player maps will
presumably have an opinion on what the terrain at those tiles is supposed to
be. Can't rule out segfaults/assertion failures.)

With the fix, I think loading from a savefile affected by the bug will
produce the following warning: "Saved game contains incomplete map data. This
can happen with old saved games, or it may indicate an invalid saved game
file. Proceed at your own risk." and carry on. That seems appropriate. Any
knock-on effects will be present in games loaded from these savegames.

Gaps in player slots can appear whenever /remove is used, whether in-game or
pre-game. (Thus, reverting bug #18641 won't hide this bug.)

_Risk/benefit:_ this change is simple and looks low-risk to me; can't see
that it can break things, especially if /remove is not used. It fixes an
observed segfault (comment #21).

*Conclusion:* Apply this "basic" patch before 2.3.1. Cross fingers.



== Divisor issue when saving/loading known tiles ==

This is the stuff in file #14574 that isn't also in file #14579 -- the *8 and
/32 confusion.

_Bug:_ I think what happens is that known information is not saved/loaded for
players with indices 40 and above; instead, zeroes are saved/loaded. (The
arithmetic starts going wrong at index 32, but indices 32..39 are saved/loaded
by luck, in the wrong bit positions; after that all the data is shifted off
the left end.)

This affects any game with >40 players (including barbarians) regardless of
whether /remove is used. It can also affect games with fewer players if
/remove is used (in-game or pre-game).

Fixing it while remaining backwards-compatible with old savefiles will be
tricky. We don't have a patch yet. There's not really any mitigation we can
apply to 2.3.1. It's not a regression from 2.3.0.

*Conclusion:* admit this as a known issue in the 2.3.1 release notes. Blocker
for 2.3.2.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Update of bug #18776 (project freeciv):

  Status:  Ready For Test => In Progress
 Planned Release: 2.3.1,2.4.0 => 2.3.2,2.4.0


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #31, bug #18776 (project freeciv):

The attached two patches should fix the bug, but (see below)

0005-basic-fix-known-map-in-savegame2.patch
- do not use the player_count() but the highest used player slot index
- fast fix to be used in 2.3.1
- it only changes the function used to calculate the number of int values
needed to save the known information to match the highest used player slot
index

0006-fix-bug-in-player_map_free.patch
- fix freeing the player map if a player is removed
- needed to remove the owner information saved in the tile data (used in a
savegame)

the 'but':
- I could not reproduce the bug without the patch, so there is no test if
this is really a fix for it
- removing a player results in strange behaviour of the client (noexistend
units and cities are shown; clicking on a removed city will crash the client)

my proposal:
- deactivate the remove command if the game was started for 2.3.1; this was
changed in bug #18641 for S2_3; it could (will) be checked for 2.3.2 ...

(file #14579, file #14580)
___

Additional Item Attachment:

File name: 0005-basic-fix-known-map-in-savegame2.patch Size:2 KB
File name: 0006-fix-bug-in-player_map_free.patch Size:2 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #30, bug #18776 (project freeciv):

> Last call for 2.3.1 -- I will make the release starting tomorrow
> morning. 
I will create a minimal version which does allow the /remove command to work.
A hint is comment #21:
> It does not crash. And i tested it without your previous
> patches, the ones with vision - while they might have fixed
> something they were not the cause of my segfaults. 
Thus, only a basic version of the fix posted at bug #19007 is needed. I will
test this and commit a version with the smallest possible change.

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Jacob Nevins]

Follow-up Comment #29, bug #18776 (project freeciv):

Last call for 2.3.1 -- I will make the release starting tomorrow morning.

Given the ongoing discussion, it's not clear to me whether the patch(es) here
is/are ready to commit -- if committed now, they would get essentially zero
pre-release testing, so there would have to be a very low risk of regressions
(at least in any functionality other than /remove). I haven't been following
this issue in detail, but comments like "Won't this break existing save
games?" are scaring me. (Please excuse my cluelessness.) So I'm inclined to
punt it to 2.3.2 (or delay the release if it's serious enough, but I'd rather
not...)

It's not immediately obvious which patches are proposed; I'm assuming:
* file #14319: 0034-fix-bug-in-player_map_free.patch
** This doesn't apply cleanly to S2_3. However, the conflict looks to be
trivial, due to patch #2357.
* file #14574: 0011-fix-known-map-in-savegame2.patch
** Is this really meant to be attached to bug #19007, not here?
** This seems to be what the ongoing discussion is about.

I guess any server operator affected by this is probably in a position to
apply the patches and recompile if they need to.

> What does this code save anyway? Map tiles without fog of war? 
> Can't this be recreated after /load?
This one I think I can answer: it's saving the per-player map knowledge,
i.e., whether a tile appears black or not to each player; this isn't something
that can be recreated; obviously. (What the player actually remembers at each
map position is saved elsewhere.)

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #28, bug #18776 (project freeciv):

What does this code save anyway? Map tiles without fog of war? Can't this be
recreated after /load?

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #27, bug #18776 (project freeciv):

p = player_index(pplayer);
l = player_index(pplayer) / 32;
(p - l * 32)

Can't we just use modulo here?

Won't this break existing save games? If i saved using the '8' code, I cannot
load using the '32' code. Well, at least if i have a player index > 31. Then
again, the save game is botched anyway, player 32 overwrites player 24.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #26, bug #18776 (project freeciv):

by Marko Lindqvist:
> I think someone has mixed number of bits and hex values at some
> point. While we read 8 hex values to known[], here we handle it
> as bits already, so that "8" should be "32" (p is always 0 to 31
> bigger than l*32)
I think that was me ... updated patch with fixed numbers attached. I do hope
this one fixes all bugs due to removed players and also saves the known
information correctly. It changes the information in a savegame file.

Please test!


(file #14574)
___

Additional Item Attachment:

File name: 0011-fix-known-map-in-savegame2.patch Size:4 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Re: [Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Marko Lindqvist]

On 15 November 2011 16:39, Michal Mazurek
 wrote:
>
> This part is really perplexing:
> +verbose+
>   2782             known[l * MAP_INDEX_SIZE + tile_index(ptile)]
>   2783               |= (1u << (p - l * 8));
> -verbose-
> Suppose there is a player with index 60. p = 60, l = 1. (p - l * 8) = (60 -
> 8) = 52. 1 << 52 bits is more than an int can handle.

 Comment above says that this is "HACK" so it's even known not to be
very clean implementation...

 I think someone has mixed number of bits and hex values at some
point. While we read 8 hex values to known[], here we handle it as
bits already, so that "8" should be "32" (p is always 0 to 31 bigger
than l*32)


 - ML

___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #25, bug #18776 (project freeciv):

> Why isn't that 1 just shifted by p % 32?
I do not know! I did take the code from the old version, (tried to)
understand it and used it in savegame2.c - and it did work! As you say, this
seems to be at least strange ...

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #24, bug #18776 (project freeciv):

> [bit] = p - floor(p/32) * 8 
> which limits it to values between 0 and 31. 

Does it? p - player index in 0..127. let p = 127.
p - floor(p/32) * 8 = 127 - 3 * 8 = 103.

I run just this part in a test c program. This is really weird.


#include 

int
main()
{
int a;
for (a = 0; a < 128; a++) {
int l = a / 32;
printf("%xn", 1u << (a - l * 8));
}
return 0;
}


I didn't restart a game with > 32 players, with a player with an index > 32
being not an idler. The Finns didn't respond to me on irc, if they restarted
their server or not. But something tells me this code will do strange things
if there are more than 32 players (I still dont understand it). Why isn't that
1 just shifted by p % 32?

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #23, bug #18776 (project freeciv):

> I removed half the players in LTeX (idlers). The memory usage
> dropped by about half. I saved the game then restarted the
> server, and a turn was made - everything is ok.
So the last patch fixed it? If this is the case, should both patches be
applied to 2.3.1 before it is released on the weekend? I added file #14560 as
bug #19007.

Memory saving is mostly due to the freed private/known maps ...

> Still, could you help me understand: 1u << (p - l * 8), which
> really means 1u << (p - p / 4), which is of course 1u << (0.75
> * p). For 126 players, you cannot shift 1 by 94 bits left!

I will try - if it is wrong please correct me!

lines is (now) calculated as the number of int values needed to save the
known information of each tile, i.e (max used player index / 32 + 1).

example: max used player index = 60 => 60/32 + 1 = 1 + 1 = 2

Now the player information has to be saved in the int values. It is done
such, that in int 1, information for player 0 .. 31 is saved, int 2 takes
player 32 to 63 etc. Calculating the bit index within the int is done using
the following equation:

[bit] = p - l * 8

with p = (player index) and l = p / 32; as l is an int the last equation is
equal to l = floor(p/32), i.e. round down. Thus, the equation reads

[bit] = p - floor(p/32) * 8

which limits it to values between 0 and 31.

l is also used to select the correct int value in the calucaltion of the
index for the bitvector known (lines 2836-2837 in savegame2.c of trunk):

known[l * MAP_INDEX_SIZE + tile_index(ptile)] |= (1u << (p - l * 8));



___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #22, bug #18776 (project freeciv):

I removed half the players in LTeX (idlers). The memory usage dropped by
about half. I saved the game then restarted the server, and a turn was made -
everything is ok.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #21, bug #18776 (project freeciv):

It does not crash. And i tested it without your previous patches, the ones
with vision - while they might have fixed something they were not the cause of
my segfaults.

Still, could you help me understand: 1u << (p - l * 8), which really means 
1u << (p - p / 4), which is of course 1u << (0.75 * p). For 126 players, you
cannot shift 1 by 94 bits left!

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #20, bug #18776 (project freeciv):

Thanks for the analyses! Could you test the attached patch? It does not use
player_count() but a new function player_slot_max_used_number() which returns
the highest used player slot index. Thus, there should be enough memory be
allocated.

(file #14560)
___

Additional Item Attachment:

File name: 0002-fix-known-map-in-savegame2.patch Size:2 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #19, bug #18776 (project freeciv):

The allocated table is not always of constant size:
+verbose+
int lines = player_count()/32 + 1;
---
./player.c:640: int player_count(void)
./player.c:641: {
./player.c:642:   return player_slots.used_slots;
./player.c:643: }
+verbose+

Should this be MAX_NUM_PLAYERS?

This part is really perplexing:
+verbose+
   2782 known[l * MAP_INDEX_SIZE + tile_index(ptile)]
   2783   |= (1u << (p - l * 8));
-verbose-
Suppose there is a player with index 60. p = 60, l = 1. (p - l * 8) = (60 -
8) = 52. 1 << 52 bits is more than an int can handle.

I added these lines:
+verbose+
   2779 fprintf(stderr, "save: size of known: %dn", lines *
MAP_INDEX_SIZE * sizeof(*known));
   2780 fprintf(stderr, "save: known index: %dn", l *
MAP_INDEX_SIZE + tile_index(ptile));

---

save: size of known: 336960
save: known index: 86016
Segmentation fault (core dumped)
-verbose-
336960/4 = 84240.

I don't really understand what this code does, especially the SAVE_MAP_CHAR
so I didn't attempt to fix it. Will this code work, if there are two players
with indexes 0 and 125?

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #18, bug #18776 (project freeciv):

any update to the status of this bug? I can not reproduce a segfault with the
patch applied. I will commit it for 2.3.1 if there are no comments.

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #17, bug #18776 (project freeciv):

> Are player indexes updated for existing players, after one from
> the middle is removed?
player_index() calls player_number() which calls player_slot_index(); thus,
the index of a player does not change if one is removed. Furthermore,
player_iterate (it's around the lines you posted) does iterate over _used_
player indices. In your case you could check the value of p (=player index())
before the crash.

One possibility: could you compare the value of player->pslot->player ==
player before creating the savegame (in  player_iterate)? If player->pslot ==
NULL but pslot->player is set it could be the reason for the crash. But what
did I do differently?

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #16, bug #18776 (project freeciv):


   2785 p = player_index(pplayer);
   2786 l = p / 32;
   2787 known[l * MAP_INDEX_SIZE + tile_index(ptile)]
   2788   |= (1u << (p - l * 8));


Are player indexes updated for existing players, after one from the middle is
removed?

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #15, bug #18776 (project freeciv):

I did try to reproduce the error with the savegame. I did removed some
players and set others to AI. It runs a autogame without problems. So there
has to be something else. Di dyou do something else between starting the game,
calling remove and saveing the game? Is the order of the removes important?
Does it work if you remove only one player?

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #14, bug #18776 (project freeciv):

I applied file #14319 and file #14320.

... /removed 15 players ...
> /save removed
Segmentation fault (core dumped)


#0  0x0019925c in sg_save_map_known (saving=0x208c3b000) at
savegame2.c:2787
2787known[l * MAP_INDEX_SIZE + tile_index(ptile)]
(gdb) bt
#0  0x0019925c in sg_save_map_known (saving=0x208c3b000) at
savegame2.c:2787
#1  0x001aa58c in savegame2_save (file=0x205fa0440,
save_reason=Variable "save_reason" is not available.
) at savegame2.c:2162
#2  0x0010ac84 in save_game (orig_filename=Variable "orig_filename"
is not available.
) at srv_main.c:1181
#3  0x0011b330 in handle_stdin_input_real (caller=0x0, str=Variable
"str" is not available.
) at stdinhand.c:725
#4  0x0011c678 in handle_stdin_input (caller=0x0, str=0x203e93120
"/save removed", check=false) at stdinhand.c:3954
#5  0x001ad2fc in handle_readline_input_callback (line=0x208c3c720
"/save removed") at sernet.c:191
#6  0x00020f9eb21c in rl_callback_read_char () at
/usr/src/gnu/lib/libreadline/callback.c:123
#7  0x001ade64 in server_sniff_all_input () at sernet.c:765
#8  0x0010e33c in srv_main () at srv_main.c:2174
#9  0x00103e98 in main (argc=16, argv=0xfffd5a08) at
civserver.c:375
(gdb) bt full
#0  0x0019925c in sg_save_map_known (saving=0x208c3b000) at
savegame2.c:2787
pplayer = Variable "pplayer" is not available.


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #13, bug #18776 (project freeciv):

> found out after my last message that loading such a savegame is
> still not possible (see comment #2). There is more to this bug
> ...
this is fixed in bug #18886

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #12, bug #18776 (project freeciv):

updated patch - remove some duplicated code

(file #14319)
___

Additional Item Attachment:

File name: 0034-fix-bug-in-player_map_free.patch Size:2 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #11, bug #18776 (project freeciv):

found out after my last message that loading such a savegame is still not
possible (see comment #2). There is more to this bug ...

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Update of bug #18776 (project freeciv):

Category:None => general
  Status:None => Ready For Test 
 Assigned to:None => syntron
 Planned Release: 2.3.2,2.4.0 => 2.3.1,2.4.0

___

Follow-up Comment #10:

quite hard to find ... on player remove not only the players map data has to
be deleted but also the knwoledge of all other players about the removed one!

fix attached; also to be used for 2.3.1

please test!

(file #14318)
___

Additional Item Attachment:

File name: 0034-fix-bug-in-player_map_free.patch Size:1 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Jacob Nevins]

Update of bug #18776 (project freeciv):

 Planned Release: => 2.3.2,2.4.0

___

Follow-up Comment #9:

Setting 2.3.2 target, although it can go in 2.3.1 if a working patch is ready
in time.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #8, bug #18776 (project freeciv):

It still crashes, though there were no assertion failures this time. The
crash is at TC, apparently during the /save phase.


#0  0x001991bc in sg_save_map_known (saving=0x20c060c00) at
savegame2.c:2782
2782known[l * MAP_INDEX_SIZE + tile_index(ptile)]
(gdb) bt full
#0  0x001991bc in sg_save_map_known (saving=0x20c060c00) at
savegame2.c:2782
pplayer = Variable "pplayer" is not available.
(gdb) bt
#0  0x001991bc in sg_save_map_known (saving=0x20c060c00) at
savegame2.c:2782
#1  0x001aa36c in savegame2_save (file=0x20e72c080,
save_reason=Variable "save_reason" is not available.
) at savegame2.c:2157
#2  0x0010ac84 in save_game (orig_filename=Variable "orig_filename"
is not available.
) at srv_main.c:1181
#3  0x0010b088 in save_game_auto (save_reason=0x3f24c0 "SIGINT",
reason_filename=0x3f24c8 "interrupted") at srv_main.c:1262
#4  0x00104788 in signal_handler (sig=2) at civserver.c:90
#5  
#6  0x000201b247d4 in sigprocmask () from /usr/lib/libc.so.58.0
#7  0x00020ed14b94 in rl_signal_handler (sig=2) at
/usr/src/gnu/lib/libreadline/signals.c:165
#8  
#9  0x000201a9afec in select () from /usr/lib/libc.so.58.0
#10 0x001ad7a4 in server_sniff_all_input () at sernet.c:661
#11 0x0010e328 in srv_main () at srv_main.c:2174
#12 0x00103e98 in main (argc=16, argv=0x1d88) at
civserver.c:375


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #7, bug #18776 (project freeciv):

updated patch (it compiles; test still needed!)

(file #14287)
___

Additional Item Attachment:

File name: 0040-fix-bug-in-player_map_free.patch Size:1 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Jacob Nevins]

Update of bug #18776 (project freeciv):

 Release: => S2_3   


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Matthias Pfafferodt]

Follow-up Comment #6, bug #18776 (project freeciv):

untested possible patch (not even compile tested) ... could you test it with
your savegame? (first check if it still crashs - then test with a patched
version of the server)

(file #14245)
___

Additional Item Attachment:

File name: 0001-fix-bug-in-player_map_free.patch Size:0 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #5, bug #18776 (project freeciv):

Did anyone have a chance to look at this bug?

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Egor Vyscrebentsov]

Follow-up Comment #4, bug #18776 (project freeciv):

Looks to me like game has borders and tile ownership wasn't cleared when
player was removed.

The only function that (to me) plays with map is player_map_free, and it only
removes vision_site.

BTW, if player had tiles ownership, there should be border recalculating
after removing him, i guess. Don't know at which time.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #3, bug #18776 (project freeciv):

Version is S2.3


#0  0x001ab454 in calc_civ_score (pplayer=0x20d084000) at
score.c:199
199   pcmap->player[player_index(owner)].landarea++;
(gdb) bt full
#0  0x001ab454 in calc_civ_score (pplayer=0x20d084000) at
score.c:199
pcity = Variable "pcity" is not available.
(gdb) bt
#0  0x001ab454 in calc_civ_score (pplayer=0x20d084000) at
score.c:199
#1  0x0010e85c in srv_main () at srv_main.c:833
#2  0x00103e98 in main (argc=16, argv=0x4ad8) at
civserver.c:375


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Re: [Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Egor Vyscrebentsov]

On Sun, 02 Oct 2011 15:07:05 +0200 Michal Mazurek wrote:

> I removed around 15 players using the /remove command. The server crashed. I
> only have a savegame from right before the /removes (which implies the turn
> change did not get to save). Here is what i got:
> 
> 
> #0  0x001ab454 in calc_civ_score (pplayer=0x20e131000) at
> score.c:199
> 199   pcmap->player[player_index(owner)].landarea++;
> 
> Either /remove is faulty, or my newcomer code is.

Version and gdb's 'bt full' might help.

-- 
WBR, Egor Vyscrebentsov

___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #2, bug #18776 (project freeciv):

After loading test.sav.gz:

1: Player shuffle 22 used two times
1: Failure loading savegame!

The savegame cannot be loaded.

Complete log:

[15:17:ttyp9][longturn@spock:~/server/ltex2.3:11]$ sh ltex2.3.sh test.sav.gz 
 
This is the server for Freeciv version 2.3.0+
You can learn a lot about Freeciv at http://www.freeciv.org/
2: Loading rulesets.
2: Ruleset: 'generator' has been set to "Island-based" (ISLAND).
2: Ruleset: 'topology' has been set to "Wrap East-West" and "Wrap
North-South" (WRAPX|WRAPY).
2: Ruleset: 'startpos' has been set to "One player per continent" (SINGLE).
2: Ruleset: 'alltemperate' has been set to enabled.
2: Ruleset: 'separatepoles' has been set to disabled.
2: Ruleset: 'huts' has been set to 0.
2: Ruleset: 'aifill' has been set to 0.
2: Ruleset: 'diplomacy' has been set to "Disabled for everyone" (DISABLED).
2: Ruleset: 'contactturns' has been set to 0.
2: Ruleset: 'revolen' has been set to 2.
2: Ruleset: 'barbarians' has been set to "No barbarians" (DISABLED).
2: Ruleset: 'techpenalty' has been set to 0.
2: Ruleset: 'startunits' has been set to "cccwww".
2: Ruleset: 'specials' has been set to 350.
2: Ruleset: 'borders' has been set to "Disabled" (DISABLED).
2: Savegame: 'size' has been set to 84.
2: Savegame: 'xsize' has been set to 468.
2: Savegame: 'ysize' has been set to 180.
2: Savegame: 'topology' has been set to "Wrap East-West" (WRAPX).
2: Savegame: 'startpos' has been set to "Generator's choice" (DEFAULT).
2: Savegame: 'separatepoles' has been set to enabled.
2: Savegame: 'alltemperate' has been set to disabled.
2: Savegame: 'mapseed' has been set to 1805029910.
2: Savegame: 'gameseed' has been set to 1313942453.
2: Savegame: 'specials' has been set to 140.
2: Savegame: 'maxplayers' has been set to 29.
2: Savegame: 'ec_info' has been set to enabled.
2: Savegame: 'startunits' has been set to "wwxxk".
2: Savegame: 'newcomerunits' has been set to "wxk".
2: Savegame: 'dispersion' has been set to 4.
2: Savegame: 'techpenalty' has been set to 100.
2: Savegame: 'citymindist' has been set to 5.
2: Savegame: 'borders' has been set to "Enabled" (ENABLED).
2: Savegame: 'diplomacy' has been set to "Enabled for everyone" (ALL).
2: Savegame: 'onsetbarbs' has been set to 0.
2: Savegame: 'diplchance' has been set to 50.
2: Savegame: 'spacerace' has been set to disabled.
2: Savegame: 'contactturns' has been set to 20.
2: Savegame: 'allowtake' has been set to "".
2: Savegame: 'timeout' has been set to 82800.
1: Player shuffle 22 used two times
1: Failure loading savegame!
2: AI*1 has been added as Easy level AI-controlled player.
2: AI*2 has been added as Easy level AI-controlled player.
2: AI*3 has been added as Easy level AI-controlled player.
2: AI*4 has been added as Easy level AI-controlled player.
2: AI*5 has been added as Easy level AI-controlled player.
2: Loading script file 'ltex2.3.serv'.
Ruleset directory set to "longturn"
2: Loading rulesets.
2: Ruleset: 'generator' has been set to "Island-based" (ISLAND).
2: Ruleset: 'topology' has been set to "Wrap East-West" and "Wrap
North-South" (WRAPX|WRAPY).
2: Ruleset: 'startpos' has been set to "One player per continent" (SINGLE).
2: Ruleset: 'alltemperate' has been set to enabled.
2: Ruleset: 'separatepoles' has been set to disabled.
2: Ruleset: 'huts' has been set to 0.
2: Ruleset: 'aifill' has been set to 0.
2: Ruleset: 'diplomacy' has been set to "Disabled for everyone" (DISABLED).
2: Ruleset: 'contactturns' has been set to 0.
2: Ruleset: 'revolen' has been set to 2.
2: Ruleset: 'barbarians' has been set to "No barbarians" (DISABLED).
2: Ruleset: 'techpenalty' has been set to 0.
2: Ruleset: 'startunits' has been set to "cccwww".
2: Ruleset: 'specials' has been set to 350.
2: Ruleset: 'borders' has been set to "Disabled" (DISABLED).
2: Removing player AI*5.
2: Removing player AI*4.
2: Removing player AI*3.
2: Removing player AI*2.
2: Removing player AI*1.
Console: 'maxplayers' has been set to 126.
Console: 'saveturns' has been set to 1.
No match for "1".
Console: 'citymindist' has been set to 5.
Console: 'dispersion' has been set to 4.
Console: 'startunits' has been set to "wwxxk".
Console: 'newcomerunits' has been set to "wxk".
Console: 'aifill' has been set to 0.
Console: 'techlevel' has been set to 0.
Console: 'diplchance' has been set to 50.
Undefined argument.  Usage:
set  
No match for "0".
Console: 'timeout' has been set to 82800.
Console: 'revolen' has been set to 2.
Console: 'trading_city' has been set to disabled.
Not enough human players; game will not start.
--
All options with non-default values
--
In the column '##' the status of the option is shown:
 - a '!' means the option is locked by the ruleset.
 - a '+' means you may change the option.
 - a '='

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

Follow-up Comment #1, bug #18776 (project freeciv):

I managed to recreate the crash. Again I removed the players.

... remove 15 players ...
> /remove "alex pancho"
2: Removing player Alex Pancho.
Removed player Alex Pancho from the game.
> /save test
Game saved as /home/longturn/server/ltex2.3//save/test.sav.gz
> /set timeout 120
Console: 'timeout' has been set to 120.
> Segmentation fault (core dumped)

segfault occured about 10 seconds after /set timeout 120.

#0  0x001ab454 in calc_civ_score (pplayer=0x20d084000) at
score.c:199
199   pcmap->player[player_index(owner)].landarea++;

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #18776] /remove perhaps faulty

[Michal Mazurek]

URL:
  

 Summary: /remove perhaps faulty
 Project: Freeciv
Submitted by: akfaew
Submitted on: Sun Oct  2 13:07:04 2011
Category: None
Severity: 3 - Normal
Priority: 5 - Normal
  Status: None
 Assigned to: None
Originator Email: 
 Open/Closed: Open
 Release: 
 Discussion Lock: Any
Operating System: None
 Planned Release: 

___

Details:

I removed around 15 players using the /remove command. The server crashed. I
only have a savegame from right before the /removes (which implies the turn
change did not get to save). Here is what i got:


#0  0x001ab454 in calc_civ_score (pplayer=0x20e131000) at
score.c:199
199   pcmap->player[player_index(owner)].landarea++;

Either /remove is faulty, or my newcomer code is.




___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev