bugFreeciv - Bugs: bug #21349, Handicaps pointer rather than...

 
 
Show feedback again

bug #21349: Handicaps pointer rather than contents copied on civil war => double free()

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sat 14 Dec 2013 04:03:50 PM UTC  
 
Category: aiSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: trunk r23854Operating 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)

Mon 30 Dec 2013 03:09:06 PM UTC, SVN revision 23934:

Don't copy handicaps when splitting player -- this is unnecessary (but
harmless) as it will shortly be overwritten.

Fix suggested by Marko Lindqvist (cazfi@gna).

See gna bug #21349.

(Browse SVN revision 23934)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 30 Dec 2013 03:06:51 PM UTC, SVN revision 23931:

Don't copy handicaps when splitting player -- this is unnecessary (but
harmless) as it will shortly be overwritten.

Fix suggested by Marko Lindqvist (cazfi@gna).

See gna bug #21349.

(Browse SVN revision 23931)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 30 Dec 2013 03:04:34 PM UTC, SVN revision 23927:

Don't copy handicaps pointer when splitting player -- causes double-free
and other trouble.

Fix suggested by Marko Lindqvist (cazfi@gna).

See gna bug #21349.

(Browse SVN revision 23927)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun 29 Dec 2013 02:05:55 AM UTC, comment #5:

Updated patch based on discussion.

(file #19539)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 14 Dec 2013 05:14:51 PM UTC, comment #4:

> I have not tested, but I think it's as simple as leaving that
> line out completely as call to set_ai_level_direct() a couple of
> lines later should do the right thing

Looks plausible (but I haven't tested either)...

> (and yes, actually it should override what that erronous line
> is doing so we don't have end-user visible bug at the moment)

...unfortunately we do have a bug on trunk, I think, as it will change the handicaps of both players. (I agree this saves S2_5 and S2_4.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 14 Dec 2013 05:06:35 PM UTC, comment #3:

> Indeed, this wouldn't be appropriate if you wanted a different > AI type for civil-war players.


Not only if you want different AI type (actually that does not make difference at the moment as handicap setting is not yet AI type dependant), but even if you have different difficulty level between default AI type players, i.e. if player has level different from what gets assigned by default to created player.

> I'll leave this to someone else (or me if I ever get round to
> it).


I have not tested, but I think it's as simple as leaving that line out completely as call to set_ai_level_direct() a couple of lines later should do the right thing (and yes, actually it should override what that erronous line is doing so we don't have end-user visible bug at the moment)

Marko Lindqvist <cazfi>
Project Administrator
Sat 14 Dec 2013 04:26:21 PM UTC, comment #2:

Hm, I wasn't really thinking about the wider context. Indeed, this wouldn't be appropriate if you wanted a different AI type for civil-war players.
I'll leave this to someone else (or me if I ever get round to it).

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 14 Dec 2013 04:20:29 PM UTC, comment #1:

Why are handicaps copied there at all? Shouldn't it be function of ai difficulty level? Given that difficulty level of the created player can be different from original one, this even sounds like a bug.

Marko Lindqvist <cazfi>
Project Administrator
Sat 14 Dec 2013 04:03:50 PM UTC, original submission:

In split_player(), we have

handicaps is a void* allocated by handicaps_init() and freed by handicaps_close(), so should be copied deeply rather than shallowly. (Caused by patch #4197, I think.)

This manifested as an invalid free() on server shutdown. Presumably also a tiny memory leak.

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 #19539:  trunk-S2_5-S2_4-handicaps-split-player.patch added by jtn (866B - text/x-diff - trunk/S2_5/S2_4 r23909)
file #19475:  trunk-handicaps-copy.patch added by jtn (2kB - text/x-diff - trunk r23854)

 

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 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 30 Dec 2013 03:12:40 PM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 29 Dec 2013 02:05:55 AM UTCjtnAttached File-=>Added trunk-S2_5-S2_4-handicaps-split-player.patch, #19539
      StatusNone=>Ready For Test
      Assigned toNone=>jtn
      Planned Release2.6.0=>2.4.2,2.5.0,2.6.0
    Sat 14 Dec 2013 04:26:21 PM UTCjtnStatusReady For Test=>None
      Assigned tojtn=>None
    Sat 14 Dec 2013 04:14:33 PM UTCjtnAttached File-=>Added trunk-handicaps-copy.patch, #19475
      StatusIn Progress=>Ready For Test
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup