headmyshoulder / odeint-v2

odeint - solving ordinary differential equations in c++ v2
http://headmyshoulder.github.com/odeint-v2/
Other
337 stars 102 forks source link

my_vector example segfault #180

Closed mariomulansky closed 8 years ago

mariomulansky commented 8 years ago

The my_vector example segfaults, see: see: http://stackoverflow.com/questions/33532775

sehe commented 8 years ago

I've analyzed the problem.

The problem is the default construction only reserves MAX_N capacity but size is 0.

So indexing [0..2] is UB.

The stepper defaults-constructs the State/Deriv type. It may /appear/ to work for a bit and on some library implementations because of the reserve calls in my_vector. But, in any case they aren't done on copy-construction of my_vector::m_v and hence the segfault anyways.

See for more detail: http://stackoverflow.com/a/33533309/85371

Proposed fix: simplify the constructors and make sure initial size is MAX_N because that's what the stepper appears to expect

template <int MAX_N> class my_vector {
    typedef std::vector<double> vector;

  public:
    typedef vector::iterator iterator;
    typedef vector::const_iterator const_iterator;

  public:
    my_vector(const size_t N = MAX_N) : m_v(N) { assert(N <= MAX_N); }

#ifdef DEBUG
    const double & operator[] (size_t n) const { return m_v.at(n); } 
    double &       operator[] (size_t n)       { return m_v.at(n); } 
#else
    const double & operator[] (size_t n) const { return m_v[n]; } 
    double &       operator[] (size_t n)       { return m_v[n]; } 
#endif

    iterator begin()             { return m_v.begin(); } 
    const_iterator begin() const { return m_v.begin(); } 
    iterator end()               { return m_v.end();   } 
    const_iterator end() const   { return m_v.end();   } 
    size_t size() const          { return m_v.size();  } 
    void resize(const size_t n)  { m_v.resize(n);      } 

  private:
    std::vector<double> m_v;
};

Note the proposed assert and .at usages in DEBUG mode, which would have caught this bug 4 years ago.

sehe commented 8 years ago

Created PR at https://github.com/boostorg/odeint/pull/15 against develop

mariomulansky commented 8 years ago

This fixes this specific example, but the fundamental problem is different. odeint has internal resizing functionality that should call resize on my_vector to ensure memory allocation. But somehow this doesn't work here. I'll try to find the problem with this.

mariomulansky commented 8 years ago

the problem was an inconsistent parameter type. my_vector was defined with an int, while in the is_resizeable is was referred with a size_t. This is fixed now.

sehe commented 8 years ago

Awesome. Good work.