bugFreeciv - Bugs: bug #19800, Server crash after reading...

 
 
Show feedback again

bug #19800: Server crash after reading "multiplayer.serv" or "civ2.serv"

Submitted by:  pepeto <pepeto>
Submitted on:  Tue 12 Jun 2012 10:36:30 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: S2_3Operating System: GNU/Linux
Planned Release: 2.3.3, 2.4.0, 2.5.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)

Sun 07 Oct 2012 07:44:42 PM UTC, SVN revision 21891:

Reallocate space for advisor government want values when their number may
change on load of new ruleset.

Reported by pepeto

Patch by me after investigation and suggestion by Jacob Nevins

See gna bug #19800

(Browse SVN revision 21891)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Oct 2012 07:44:37 PM UTC, SVN revision 21890:

Reallocate space for advisor government want values when their number may
change on load of new ruleset.

Reported by pepeto

Patch by me after investigation and suggestion by Jacob Nevins

See gna bug #19800

(Browse SVN revision 21890)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Oct 2012 07:44:32 PM UTC, SVN revision 21889:

Reallocate space for advisor government want values when their number may
change on load of new ruleset.

Reported by pepeto

Patch by me after investigation and suggestion by Jacob Nevins

See gna bug #19800

(Browse SVN revision 21889)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 05 Oct 2012 03:07:27 AM UTC, comment #5:

Patches

(file #16636, file #16637)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 15 Jun 2012 07:54:13 PM UTC, comment #4:

The calls to ai_data_init() and ai_data_close() look balanced.

However, the second significant valgrind error ("Invalid write of size 4") is relevant, I think.

When ai_data_init() is called for a player, it allocates ai->government_want according to the government_count() in force at the time.

It looks like ai_data_init() is only called when a player is first created. I'm guessing the problem is that if a player exists over a ruleset reload, that player's government_want remains sized for the old ruleset.

It looks like ai_data_default() is called to reinitialise the player on ruleset reload, but that doesn't reallocate ai->government_want. However it does memset it, based on the new government_count().

Since the civ2 and multiplayer rulesets have an extra government compared to the classic ruleset (Fundamentalism), that memset will overrun.

Is the fix simply to reallocate government_want in ai_data_default() rather than ai_data_init()?
I'll leave this for someone else...

(The other valgrind error looks unrelated -- although player data applicable to long-gone rulesets is also implicated -- so I've raised it as bug #19814.)

Jacob Nevins <jtn>
Project Administrator
Fri 15 Jun 2012 10:45:39 AM UTC, comment #3:

Revision: 21191 ; This is valgrind backtrace :

pepeto <pepeto>
Project Member
Fri 15 Jun 2012 12:06:31 AM UTC, comment #2:

> I can't trivially reproduce this


As this seems to be about ai->government_want being non-NULL illegal pointer it could be simply that it's not initialized. 1) To avoid compiler setting it to NULL you need to compile with optimization. 2) You must be lucky that the uninitialized value is something that does not just happen to pass.

Pepeto: Can you add logging to ai_data_init() and ai_data_close() with player names to check that they are always called in pairs (ai_data_init() must be called before ai_data_close() and ai_data_close() should not be called multiple times)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 14 Jun 2012 11:40:12 PM UTC, comment #1:

Hey pepeto, long time no see...

I can't trivially reproduce this with head of S2_3 (r21191), but then I don't think the line numbers in the backtrace match up with that. Which revision was this?

We fixed several player-removal bugs recently, but none strikes me as obviously relevant.

Jacob Nevins <jtn>
Project Administrator
Tue 12 Jun 2012 10:36:30 AM UTC, original submission:

I get a crash when quitting the server after having loaded "multiplayer" or "civ2" rulesets at first turn.

pepeto <pepeto>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #16636:  GovCountChange.patch added by cazfi (973B - text/x-patch)
file #16637:  GovCountChange-S2_3.patch added by cazfi (818B - text/x-patch)

 

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 (Posted a comment)
  • -unavailable- added by pepeto (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 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Oct 2012 07:44:58 PM UTCcazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Fri 05 Oct 2012 03:07:27 AM UTCcazfiAttached File-=>Added GovCountChange.patch, #16636
      Attached File-=>Added GovCountChange-S2_3.patch, #16637
      StatusNone=>Ready For Test
    Mon 25 Jun 2012 06:16:00 PM UTCcazfiPlanned Release=>2.3.3, 2.4.0, 2.5.0
    Fri 15 Jun 2012 07:54:13 PM UTCjtnSummaryServer crash after reading "multiplayer.serv" or "civ.serv"=>Server crash after reading "multiplayer.serv" or "civ2.serv"
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup