bugFreeciv - Bugs: bug #21384, Client segfault in...

 
 
Show feedback again

bug #21384: Client segfault in fill_grid_sprite_array on server shutdown or player /remove

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sat 28 Dec 2013 02:17:11 PM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: S2_4 r23906Operating System: Any
Planned Release: 2.4.2,2.5.0,2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 04 Feb 2014 10:33:23 AM UTC, comment #10:

> What kind of implications this has on 'foggedborders' setting?

The bug, or the fix?

I think the crash occurred regardless of foggedborders.

As regards the fixed version, this bit of comment #3 describes the main quirk that can happen with foggedborders enabled:

> if the [dead] player slot ever ends up reused to make room for a
> new player, as in create_command_newcomer(), then we will need
> to remove the dead players' info from other player maps at that
> time, which will look a little odd -- information will disappear
> from your map for no apparent reason -- but probably won't occur
> much in practice

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 03 Feb 2014 07:32:17 PM UTC, comment #9:

What kind of implications this has on 'foggedborders' setting?

Marko Lindqvist <cazfi>
Project Administrator
Sat 04 Jan 2014 11:42:50 PM UTC, comment #8:

Postscript: cazfi actually spotted this problem nearly two years ago. From bug #18588 comment 1:

> I suspect that there's similar problem when tile owner is
> removed from game. Remaining players may still have him as
> owner of the tile in their player map. Fixing this needs more
> investigation. Setting tile owner to NULL is part of player
> map removal, and it might be bad idea to simply call tile
> knowledge update for all players when one player map is in
> such a state.

I don't think my solution trips over the issue described...

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 31 Dec 2013 09:41:28 AM UTC, SVN revision 23953:

When a player is removed, remove all references to it from other
players' private maps, and ensure maps are pushed to clients before
player removal packet. Fixes client crash when a player is removed (or,
occasionally, when server quits game).

See gna bug #21384.

(Browse SVN revision 23953)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 31 Dec 2013 09:38:27 AM UTC, SVN revision 23947:

When a player is removed, remove all references to it from other
players' private maps, and ensure maps are pushed to clients before
player removal packet. Fixes client crash when a player is removed (or,
occasionally, when server quits game).

See gna bug #21384.

(Browse SVN revision 23947)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 31 Dec 2013 09:36:41 AM UTC, SVN revision 23941:

When a player is removed, remove all references to it from other
players' private maps, and ensure maps are pushed to clients before
player removal packet. Fixes client crash when a player is removed (or,
occasionally, when server quits game).

See gna bug #21384.

(Browse SVN revision 23941)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun 29 Dec 2013 04:44:41 PM UTC, comment #4:

Patches which clear up other player's maps when a player is removed. I haven't managed to reproduce a crash after applying these patches.

(file #19557, file #19558)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 28 Dec 2013 11:36:54 PM UTC, comment #3:

> And from backtrace:
>> timer_callback()


...although I think I have seen a backtrace that doesn't involve that (I don't have it to hand now, but see below).

Reducing the focus circle update time in get_focus_unit_toggle_timeout() from 0.1 to 0.02 seemed to make this more reproducible, to the point where I got a plausibly-related symptom under valgrind:

...followed by more similar messages. Which I think confirms that it's due to player destruction.

I'm guessing that the problem is that knowledge of a removed player can continue indefinitely in other players' private maps. player_map_free() on the server (oddly) removes knowledge of the removed player's cities from other players (although I'm not sure it pushes this information to clients), but I don't see anything to clear down tile owners (possibly this was missed when foggedborders was added in patch #1258).

And indeed, a more targeted test where I "/remove" a player does cause the client of another player to crash sooner or later. Here's a crash not involving a timer:

If I'm right about the cause, probably the thing to do is to ensure that all owners and cities are cleared from other players' maps when a player is removed, and that these updates are pushed to clients in a timely fashion so that they happen before the client does handle_player_remove().

(Note that this only affects outright player removal -- at game end, via /remove, or in the editor -- and not players dying during gameplay due to e.g. losing a GameLoss unit, who will hang around as dead players. In that case we would ideally like information about dead players to remain in other players' maps -- with the introduction of gameloss_style (bug #20577), it's no longer guaranteed that players' cities will have disappeared when news of the player's demise reaches us, they might now be under new management. However, if the player slot ever ends up reused to make room for a new player, as in create_command_newcomer(), then we will need to remove the dead players' info from other player maps at that time, which will look a little odd -- information will disappear from your map for no apparent reason -- but probably won't occur much in practice. The alternative, remembering player colours etc for an unbounded number of dead players, is probably not worth the effort.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 28 Dec 2013 07:40:39 PM UTC, comment #2:

> it seemed to occur intermittently


And from backtrace:

> timer_callback()


So it seems like unlucky semi-parallel execution, timer callback is executed while some state is temporarily not consistent.

Marko Lindqvist <cazfi>
Project Administrator
Sat 28 Dec 2013 07:06:42 PM UTC, comment #1:

tilespec.c:4372 is retrieving a player-specific border sprite to draw a border edge. I'm guessing (but haven't checked) that that player has been freed by now.

I wonder if this is related to something I said in comment 7 on bug #21116:

> I'm a bit worried by handle_player_remove() calling
> tileset_player_free() when not all of the player's cities/units
> might yet have been removed from the map, but probably not
> relevant to this ticket.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 28 Dec 2013 02:17:11 PM UTC, original submission:

I've been following the progress of an autogame by starting a separate server with autogame savefiles, connecting a client to it, and then using the editor to switch between global observer and AI players.

Sometimes, when I quit the server with the client still attached, the client segfaults. In at least this last case, the client was connected as one of the AI players (Oscar II) at the time of quit, not a global observer.

Savefile attached, but it seemed to occur intermittently on many savefiles from this autogame.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #19557:  trunk-remove-player-map.patch added by jtn (5kB - text/x-diff - trunk/S2_5/S2_4 r23912)
file #19558:  S2_5-S2_4-remove-player-map.patch added by jtn (5kB - text/x-diff - trunk/S2_5/S2_4 r23912)
file #19534:  autogame-T0690-Y02252-auto.sav.bz2 added by jtn (61kB - application/x-bzip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by jtn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 31 Dec 2013 09:45:24 AM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 29 Dec 2013 04:44:41 PM UTCjtnAttached File-=>Added trunk-remove-player-map.patch, #19557
      Attached File-=>Added S2_5-S2_4-remove-player-map.patch, #19558
      Categoryclient-gtk-2.0=>general
      StatusNone=>Ready For Test
      Assigned toNone=>jtn
      Operating SystemGNU/Linux=>Any
      Planned Release2.4.2=>2.4.2,2.5.0,2.6.0
      SummaryClient segfault in fill_grid_sprite_array on server shutdown=>Client segfault in fill_grid_sprite_array on server shutdown or player /remove
    Sat 28 Dec 2013 02:17:11 PM UTCjtnAttached File-=>Added autogame-T0690-Y02252-auto.sav.bz2, #19534
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup