View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002536 | FSSCP | Pilot data | public | 2011-11-05 08:17 | 2012-11-24 02:43 |
| Reporter | Echelon9 | Assigned To | Echelon9 | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | closed | Resolution | unable to reproduce | ||
| Product Version | 3.6.14 RC1 | ||||
| Summary | 0002536: Valgrind: Conditional jump or move depends on uninitialized value(s) - mission_campaign_savefile_load() | ||||
| Description | Valgrind reports a memory management error caused by an uninitialised value created by a stack allocation in mission_campaign_savefile_load(char*, player*) (missioncampaign.cpp:1105) | ||||
| Additional Information | ==2721== Conditional jump or move depends on uninitialised value(s) ==2721== at 0x46AECC5: strcasecmp_l (in /usr/lib/system/libsystem_c.dylib) ==2721== by 0x46AEEE5: strcasecmp (in /usr/lib/system/libsystem_c.dylib) ==2721== by 0x423CF1: weapon_info_lookup(char const*) (weapons.cpp:459) ==2721== by 0x237060: red_alert_read_wingman_status_campaign(CFILE*, char (*) [32], char (*) [32]) (redalert.cpp:918) ==2721== by 0x2284AD: mission_campaign_savefile_load(char*, player*) (missioncampaign.cpp:1368) ==2721== by 0x22AACF: mission_campaign_load(char*, player*, int) (missioncampaign.cpp:452) ==2721== by 0x22BE38: mission_load_up_campaign(player*) (missioncampaign.cpp:2346) ==2721== by 0x1EB248: player_select_close() (playermenu.cpp:467) ==2721== by 0x6574E: game_leave_state(int, int) (freespace.cpp:5862) ==2721== by 0x117D6D: gameseq_set_state(int, int) (gamesequence.cpp:277) ==2721== by 0x6C4C0: game_process_event(int, int) (freespace.cpp:5124) ==2721== by 0x118335: gameseq_process_events() (gamesequence.cpp:397) ==2721== Uninitialised value was created by a stack allocation ==2721== at 0x2271B0: mission_campaign_savefile_load(char*, player*) (missioncampaign.cpp:1105) | ||||
| Tags | No tags attached. | ||||
|
|
Assigning to CommanderDJ, since this should be a good bug for him to practice his debugging skills on. CommanderDJ, it's possible that the line numbers as described in Additional Information are no longer accurate. In that case, you'll have to check out a separate copy of the source as it appeared on the date this bug was filed. |
|
|
CommanderDJ: I'm happy to walk you through this bug if you have questions. I'm travelling for the next two weeks. |
|
|
Echelon9, some advice would be appreciated. I don't have Valgrind myself (Windows user) so I can't test with it to see if the bug's gone. Perhaps you can check my understanding of the error report then: Somewhere in mission_campaign_savefile_load(), a variable is created but never initialised. This variable is then used in a conditional jump or move, and the result of the conditional evaluates to what it does because the variable is uninitialised. (I'm just guessing here mostly, lol.) Is this assessment accurate at all? :P Also, is the first part of the error report a function call chain that leads to this bug? |
|
|
Yes and yes. You can find the variable which was created but never initialized on the particular function chain on the last line of the report. |
|
|
Okay, so I checked out a copy of the code as it was on the date this report was created, and the variable in question (as in, at missioncampaign.cpp 1105) was CFILE *fp (currently on line 1114). It's true that this variable isn't initialised upon declaration, but I'm not sure how this leads to the bug as it's initialised with the cfopen function on line 1156 before being used in the function. Would, say, initialising it to NULL upon declaration fix this, or am I missing something here? |
|
|
It's possible that this is a false positive. Where is the Valgrind report? Does the last line of the report specifically mention fp? |
|
|
I thought the Valgrind report was what's in the Additional Information window. I don't have Valgrind myself so I can't generate my own reports. I thought it was fp because that's what's at missioncampaign.cpp 1105 (last line of Additional Information) at the time this bug was filed. |
|
|
It should be a variable passed to strcasecmp. I wouldn't have thought a CFILE *fp would have been. |
|
|
I thought there might be another report, perhaps one which Echelon9 forgot to attach, based on the line "You can find the variable which was created but never initialized on the particular function chain on the last line of the report." But taking a closer look at the information, it seems that mission_campaign_savefile_load is where the variable is allocated but strcasecmp is where it's referenced. The function call chain leads through red_alert_read_wingman_status_campaign and weapon_info_lookup first. Since there's only one weapon_info_lookup in the red_alert function, it seems that w_name is the variable that Valgrind is complaining about. |
|
|
Yup, my recollection of the bug when I looked at it was that the variable was related to a name. |
|
|
Ah okay. Thanks for your patience and for walking me through this. So I looked into this and w_name is memset to 0 well before the call to red_alert_read_wingman_status_campaign, and indeed it is actually filled with weapon names before that call ever occurs, so I'm not sure what's going on there. There is a call to weapon_info_lookup before the red_alert function, but that one is always called with an already filled entry of w_name. |
|
|
Maybe Valgrind doesn't like the one-single-block memset. Maybe Valgrind wants the initialization to be done by looping over the first dimension of the array and memsetting NAME_LENGTH for each index. Of course, that would raise the question of why the same error isn't triggered for s_name. Who has the ability to run Valgrind and test various methods of initialization? |
|
|
I can do that - Echelon9 - can you confirm that to reproduce this issue I just need to run run valgrind with FSO as an argument, then play through the part of the game that runs the code in mission_campaign_savefile_load() (i.e. entering the campaign room?) |
|
|
It was created by running FSO through Valgrind, up to the main hall lobby and then exiting. With the w_name variable, are we sure we are null terminating the string when data is copied to it? |
|
|
I think the w_name strings are being null terminated, all chars are set to null by the memset, and the cfread_string_len function also sets the last char of the string to null (i.e. char 0 in ASCII), and ensures any data read from file is at most 31 chars long (1 less than NAME_LENGTH) so that the last char can be set to null. I've tried running valgrind on trunk 7939 (this is a close as I could get to 2011-11-05, 7941 & 7942 seg faulted on me) and I can't reproduce the error report. Here's the valgrind options I used: valgrind -v --leak-check=yes --track-origins=yes ./fs2_open_INF_d Here's the valgrind version: $ valgrind --version valgrind-3.6.0.SVN-Debian Is this different from what you were using Echelon9? |
|
|
Assigning to niffiwan since I can't really be of any use here. Will be keeping an eye on it though. |
|
|
Niffiwan, that's the Valgrind setup I used. |
|
|
Reported with latest Valgrind SVN, and FSO SCP r8543 ----- ==43478== Conditional jump or move depends on uninitialised value(s) ==43478== at 0x5836D0F: strcasecmp_l (in /usr/lib/system/libsystem_c.dylib) ==43478== by 0x5836EE5: strcasecmp (in /usr/lib/system/libsystem_c.dylib) ==43478== by 0x523BB8: weapon_info_lookup(char const*) (weapons.cpp:457) ==43478== by 0x28FC89: mission_campaign_savefile_load(char*, player*) (missioncampaign.cpp:1496) ==43478== by 0x28DAB5: mission_campaign_load(char*, player*, int) (missioncampaign.cpp:667) ==43478== by 0x295263: mission_load_up_campaign(player*) (missioncampaign.cpp:2346) ==43478== by 0x23D6DE: player_select_close() (playermenu.cpp:475) ==43478== by 0x3A027: game_leave_state(int, int) (freespace.cpp:5880) ==43478== by 0x11C4B9: gameseq_set_state(int, int) (gamesequence.cpp:277) ==43478== by 0x3860E: game_process_event(int, int) (freespace.cpp:5133) ==43478== by 0x11CA81: gameseq_process_events() (gamesequence.cpp:397) ==43478== by 0x3D02B: game_main(char*) (freespace.cpp:7103) ==43478== Uninitialised value was created by a stack allocation ==43478== at 0x28DB16: mission_campaign_savefile_load(char*, player*) (missioncampaign.cpp:1106) |
|
|
So niffiwan, what's the status on this? |
|
|
I can't reproduce this, maybe it's something that appears only on OSX? Re-assigning to Echelon9... |
|
|
If the build is accurate, this was reported against 3.6.14 RC1, which means missioncampaign.cpp would be revision 6773. And in that revision, the variable on line 1105 is k_count, which makes as much sense as any of the previous suggestions: that is, none at all. We need Echelon9 to run Valgrind against latest trunk, or else close this as unable to reproduce. |
|
|
I'm going to close for now, as although I kept seeing it reported by latest Valgrind in the trunk versions of FS2_Open some time ago, I'm unable to reconfirm with my current setup. Valgrind doesn't fully support OS X 10.8 that I'm using post the Diaspora release. If the situation changes and I can repro it again, I'll reopen the ticket. |
|
|
Sounds good... and correcting categorization to "unable to reproduce". |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2011-11-05 08:17 | Echelon9 | New Issue | |
| 2011-11-05 08:17 | Echelon9 | Status | new => assigned |
| 2011-11-05 08:17 | Echelon9 | Assigned To | => Echelon9 |
| 2011-11-26 07:56 | Echelon9 | Description Updated | |
| 2011-11-26 07:56 | Echelon9 | Additional Information Updated | |
| 2012-02-09 06:57 | Goober5000 | Note Added: 0013266 | |
| 2012-02-09 06:57 | Goober5000 | Assigned To | Echelon9 => CommanderDJ |
| 2012-02-11 03:25 | Echelon9 | Note Added: 0013275 | |
| 2012-02-11 03:38 | CommanderDJ | Note Added: 0013276 | |
| 2012-02-11 04:34 | Echelon9 | Note Added: 0013278 | |
| 2012-02-12 03:24 | CommanderDJ | Note Added: 0013286 | |
| 2012-02-12 05:15 | Goober5000 | Note Added: 0013288 | |
| 2012-02-12 05:29 | CommanderDJ | Note Added: 0013289 | |
| 2012-02-12 07:07 | Echelon9 | Note Added: 0013290 | |
| 2012-02-12 08:38 | Goober5000 | Note Added: 0013291 | |
| 2012-02-12 08:51 | Echelon9 | Note Added: 0013292 | |
| 2012-02-12 14:26 | CommanderDJ | Note Added: 0013293 | |
| 2012-02-13 00:31 | Goober5000 | Note Added: 0013294 | |
| 2012-02-13 22:53 | niffiwan | Note Added: 0013305 | |
| 2012-02-14 01:21 | Echelon9 | Note Added: 0013318 | |
| 2012-02-14 11:17 | niffiwan | Note Added: 0013326 | |
| 2012-02-15 16:42 | CommanderDJ | Assigned To | CommanderDJ => niffiwan |
| 2012-02-15 16:42 | CommanderDJ | Note Added: 0013331 | |
| 2012-02-20 18:41 | Echelon9 | Note Added: 0013338 | |
| 2012-03-01 12:26 | Echelon9 | Note Added: 0013370 | |
| 2012-07-02 05:28 | Goober5000 | Note Added: 0013782 | |
| 2012-07-02 09:13 | niffiwan | Note Added: 0013791 | |
| 2012-07-02 09:14 | niffiwan | Assigned To | niffiwan => Echelon9 |
| 2012-11-23 04:14 | Goober5000 | Note Added: 0014156 | |
| 2012-11-24 00:56 | Echelon9 | Note Added: 0014167 | |
| 2012-11-24 00:56 | Echelon9 | Status | assigned => closed |
| 2012-11-24 00:56 | Echelon9 | Resolution | open => fixed |
| 2012-11-24 02:43 | Goober5000 | Note Added: 0014171 | |
| 2012-11-24 02:43 | Goober5000 | Resolution | fixed => unable to reproduce |
