Unclear segmentation fault in a constructor

Question

I have built a constructor for a matrix object. The data is being stored in an array of struct val, which in turn hold position (in the matrix) and value. This is the code:

SparseMatrix::SparseMatrix(const int numRow, const int numCol, vector<double> fill):
        Matrix(numRow,numCol)
{
    _matrix = new vector<val*>(fill.size());
    vector<double>::iterator itr = fill.begin();
    for(signed int i=0; i<numRow; i++)
    {
        for(signed int j=0; j<numCol; j++, itr++)
        {
            if (*itr == 0)
            {
                continue;
            }
            val *temp = new val;
            temp->value = *itr;
            temp->x = i;
            temp->y = j;
            _matrix->push_back(temp);
            cout << "Psition: " << ": " << _matrix->front()->x << //<--ERROR     
            cout << " " << _matrix->back()->y << endl;
        }
    }
}

You'll notice that I've added cout just to verify that push_back does not really work for me. _matrix is on the heap and so does all the structs that are being held in it. All created using 'new'. I fail to see why this doesn't work. One line after I push a new struct pointer to the vector I can't read it (segmentation fault as I said).

Any ideas? thanks!

EDIT: Sorry, this is valgrind's message:

==13334== Invalid read of size 4
==13334==    at 0x804AEAE: SparseMatrix::SparseMatrix(int, int, std::vector<double,     std::allocator<double> >) (in /home/yotamoo/workspace/ex3/main)
==13334==    by 0x8048C10: main (in /home/yotamoo/workspace/ex3/main)
==13334==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
==13334== 
==13334== 
==13334== Process terminating with default action of signal 11 (SIGSEGV)
==13334==  Access not within mapped region at address 0x8
==13334==    at 0x804AEAE: SparseMatrix::SparseMatrix(int, int, std::vector<double,     std::allocator<double> >) (in /home/yotamoo/workspace/ex3/main)
==13334==    by 0x8048C10: main (in /home/yotamoo/workspace/ex3/main)
==13334==  If you believe this happened as a result of a stack
==13334==  overflow in your program's main thread (unlikely but
==13334==  possible), you can try to increase the size of the
==13334==  main thread stack using the --main-stacksize= flag.
==13334==  The main thread stack size used in this run was 8388608.

And - segmentation fault occurs during the first iteration!

What did gdb tell you? Valgrind? And why are you storing pointers? — Lightness Races in Orbit, Sep 10 '11 at 20:51

Daniel Trebbien · Accepted Answer · 2011-09-14T22:24:42.683

This is the std::vector<val *> constructor that is being used to construct *_matrix:

explicit vector ( size_type n, const val *& value = val *(), const std::allocator<val *>& = std::allocator<val *>() );

The invocation of that constructor with n set to fill.size() creates a new std::vector<val *> having fill.size() default-constructed val * objects (all NULL). When you push back a new val pointer, *_matrix has fill.size() + 1 elements and _matrix->front() is still a NULL pointer. In effect, you are dereferencing NULL.

You are probably looking for the reserve() method.

EDIT: There are some other things that I noticed which can be improved:

The SparseMatrix constructor is taking fill by value. This means that in order to construct a new SparseMatrix, a complete copy of the vector of double objects is made. You should pass fill by const-reference instead.
With STL container iterators, unless you need the result of a post-increment, you should always use the pre-increment operator.
If one of the allocations of a new val object throws a std::bad_alloc exception, then you will leak memory.
Consider using a pool allocator to allocate the many val objects. Some C/C++ runtime library implementations (e.g. OpenBSD's) randomize memory allocations. Allocating many, small objects on the heap can lead to severe heap fragmentation.

score 2 · Answer 2 · answered Sep 10 '11 at 20:55

2

If numCol > fill.size(), your loop will run off the end.

answered Sep 10 '11 at 20:55

Tom Zych

13,329
9
36
53

Thomas · Answer 3 · 2013-02-15T01:00:11.557

The error, as explained by others, is that you are initializing with nullptrs instead of reserving and then dereferencing them.

You are also reserving space for all items and not freeing the unused space. Your sparse array will always use more space than the original vector passed in.

You should not be newing all these items. Work with a vector of values. This will make your code exception safe.

There is also the issue that you are not checking you access to fill. There should at least be an assert on the number of elements.

Assuming that val is a struct like this:

struct val
{
     val( int x, int y, double value ):x(x),y(y),value(value){}
     int x;
     int y;
     double value;
};

then an improved/corrected version could look like this:

class SparseMatrix
{
   vector<val> _matrix;

public:

    SparseMatrix(const int numRow, const int numCol, vector<double> const& fill)
    {
        assert( numCol > 0 );
        assert( numRow > 0 );
        assert( numRow * numCol == fill.size() );

        int skipped = std::count( fill.begin(), fill.end(), 0.0 );

        _matrix.reserve( fill.size() - skipped );

        vector<double>::const_iterator itr = fill.begin();
        for( int i=0; i<numRow; ++i)
        {
            for(int j=0; j<numCol; ++j, ++itr)
            {
                if( *itr != 0.0 )
                {
                    _matrix.push_back( val( i, j, *itr ) );
                }
            }
        }
    }
};

Unclear segmentation fault in a constructor

3 Answers3