bugFreeciv - Bugs: bug #21126, assertion 'prgbcolor != ((void...

 
 
Show feedback again

bug #21126: assertion 'prgbcolor != ((void *)0)' failed.

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Tue 10 Sep 2013 05:42:01 PM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: Operating System: None
Planned Release: 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 17 Feb 2014 10:03:06 AM UTC, SVN revision 24498:

Correctly assign player colors when loading pre-2.4 savegames, and add
more checks that player colors are assigned when they should be.

Patch by Marko Lindqvist (cazfi@gna) and myself.

See gna bug #21126.

(Browse SVN revision 24498)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 17 Feb 2014 09:56:37 AM UTC, SVN revision 24493:

Correctly assign player colors when loading pre-2.4 savegames, and add
more checks that player colors are assigned when they should be.

Patch by Marko Lindqvist (cazfi@gna) and myself.

See gna bug #21126.

(Browse SVN revision 24493)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 15 Feb 2014 09:25:09 PM UTC, comment #9:

Patches based on cazfi's attached, for trunk/S2_5.

Main change is to conditionalise on was_game_started() rather than S_S_INITIAL, as the former is the right answer in the case of loading a started/"running" game into a fresh server (which will sit in S_S_INITIAL until everyone is ready).

It fixes the original reported issue, although it's now necessary to dig out tileset-demo.sav from S2_3 or thereabouts to reproduce it.

Haven't looked at whether anything needs doing for S2_4 yet.

>> I think the only issue [on S2_4] is in loading then saving a
>> pregame savegame (which is somewhat theoretical)?
> Does it really require loading old game first? Isn't there a
> problem even if one "/create" a player (maybe even having aifill
> != 0 suffice?) and then saves in pregame? Use-case for that
> would be creation of new scenario, one where is no map but all
> the other settings, including players, set. Updating such a
> scenario would be use-case for load + save, of course.

I'm not sure it's currently possible to save a game that's not yet started but with players defined? I've not found a way to do so.

(file #20067, file #20068)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun 26 Jan 2014 12:13:56 AM UTC, comment #8:

Giving to one who knows the code in question.

Marko Lindqvist <cazfi>
Project Administrator
Sat 25 Jan 2014 04:49:19 PM UTC, comment #7:

> perhaps we should put this cleanup back to 2.4.3


Since you made it clear that it's not "obviously correct", we shouldn't risk 2.4.2 with it, true. Even if we deem it more than "cleanup" (sse below), it's not "reggression" nor "high severity" either.

> I think the only issue is in loading then saving a pregame
> savegame (which is somewhat theoretical)?


Does it really require loading old game first? Isn't there a problem even if one "/create" a player (maybe even having aifill != 0 suffice?) and then saves in pregame? Use-case for that would be creation of new scenario, one where is no map but all the other settings, including players, set. Updating such a scenario would be use-case for load + save, of course.

Marko Lindqvist <cazfi>
Project Administrator
Sat 25 Jan 2014 04:05:37 PM UTC, comment #6:

Digging into how I managed to make so many mistakes:

I think I made this much worse on S2_5+ in patch #3443 (nation colours); before that (and hence on current S2_4), server_create_player() assigns a colour if passed NULL outside of pregame (so the comment referred to in comment #2 used to be more correct); afterward, it became the caller's responsibility. My mistakes were (a) not updating the comment (b) not checking all callers thoroughly enough (particularly savegame loading).

  • (Loading many S2_3 games into say S2_5 causes no obvious difficulty, which surprised me. Turns out the colorlessness of players persists indefinitely in the server, but with the default plrcolormode, the client gets the 'virtual' colors from player_get_preferred_color(), so all seems to be well until a save is attempted. Perhaps adding an assertion in package_player_info() based on server state would be a good idea.)

Since S2_4 has different invariants, I think any patch for that branch probably needs to be a different shape.
I think the bug is less severe there, too -- loading 2.3 savegames is fine, so I think the only issue is in loading then saving a pregame savegame (which is somewhat theoretical)? If so, perhaps we should put this cleanup back to 2.4.3?

Also: in the current patch, in sg_save_player_main(), the sense of the "Colorless player outside pregame" test is wrong (generates errors when saving a valid game).

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Fri 24 Jan 2014 06:14:53 PM UTC, comment #5:

Attached patches

- Allow colorless players to be saved in pregame
- Assign players when loading old savegames that have already been started but lack player colors

I don't think there's any reason not to have it in this form in S2_4 despite format freeze - no matter what we do, older revisions from 2.4.x are buggy with savegames without player colors, and such savegames already exist from 2.3.

(file #19841, file #19842)

Marko Lindqvist <cazfi>
Project Administrator
Wed 22 Jan 2014 11:46:37 PM UTC, comment #4:

> Then we need always to assign color to each player as soon as
> (s)he is created, one by one? In that case it might be better
> to just allow colorless players in pregame savegames.

I think the latter. I added colourless players to allow the PLR_SET mode to behave sanely.
I guess I didn't think of (or test) pregame savegames containing players when I was working in this area.

Another place where we might need to expose the notion of colourless players is in the network protocol, for patch #4447.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 22 Jan 2014 04:36:56 PM UTC, comment #3:

Now I wonder if similar thing happens when ever (not only when loading them from saved game) there's players in pregame and one tries to save. Then we need always to assign color to each player as soon as (s)he is created, one by one? In that case it might be better to just allow colorless players in pregame savegames.

Marko Lindqvist <cazfi>
Project Administrator
Tue 21 Jan 2014 09:35:03 PM UTC, comment #2:

> /* If game is already running, server_create_player() will assign
> * a color, else colors will be assigned on game start */


Comment is wrong, by the way. When the savegame had no colour information, server_create_player() will be called with NULL colour so it won't be assigned.

Fix to entire bug attached.

As there's no scensave command in 2.4, the bug is rather theoretical there - AFAICS nothing requires colours before "start". Postponing until after 2.4.2 has been safely released.

(file #19814)

Marko Lindqvist <cazfi>
Project Administrator
Tue 03 Dec 2013 11:47:46 PM UTC, comment #1:

I assume this code in savegame2.c to be relevant:

/* Get player color */
if (!rgbcolor_load(loading->file, &prgbcolor, "player%d.color",
pslot_id)) {
log_verbose("No color defined for player %d.", pslot_id);
/* If game is already running, server_create_player() will assign
* a color, else colors will be assigned on game start */
}

1) Savegame is so old it has no player color information
2) It's not already running game
3) It's not started before we tried to /scensave

