[Freeciv-Dev] Memory handling bugs in civserver (at least 1.13.0)

[Sami Liedes <smliedes@xxxxxxxxxxxxxxxxxxxx>]

Hello,

I just finished a game of freeciv with civserver running on the valgrind 
memory debugger and took a look at some memory handling bugs. I'm 
feeling too lazy right now to replay on the CVS version (sorry about 
that), especially since some of them only occurred halfway through the 
game. So instead of mass-filing bug reports I decided to send some 
information to this list, as there probably are people here who know 
what's been fixed and what not.

Some of these are not very obvious bugs (like sending uninitialized 
values over the network somewhere where that doesn't matter much), but 
not very beautiful anyway. Some bugs are more obvious, however. Some 
seem fairly trivial, and then there are a couple of bugs I don't 
understand very well (I'm not very familiar with the code). Also I only 
played alone (against AI), so e.g. meeting code and such didn't get 
tested at all.

Based on my recent experiences on valgrind, I can recommend it to anyone 
developing in C or C++ for x86 Linux (it's free software, GPL). It 
helps to have a fairly fast computer with plenty of memory, though - 
the game was barely playable when the server was running in valgrind on 
a 1200 MHz Duron.

If this report is of any help, I might also take a look at the client 
some day. This time I promise to use the latest version from CVS :-)

Attached below is a list of the issues I found. Hope at least some of 
these are real bugs, so I didn't bother you for nothing ;)

        Sami


In server/citytools.c::create_city(), pcity->tile_trade is not 
initialized. This value is later used in 
common/city.c::generic_city_refresh().

========================================

In server/gamehand.c::send_game_info(), ginfo.diplcost, ginfo.freecost 
and ginfo.conquercost go unitialized; also, if game.timeout==0, 
ginfo.seconds_to_turndone is uninitialized (so basically garbage is 
sent over the network and discarded).

========================================

In server/report.c::page_conn_etype(), genmsg.x and genmsg.y are not 
initialized, yet genmsg.x is later used in 
common/packets.c::send_packet_generic_message() in "if (packet->x == 
1)".

========================================

In server/settlers.c::contemplate_new_city(), gx is still sometimes 
uninitialized at the "if (gx == -1)" statement. This can be verified by 
initializing gx to e.g. 54321 and inserting a check for this invalid 
value just before the if statement. This happened probably a few dozen 
times during a game.

========================================

In server/settlers.c::road_bonus(), uninitialized values are sometimes 
used in some mysterious if statement (please just ignore the first two 
frames, they're from valgrind):

(gdb) bt
#0  vg_do_syscall3 (syscallno=20, arg1=3, arg2=5, arg3=3221222380)
    at vg_mylibc.c:91
#1  0x00003516 in ?? ()
#2  0x080a725b in road_bonus (x=44, y=49, spc=2) at settlers.c:658
#3  0x080a89c7 in evaluate_improvements (punit=0xbffff5bc,
    best_act=0xbffff5b4, gx=0xbffff5b0, gy=0xbffff5ac) at 
settlers.c:1112
#4  0x080aa568 in contemplate_terrain_improvements (pcity=0x433d7a48)
    at settlers.c:1576
#5  0x080e0b3c in ai_manage_cities (pplayer=0x8171148) at aicity.c:507
#6  0x080e3a02 in ai_do_last_activities (pplayer=0x8171148) at 
aihand.c:370
#7  0x08087a02 in update_player_activities (pplayer=0x8171148) at 
plrhand.c:172
#8  0x0804e68e in end_turn () at srv_main.c:421
#9  0x0805155d in main_loop () at srv_main.c:1722
#10 0x08051def in srv_main () at srv_main.c:1970
#11 0x08049f9c in main (argc=3, argv=0xbffff804) at civserver.c:153
(gdb) fra 2
#2  0x080a725b in road_bonus (x=44, y=49, spc=2) at settlers.c:658
658       if (rd[1] && !rd[4] && !rd[3] && (!te[5] || !te[6] || !te[7])) 
m++;
(gdb) print rd
$1 = {0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}
(gdb) print te
$2 = {1, 0, 1, 0, 0, 49, 44, 1, 1, 0, 0, -1073744724}

The same seems to apply to the rest of this bunch of ifs.

========================================

In ai/aiunit.c::look_for_charge(), 
ai/advmilitary.c::assess_defense_quadratic() and 
ai/aitools.c::ai_advisor_choose_building(), some structures seem to be 
uninitialized where they probably shouldn't. Looks like the same bug is 
causing all of these:

(gdb) bt
#0  vg_do_syscall3 (syscallno=109, arg1=1073821132, arg2=1, arg3=42)
    at vg_mylibc.c:91
#1  0x000037b0 in ?? ()
#2  0x080e9702 in look_for_charge (pplayer=0x816dfc4, punit=0x41103db8,
    aunit=0xbffff630, acity=0xbffff634) at aiunit.c:1384
