[freespace2] code cleaning - stage 2

[Taylor Richards]

On Mon, 2004-05-10 at 18:07, Mihai Rusu wrote:
> Hi
> 
> For quite some time I use a super debugging tool to detect and fix memory 
> utilization bugs. Its called valgrind, more information can be found at 
> http://valgrind.kde.org.
> 
> I would like to disucss some possible problems found by valgrind bellow. 
> Information is based on icculus.org latest CVS:

I've valgrinded the thing to death.  I even went so far as to get the
demo playback system working and just leave valgrind playing missions
for hours at the time.  Newer versions would probably pick up new things
though if you don't mind to continue looking.

> ==12654== Conditional jump or move depends on uninitialised value(s)
> ==12654==    at 0x814EBBE: ds_play(int, int, int, int, int, int, int, bool) (ds.cpp:2593)
> ==12654==    by 0x814CAF0: snd_play_looping(game_snd*, float, int, int, float, int, int) (sound.cpp:1080)
> ==12654==    by 0x80C968A: main_hall_start_ambient() (mainhallmenu.cpp:1859)
> ==12654==    by 0x80D41E8: player_select_init() (playermenu.cpp:449)
> ==12654==    by 0x805280C: game_enter_state(int, int) (freespace.cpp:6459)
> ==12654==    by 0x8077A43: gameseq_set_state(int, int) (gamesequence.cpp:497)
> ==12654==    by 0x8051B56: game_process_event(int, int) (freespace.cpp:5635)
> ==12654==    by 0x8077C8C: gameseq_process_events() (gamesequence.cpp:610)
> ==12654==    by 0x80536F3: WinMainSub(int, int, char*, int) (freespace.cpp:7203)
> ==12654==    by 0x805373A: WinMain(int, int, char*, int) (freespace.cpp:7241)
> 
> Seems to me that Channels[i].is_voice_msg doesnt get set/initilized 
> properly for every channel < MAX_CHANNELS.  In what function should this 
> happen ?
> 
> ==12654== Conditional jump or move depends on uninitialised value(s)
> ==12654==    at 0x814F30C: ds_do_frame() (ds.cpp:3759)
> ==12654==    by 0x814D36A: snd_do_frame() (sound.cpp:1575)
> ==12654==    by 0x8052843: game_do_state_common(int, int) (freespace.cpp:6485)
> ==12654==    by 0x80528AC: game_do_state(int) (freespace.cpp:6525)
> ==12654==    by 0x8077CC7: gameseq_process_events() (gamesequence.cpp:620)
> ==12654==    by 0x80536F3: WinMainSub(int, int, char*, int) (freespace.cpp:7203)
> ==12654==    by 0x805373A: WinMain(int, int, char*, int) (freespace.cpp:7241)
> ==12654==    by 0x8054D50: main (unixmain.cpp:40)
> ==12654==    by 0x404C3C63: __libc_start_main (in /lib/libc-2.3.2.so)
> ==12654==    by 0x804B990: (within /home/dizzy/work/cvs/freespace2/freespace2)
> Same problem here.

I thought I fixed these two.  I must have forgotten to commit it at some
point.  It needs to go in ds_init_channels() if you want to do that.

> ==12654== Use of uninitialised value of size 4
> ==12654==    at 0x8080EB7: opengl_create_texture_sub(int, int, unsigned short*, int, int, int, int, int, int, int, int, tcache_slot_opengl*, int, int) (gropengl.cpp:2152)
> ==12654==    by 0x808145D: opengl_create_texture(int, int, tcache_slot_opengl*, int) (gropengl.cpp:2314)
> ==12654==    by 0x8081907: gr_opengl_tcache_set(int, int, float*, float*, int, int, int, int) (gropengl.cpp:2468)
> ==12654==    by 0x807DC00: gr_opengl_aabitmap_ex_internal(int, int, int, int, int, int) (gropengl.cpp:807)
> ==12654==    by 0x807E423: gr_opengl_string(int, int, char*) (gropengl.cpp:1026)
> ==12654==    by 0x81BB904: UI_WINDOW::draw_one_xstr(UI_XSTR*, int) (window.cpp:759)
> ==12654==    by 0x81BB949: UI_WINDOW::draw_xstrs() (window.cpp:774)
> ==12654==    by 0x81BB07B: UI_WINDOW::draw() (window.cpp:379)
> ==12654==    by 0x80D49F7: player_select_do() (playermenu.cpp:693)
> ==12654==    by 0x8052C96: game_do_state(int) (freespace.cpp:6737)
> Seems that xlat[bmp_data[i*bmap_w+j]] wasnt set/initilized before reading 
> it.

I never really looked at this one close enough to figure it out.  If you
figure out where the error is please let me know.

> ==12654== Source and destination overlap in strncpy(0x872e789, 0x872e789, 32)
> ==12654==    at 0x40022156: strncpy (in /usr/lib/valgrind/vgskin_memcheck.so)
> ==12654==    by 0x8158D8C: player_set_squad_bitmap(player*, char*) (managepilot.cpp:1377)
> ==12654==    by 0x815707D: read_pilot_file(char*, int, player*) (managepilot.cpp:621)
> ==12654==    by 0x80D4B32: player_select_close() (playermenu.cpp:774)
> ==12654==    by 0x8052105: game_leave_state(int, int) (freespace.cpp:6015)
> ==12654==    by 0x8077A15: gameseq_set_state(int, int) (gamesequence.cpp:493)
> ==12654==    by 0x805150B: game_process_event(int, int) (freespace.cpp:5345)
> ==12654==    by 0x8077C8C: gameseq_process_events() (gamesequence.cpp:610)
> ==12654==    by 0x80536F3: WinMainSub(int, int, char*, int) (freespace.cpp:7203)
> ==12654==    by 0x805373A: WinMain(int, int, char*, int) (freespace.cpp:7241)
> 
> This while probably harmless on many libc on x86 it can prove dangerous on 
> other architectures/in time because strcpy should not ever overlap. I dont 
> have the knowledge to either decide if change this to memmove() or the bug 
> comes from the calling codes.

I haven't seen this one in my valgrind logs before.  MAX_FILENAME_LEN
should be 32 so check to see if it's been changed anywhere and that
pstypes.h is included in managepilot.cpp.  MAX_FILENAME_LEN cannot be
the Unix size because it would break multiplayer.  It must remain 32.

> This "bugs" I found untile the "select player" window. Ill dig some more 
> when I have more time if there is any interest.

Anything that will make the game work at little better is welcome.  I
can also send you a couple of my old valgrind logs if you want to
compare.


Taylor

-- 
Taylor Richards <mtrs at bellsouth.net>