Re: [Freeciv-Dev] (PR#40303) [Patch] Fix Global Observer crash in connectdlg
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 2008/6/22 Jason Dorje Short: I'd suggest using valgrind to find the exact line where the error occurs here and to make sure the fix actually fixes and doesn't just hide the error. Valgrind can show only the point where actual illegal memory access is made, which in this case caused segfault. It cannot tell when the variable gets illegal pointer assigned. Traditional debugging shows that player_ptr is NULL when entering game_renumber_players() and gets set there. Patch fixes that. Only possible 'hidden' error is that player_idx is not set to special flag value like -1. - ML ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40303) [Patch] Fix Global Observer crash in connectdlg
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303
Marko Lindqvist wrote:
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303
$subject
To be committed soon.
if (game.player_ptr) {
char *text;
-if (game.player_ptr-is_ready) {
+if (game.player_ptr == NULL) {
+ text = _(_Ready);
+} else if (game.player_ptr-is_ready) {
text = _(Not _ready);
} else {
int num_unready = 0;
This patch makes no sense.
How can game.player_ptr be NULL when 3 lines above there's a check to
see if it's non-NULL?
And why is the text Not ready when the is_ready field is set?
-jason
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40303) [Patch] Fix Global Observer crash in connectdlg
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303
Maybe this patch makes more sense.
Crash was caused by adjusting player_idx, and setting bogus
player_ptr, for global observer in game_renumber_players.
S2_1 version:
- Fixes the bug
- Adds sanity check for player number received in player_remove_player
- Reverts my earlier patch
There is no game.player_ptr or packet_remove_player in S2_2 and
TRUNK. This patch only
- Revert my earlier patch, but uses proper accessor functions instead
of old implementation
- ML
diff -Nurd -X.diff_ignore freeciv/client/gui-gtk-2.0/gui_main.c
freeciv/client/gui-gtk-2.0/gui_main.c
--- freeciv/client/gui-gtk-2.0/gui_main.c 2008-06-22 10:07:27.0
+0300
+++ freeciv/client/gui-gtk-2.0/gui_main.c 2008-06-22 11:21:52.0
+0300
@@ -1487,9 +1487,7 @@
if (game.player_ptr) {
char *text;
-if (game.player_ptr == NULL) {
- text = _(_Ready);
-} else if (game.player_ptr-is_ready) {
+if (game.player_ptr-is_ready) {
text = _(Not _ready);
} else {
int num_unready = 0;
diff -Nurd -X.diff_ignore freeciv/client/packhand.c freeciv/client/packhand.c
--- freeciv/client/packhand.c 2008-05-16 01:50:17.0 +0300
+++ freeciv/client/packhand.c 2008-06-22 11:02:53.0 +0300
@@ -2178,6 +2178,12 @@
**/
void handle_player_remove(int player_id)
{
+ if (player_id 0 || player_id = MAX_NUM_PLAYERS) {
+freelog(LOG_ERROR, Illegal player_remove packet from server (%d),
+player_id);
+return;
+ }
+
client_remove_player(player_id);
update_conn_list_dialog();
}
diff -Nurd -X.diff_ignore freeciv/common/game.c freeciv/common/game.c
--- freeciv/common/game.c 2008-05-08 00:25:01.0 +0300
+++ freeciv/common/game.c 2008-06-22 11:21:01.0 +0300
@@ -534,7 +534,7 @@
assert(city_list_size(game.players[i].cities) == 0);
}
- if(game.info.player_idx plrno) {
+ if(game.player_ptr game.info.player_idx plrno) {
game.info.player_idx--;
game.player_ptr = game.players[game.info.player_idx];
}
diff -Nurd -X.diff_ignore freeciv/client/gui-gtk-2.0/gui_main.c
freeciv/client/gui-gtk-2.0/gui_main.c
--- freeciv/client/gui-gtk-2.0/gui_main.c 2008-06-22 10:07:44.0
+0300
+++ freeciv/client/gui-gtk-2.0/gui_main.c 2008-06-22 11:24:47.0
+0300
@@ -1559,12 +1559,10 @@
{
GtkTreeIter it[player_count()];
- if (NULL != client.conn.playing) {
+ if (!client_has_player()) {
char *text;
-if (!client_has_player()) {
- text = _(_Ready);
-} else if (client.conn.playing-is_ready) {
+if (client_player()-is_ready) {
text = _(Not _ready);
} else {
int num_unready = 0;
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40303) [Patch] Fix Global Observer crash in connectdlg
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 How do you trigger this crash? -jason ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40303) [Patch] Fix Global Observer crash in connectdlg
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 2008/6/22 Jason Dorje Short [EMAIL PROTECTED]: URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 How do you trigger this crash? In theory: 1) Start New Game 2) Observe (become global observer) 3) Pick Nation ( /take - ) In practice, crash has happened more consistently after: 1) Start New Game 2) Observe 3) Do not Observe 4) Observe 5) Pick Nation Server is probably removing aifill-player to make space for me when the crash happens. - ML ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40303) [Patch] Fix Global Observer crash in connectdlg
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 Marko Lindqvist wrote: URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 2008/6/22 Jason Dorje Short [EMAIL PROTECTED]: URL: http://bugs.freeciv.org/Ticket/Display.html?id=40303 How do you trigger this crash? In theory: 1) Start New Game 2) Observe (become global observer) 3) Pick Nation ( /take - ) In practice, crash has happened more consistently after: 1) Start New Game 2) Observe 3) Do not Observe 4) Observe 5) Pick Nation Server is probably removing aifill-player to make space for me when the crash happens. I'd suggest using valgrind to find the exact line where the error occurs here and to make sure the fix actually fixes and doesn't just hide the error. -jason ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