#3  0x080e9c96 in ai_military_findjob (pplayer=0x816dfc4, 
punit=0x41103db8)
    at aiunit.c:1485
#4  0x080ecb9f in ai_manage_military (pplayer=0x816dfc4, 
punit=0x41103db8)
    at aiunit.c:2168
#5  0x080ed1d5 in ai_manage_unit (pplayer=0x816dfc4, punit=0x41103db8)
    at aiunit.c:2304
#6  0x080ed464 in ai_manage_units (pplayer=0x816dfc4) at aiunit.c:2356
#7  0x080e39ea in ai_do_first_activities (pplayer=0x816dfc4) at 
aihand.c:362
#8  0x0804e4df in ai_start_turn () at srv_main.c:368
#9  0x080514f0 in main_loop () at srv_main.c:1718
#10 0x08051def in srv_main () at srv_main.c:1970
#11 0x08049f9c in main (argc=3, argv=0xbffff804) at civserver.c:153
(gdb) fra 2
#2  0x080e9702 in look_for_charge (pplayer=0x816dfc4, punit=0x41103db8,
    aunit=0xbffff630, acity=0xbffff634) at aiunit.c:1384
1384        if (mycity->ai.urgency == 0) continue;
(gdb) print mycity->ai.urgency
$1 = 540028960
(gdb) print mycity->ai
$4 = {workremain = 1, ai_role = 0, building_want = {0 <repeats 200 
times>},
  danger = -1, diplomat_threat = 1130804828, urgency = 540028960,
  grave_danger = 807415856, wallvalue = 808460336, trade_want = 12, 
choice = {
    choice = -435, want = -15, type = 0}, downtown = 0,
  distance_to_wonder_city = 808460336, detox = {{12320, -1, -1, -1, 
8240}, {
      -1, -1, -1, -1, -1}, {-1, -1, -1, -1, -1}, {-1, -1, -1, -1, -1}, 
{-450,
      -1, -1, -1, 0}}, derad = {{0, -1, -1, -1, 13670}, {-1, -1, -1, -1, 
-1}, {
      -1, -1, -1, -1, -1}, {-1, -1, -1, -1, -1}, {26144, -1, -1, -1, 
8294}},
  mine = {{-465, -1, -1, 34, 0}, {-1, -1, -1, -1, 76}, {-1, -1, 161, -1, 
76}, {
      -1, -1, 34, -1, -1}, {-18756, -1, -1, -1, 12336}}, irrigate = 
{{12320,
      67, -1, -1, 8240}, {-1, -1, 67, -1, -1}, {-1, 42, -1, -1, -1}, 
{42, -1,
      -1, 42, -1}, {12592, -1, -1, 42, 16658}}, road = {{-18720, 66, -1, 
17,
      12592}, {-1, -1, 66, -1, 25}, {-1, 59, -1, 66, 25}, {41, 41, 17, 
59,
      66}, {12320, 74, 41, 59, 8294}}, railroad = {{26214, -1, -1, -1, 
16658},
    {-1, -1, -1, -1, -1}, {-1, -1, -1, -1, -1}, {-1, -1, -1, -1, -1}, 
{0, -1,
      -1, -1, 12336}}, transform = {{12320, -1, -1, 25, 8240}, {-1, -1, 
-1,
      -1, -1}, {-1, -1, -1, -1, -1}, {42, 42, 25, -1, -1}, {-525, -1, 
42, -1,
      0}}, tile_value = {{0, 0, 0, 8240, 12592}, {12320, 8240, 12592, 
12320,
      8240}, {12592, 20688, 16658, 20568, 16658}, {-18604, 17254, 12320, 
8248,
      12592}, {25888, 8241, 13670, 12320, 8245}}, settler_want = -540,
  founder_want = -15, a = 0, f = 0, invasion = 0}

