Leak in Exception Code C++

Question

I've been working with a school project, and one of the tasks is to make sure it doesn't leak at all. So, I ran my program through valgrind, and because I'm not using any dynamic memory allocation, I didn't think I would find anything.

Oops, I did. Valgrind gave me this:

==22107== 16 bytes in 1 blocks are definitely lost in loss record 1 of 4
==22107==    at 0x100038915: malloc (vg_replace_malloc.c:236)
==22107==    by 0x1000950CF: __cxa_get_globals (in /usr/lib/libstdc++.6.0.9.dylib)
==22107==    by 0x100094DCD: __cxa_allocate_exception (in /usr/lib/libstdc++.6.0.9.dylib)
==22107==    by 0x100051D42: std::__throw_out_of_range(char const*) (in /usr/lib/libstdc++.6.0.9.dylib)
==22107==    by 0x100005463: std::vector<std::vector<int, std::allocator<int> >, std::allocator<std::vector<int, std::allocator<int> > > >::_M_range_check(unsigned long) const (in ./connect3)
==22107==    by 0x100005482: std::vector<std::vector<int, std::allocator<int> >, std::allocator<std::vector<int, std::allocator<int> > > >::at(unsigned long) (in ./connect3)
==22107==    by 0x1000016E3: connect3::checkIfPositionIsBaseCase(Position) const (in ./connect3)
==22107==    by 0x100007BD8: Game::evaluate(Position) (in ./connect3)
==22107==    by 0x100007D72: Game::evaluate(Position) (in ./connect3)
==22107==    by 0x1000043B4: main (in ./connect3)
==22107== 
==22107== LEAK SUMMARY:
==22107==    definitely lost: 16 bytes in 1 blocks
==22107==    indirectly lost: 0 bytes in 0 blocks
==22107==      possibly lost: 0 bytes in 0 blocks
==22107==    still reachable: 8,280 bytes in 3 blocks
==22107==         suppressed: 0 bytes in 0 blocks
==22107== Reachable blocks (those to which a pointer was found) are not shown.
==22107== To see them, rerun with: --leak-check=full --show-reachable=yes

Well, I took a look at it is coming from my function "checkIfPositionIsBaseCase(Position)". Looking at this method (which my partner wrote), I was actually surprised to see something which may have caused the leak.

Exceptions. Here is the code for that function. (It's pretty much the same thing through out, read the first try catch and you've read them all).

///
/// checkIfPositionIsBaseCase
///
bool connect3::checkIfPositionIsBaseCase(Position aPosition) const {

    vector< vector< int > > thisP = aPosition.getBoard();

    for( int w = 0; w < thisP.size(); w++ ) {
        for( int h = 0; h < thisP.at(w).size(); h++ ){
            int thisS = thisP.at( w ).at( h );
            if( thisS != 0 ){
                try{
                    if( thisP.at( w - 1 ).at( h - 1 ) == thisS ){
                        if( thisP.at( w - 2 ).at( h - 2 ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w ).at( h - 1 ) == thisS ){
                        if( thisP.at( w ).at( h - 2 ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w + 1 ).at( h - 1 ) == thisS ){
                        if( thisP.at( w + 2 ).at( h - 2 ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w - 1 ).at( h ) == thisS ){
                        if( thisP.at( w - 2 ).at( h ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w + 1 ).at( h ) == thisS ){
                        if( thisP.at( w + 2 ).at( h ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w - 1 ).at( h + 1 ) == thisS ){
                        if( thisP.at( w - 2 ).at( h + 2 ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w ).at( h + 1 ) == thisS ){
                        if( thisP.at( w ).at( h + 2 ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}

                try{
                    if( thisP.at( w + 1 ).at( h + 1 ) == thisS ){
                        if( thisP.at( w + 2 ).at( h + 2 ) == thisS ){
                            return true;
                        }
                    }
                }catch( out_of_range& ){}
            }
        }
    }
    ///
    /// One possibility
    ///
    for (int i = 0; i < thisP.size(); i++) {
        for (int j = 0; j < thisP.at(i).size(); j++) {
            if (thisP.at(i).at(j) == 0) {
                return false;
            }
        }
    }
    return true;
}

I did a little reading, and it looks like the fact that I am catching exceptions means that I am leaking memory, but I don't know how to resolve this. How can I refactor the code so I don't leak memory?

Answer 1

It's important to understand the difference between leaking memory on a repeated basis (which can lead to exhaustion), and having some underlying support code or library have a one-off intialisation step that gets some heap memory it will use while the program runs (in which case it's not really useful or necessary to free/delete the memory at program termination, and it may be quite a hassle trying to arrange it).

Here, __cxa_get_globals seems to be doing a one-off malloc.

Short story: just make sure you don't get multiple unreleased blocks (or a bigger one) when those exceptions are called repeatedly....

Answer 2

Exceptions are better used for exceptional use cases though, here they will occur quite often (at least 4 times per outer loop...)

This can easily be refactored:

typedef std::vector< std::vector<int> > board_type;

namespace {
  bool check(board_type const& board, int val, int w1, int h1, int w2, int h2)
  {
    if (w1 < 0 || w2 < 0) { return false; }
    if (w1 >= board.size() || w2 >= board.size()) { return false; }
    if (h1 < 0 || h2 < 0) { return false; }
    if (h1 >= board[w1].size() || h2 >= board[w2].size()) { return false; }
    return board[w1][h1] == val && board[w2][h2] == val;
  }
} // anonymous namespace

bool connect3::checkIfPositionIsBaseCase(Position aPosition) const {

  vector< vector< int > > thisP = aPosition.getBoard();

  bool encounteredZero = false;

  for( int w = 0; w < thisP.size(); w++ ) {
    for( int h = 0; h < thisP.at(w).size(); h++ ){
      int val = thisP[w][h];
      if (val == 0) { encounteredZero = true; continue; }

      // Check in all directions with a clock-wise rotation
      if (check(thisP, val, w-1, h-1, w-2, h-2)) { return true; }
      if (check(thisP, val, w  , h-1, w  , h-2)) { return true; }
      if (check(thisP, val, w+1, h-1, w+2, h-2)) { return true; }
      if (check(thisP, val, w+1, h  , w+2, h  )) { return true; }
      if (check(thisP, val, w+1, h+1, w+2, h+2)) { return true; }
      if (check(thisP, val, w  , h+1, w  , h+2)) { return true; }
      if (check(thisP, val, w-1, h+1, w-2, h+2)) { return true; }
      if (check(thisP, val, w-1, h  , w-2, h  )) { return true; }
    }
  }

  return !encounteredZero;
}

And there won't be any exception there :) I also find it easier to verify that the checks are correct and exhaustive...