Page 1 of 1

FreeOrion bug hunt 2009 #1

Posted: Fri Jan 02, 2009 10:32 pm
by OndrejR
I did source code analysis to FreeOrion with cppcheck and found this bugs:

Code: Select all

[network/Message.cpp:121] Uninitialized member variable 'Message::m_synchronous_response'
[server/ServerApp.cpp:69] Uninitialized member variable 'ServerApp::m_single_player_game'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_design_id'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_object_id'
I also did code analysis with Valgrind and found a lot of bugs(attachment valgrind.zip)

Re: FreeOrion bug hunt 2009 #1

Posted: Sat Jan 03, 2009 2:00 am
by Geoff the Medio
OndrejR wrote:I did source code analysis to FreeOrion with cppcheck and found this bugs:

Code: Select all

[network/Message.cpp:121] Uninitialized member variable 'Message::m_synchronous_response'
[server/ServerApp.cpp:69] Uninitialized member variable 'ServerApp::m_single_player_game'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_design_id'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_object_id'
None of those are really "bugs" since all those variables (should) get initialized later before they're used. For example, the Universe last allocated ids are set to -1 in Universe::CreateUniverse or are set when deserializing the universe that's sent from the server or loaded from disk. I added appropriate initializations for them anyway, though.
I also did code analysis with Valgrind and found a lot of bugs(attachment valgrind.zip)
There are a bunch of font bugs that might interest tzlaine, but a lot of it is boost internal stuff or openal internal stuff that I doubt we can do anything about. I'm also skeptical of the accuracy... For example, there is this bug listing...

Code: Select all

==10554== 15,768 (792 direct, 14,976 indirect) bytes in 9 blocks are definitely lost in loss record 752 of 1,257
==10554==    at 0x4C21FCC: operator new(unsigned long) (vg_replace_malloc.c:230)
==10554==    by 0x520BCF: __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::allocate(unsigned long, void const*) (new_allocator.h:92)
==10554==    by 0x520BF3: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_get_node() (stl_tree.h:357)
==10554==    by 0x525C9F: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_create_node(std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:366)
==10554==    by 0x52D8C4: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_insert_(std::_Rb_tree_node_base const*, std::_Rb_tree_node_base const*, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:852)
==10554==    by 0x52DA29: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_insert_unique(std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:1148)
==10554==    by 0x52DC51: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_insert_unique_(std::_Rb_tree_const_iterator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:1188)
==10554==    by 0x52DFF7: std::map<std::set<int, std::less<int>, std::allocator<int> >, double, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::insert(std::_Rb_tree_iterator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_map.h:496)
==10554==    by 0x52E0AA: std::map<std::set<int, std::less<int>, std::allocator<int> >, double, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::operator[](std::set<int, std::less<int>, std::allocator<int> > const&) (stl_map.h:419)
==10554==    by 0x53916D: ResourcePool::SetSystemSupplyGroups(std::set<std::set<int, std::less<int>, std::allocator<int> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::set<int, std::less<int>, std::allocator<int> > > > const&) (ResourcePool.cpp:177)
==10554==    by 0x4F79F6: Empire::InitResourcePools() (Empire.cpp:2376)
==10554==    by 0x9ABDE1: MapWnd::InitTurn(int) (MapWnd.cpp:713)
Which refers to this line of ResourcePool::SetSystemSupplyGroups:

Code: Select all

        m_supply_system_groups_resource_production[*it] = 0.0;
were it is a std::set<std::set<int> >::const_iterator. So apparently assigning a value to an possibly creating a new entry in a std::map<std::set<int>, double> causes a loss of memory? m_supply_system_groups_resource_production is cleared earlier in the function, so I don't see what is wrong with this code...

Re: FreeOrion bug hunt 2009 #1

Posted: Sat Jan 03, 2009 9:50 am
by OndrejR
Geoff the Medio wrote:but a lot of it is boost internal stuff or openal internal stuff that I doubt we can do anything about
I can at least found that this bug exists, install debug boost and openal libraries and report this bug to boost and openal forum.
Geoff the Medio wrote:I'm also skeptical of the accuracy... For example, there is this bug listing...
Valgrind errors are not always really errors. But error rate should be high and if it is really bug we need to solve.

Re: FreeOrion bug hunt 2009 #1

Posted: Sat Jan 10, 2009 1:27 am
by Maz
Valgrind indeed is great tool. But there are certain cases when it does not detect memory issues correctly (or at least there was ... some (longish) time ago). One such situation is when shared memory is reserved / initialized by one process, and another process then uses / frees it (As far as I remember).

Also I do not know what kind of cleanup procedures you do when one exits the game, but often this part is overlooked (well, OS releases the memory when process ends anyways). However valgrind thinks the memory has leaked (as it technically speaking has - although with no other real meaning but showing up as an error in valgrind.

However valgrind reports can (and should) be used (in my humble opinion). Especially invalid writes and reads should all be analyzed carefully.

Re: FreeOrion bug hunt 2009 #1

Posted: Sat Jan 10, 2009 3:36 am
by tzlaine
I regularly use Valgrind, and the recent work reported against the "Memory Leaks?" SourceForge bug is directly attributable to Valgrind reports. However, it irritatingly reports many std::string as "possibly lost", due to the COW implementation. It makes looking for legit leaks pretty difficult.