=====

(gdb) bt
#0  vg_do_syscall3 (syscallno=1091583416, arg1=5, arg2=3221222796,
    arg3=134962266) at vg_mylibc.c:91
#1  0x0000380c in ?? ()
#2  0x080daf2a in assess_defense_quadratic (pcity=0x41124860)
    at advmilitary.c:82
#3  0x080e9734 in look_for_charge (pplayer=0x816dfc4, punit=0x41103db8,
    aunit=0xbffff630, acity=0xbffff634) at aiunit.c:1386
#4  0x080e9c96 in ai_military_findjob (pplayer=0x816dfc4, 
punit=0x41103db8)
    at aiunit.c:1485
#5  0x080ecb9f in ai_manage_military (pplayer=0x816dfc4, 
punit=0x41103db8)
    at aiunit.c:2168
#6  0x080ed1d5 in ai_manage_unit (pplayer=0x816dfc4, punit=0x41103db8)
    at aiunit.c:2304
#7  0x080ed464 in ai_manage_units (pplayer=0x816dfc4) at aiunit.c:2356
#8  0x080e39ea in ai_do_first_activities (pplayer=0x816dfc4) at 
aihand.c:362
#9  0x0804e4df in ai_start_turn () at srv_main.c:368
#10 0x080514f0 in main_loop () at srv_main.c:1718
#11 0x08051def in srv_main () at srv_main.c:1970
#12 0x08049f9c in main (argc=3, argv=0xbffff804) at civserver.c:153
(gdb) fra 2
#2  0x080daf2a in assess_defense_quadratic (pcity=0x41124860)
    at advmilitary.c:82
82        for (l = 0; l * l < pcity->ai.wallvalue * 10; l++) {
(gdb) print pcity->ai
[output snipped, identical or very similar to the previous case]

=====

(gdb) bt
#0  vg_do_syscall3 (syscallno=1, arg1=90, arg2=68, arg3=72) at 
vg_mylibc.c:91
#1  0x00003877 in ?? ()
#2  0x080e4ed6 in ai_advisor_choose_building (pcity=0x411c08a4,
    choice=0xbffff540) at aitools.c:148
#3  0x0806ecf5 in advisor_choose_build (pplayer=0x8167cbc, 
pcity=0x411c08a4)
    at cityturn.c:621
#4  0x0806fd12 in city_build_building (pplayer=0x8167cbc, 
pcity=0x411c08a4)
    at cityturn.c:1017
#5  0x0807007c in city_build_stuff (pplayer=0x8167cbc, pcity=0x411c08a4)
    at cityturn.c:1096
#6  0x08070832 in update_city_activity (pplayer=0x8167cbc, 
pcity=0x411c08a4)
    at cityturn.c:1268
#7  0x0806e2a1 in update_city_activities (pplayer=0x8167cbc) at 
cityturn.c:413
#8  0x08087a3e in update_player_activities (pplayer=0x8167cbc) at 
plrhand.c:178
#9  0x0804e68e in end_turn () at srv_main.c:421
#10 0x0805155d in main_loop () at srv_main.c:1722
#11 0x08051def in srv_main () at srv_main.c:1970
#12 0x08049f9c in main (argc=3, argv=0xbffff804) at civserver.c:153
(gdb) fra 2
#2  0x080e4ed6 in ai_advisor_choose_building (pcity=0x411c08a4,
    choice=0xbffff540) at aitools.c:148
148         if (!is_wonder(i) ||
(gdb) l
143         downtown += acity->ai.downtown;
144         cities++;
145       city_list_iterate_end;
146
147       impr_type_iterate(i) {
148         if (!is_wonder(i) ||
149            (!pcity->is_building_unit && 
is_wonder(pcity->currently_building) &&
150            pcity->shield_stock >= improvement_value(i) / 2) ||
151            (!is_building_other_wonder(pcity) &&
152             pcity->ai.grave_danger == 0 && /* otherwise caravans 
will be killed! */
(gdb) print pcity->ai.grave_danger
$6 = -1137215012