Marko Lindqvist <cazfi>
Project Administrator
Tue 10 Sep 2013 05:42:01 PM UTC, original submission:

In S2_5:

> ./fcser -f ../src/data/scenarios/tileset-demo.sav

...

>> scensave tileset-demo


results in a number of failed asserts

in rgbcolor_save() [../src/common/rgbcolor.c::128]: assertion 'prgbcolor != ((void *)0)' failed.
2: Backtrace:
2: 0: ./server/freeciv-server() [0x6081ed]
2: 1: ./server/freeciv-server(vdo_log+0x89) [0x60ba59]
2: 2: ./server/freeciv-server(do_log+0x7d) [0x60bb0d]
2: 3: ./server/freeciv-server(fc_assert_fail+0x9f) [0x60bd3f]
2: 4: ./server/freeciv-server(rgbcolor_save+0x145) [0x5f3fa5]
2: 5: ./server/freeciv-server() [0x4bb029]
2: 6: ./server/freeciv-server() [0x4c97c6]
2: 7: ./server/freeciv-server(savegame2_save+0x65) [0x4cc375]
2: 8: ./server/freeciv-server(save_game+0xe0) [0x435920]
2: 9: ./server/freeciv-server() [0x447cd1]
2: 10: ./server/freeciv-server() [0x4cda00]
2: 11: /lib/x86_64-linux-gnu/libreadline.so.6(rl_callback_read_char+0x11b) [0x7f0f4d0d97fb]
2: 12: ./server/freeciv-server(server_sniff_all_input+0x111b) [0x4cf36b]
2: 13: ./server/freeciv-server(srv_main+0x16d) [0x439d6d]
2: 14: ./server/freeciv-server(main+0x21e) [0x43117e]
2: 15: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7f0f4c5fa995]
2: 16: ./server/freeciv-server() [0x431b8f]

Marko Lindqvist <cazfi>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #20067:  trunk-audit-color-assignment.patch added by jtn (6kB - text/x-patch - trunk/S2_5 r24474)
file #20068:  S2_5-audit-color-assignment.patch added by jtn (6kB - text/x-patch - trunk/S2_5 r24474)

 

Depends on the following items: None found

Digest:
   task dependencies, patch dependencies.

 

Carbon-Copy List
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (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 18 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 17 Feb 2014 10:08:07 AM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sat 15 Feb 2014 09:25:09 PM UTCjtnAttached File-=>Added trunk-audit-color-assignment.patch, #20067
      Attached File-=>Added S2_5-audit-color-assignment.patch, #20068
      StatusNone=>Ready For Test
      Planned Release2.4.3, 2.5.0, 2.6.0=>2.5.0, 2.6.0
    Sun 26 Jan 2014 12:13:56 AM UTCcazfiStatusReady For Test=>None
      Assigned toNone=>jtn
    Sat 25 Jan 2014 04:49:19 PM UTCcazfiPlanned Release2.4.2, 2.5.0, 2.6.0=>2.4.3, 2.5.0, 2.6.0
    Fri 24 Jan 2014 06:15:58 PM UTCcazfiPlanned Release2.4.3, 2.5.0, 2.6.0=>2.4.2, 2.5.0, 2.6.0
    Fri 24 Jan 2014 06:14:53 PM UTCcazfiAttached File-=>Added AllowColorlessPregame.patch, #19841
      Attached File-=>Added AllowColorlessPregame-S2_5.patch, #19842
      StatusIn Progress=>Ready For Test
    Thu 23 Jan 2014 04:43:32 AM UTCcazfiStatusReady For Test=>In Progress
    Tue 21 Jan 2014 09:35:03 PM UTCcazfiAttached File-=>Added AssignSavegamePlrColours.patch, #19814
      CategoryNone=>general
      StatusNone=>Ready For Test
      Planned Release=>2.4.3, 2.5.0, 2.6.0
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup