memory leak strtok & atof

Question

int fromVectoEigen(vector<string> & source, double ** & array, int & n)
{  
    cout<<"n:"<<n<<'\n';
    int counter = 0;

    for (int j = n-1 ; j >= 0 ; j--) // iterate through each line
    {
        string mystring = source.back(); // take last line

        counter++;
        char * str = new char [mystring.length()+1];
        std::strcpy (str, mystring.c_str()); // copy string to char array
        char * pch; // pointer to tokens
        pch = strtok (str," ,-");
        for( int i = 0 ; i<3 ; i++) // dismiss first 3 columns
        {

            pch = strtok (NULL, " ,-");
        }

        for (int i= 0 ; i<n ; i++)
        {
            double val = atof(pch); // cast char to double

            //array[j][i]= val;
            //cout<<array[j][i]<<'\t';
            //pch = strtok (NULL, " ,-");
        }
        //
        //source.pop_back();

        }

    return 0;
}

Hey!

With this function I want to read in a matrix from file to an twodimensional array The matrix has as many lines as columns. And i want to split the lines by space with strtok. The lines were already read into a vector in another function.(which worked - itested it). So I dont know the Problem because I tried to run this with a small matrix 4 columns 4 lines, which worked perfectly! Now I wanted to try it with a big matrix which has over 1000 lines and as many columns. and I get this error message from valgrind:

Invalid read of size 1 ==26501== at 0x58A87AB: __strtod_l_internal (strtod_l.c:538) ==26501== by 0x4015BB: fromVectoEigen(std::vector >&, double**&, int&) (topo.cpp:70) ==26501== by 0x40362B: main (main.cpp:36) ==26501== Address 0x0 is not stack'd, malloc'd or (recently) free'd

I tried the comment/uncomment method and everything works fine up to where i use atof....which I dont understand cause it worked fine with the small matrix

The values in the big matrix look like this: 0.11991517 where as in the small test matrix I only had values like 0 or 0.1

I hope I explained enough...pls ask for more detail if needed.

Hey ... are you sure you have the 4th column ? strtok may return NULL if you're past the last token. (it's not safe to trust that there are n tokens, you can double check and see if there really are enough) — Sulea Cosmin, Feb 13 '14 at 21:42
Also, in the second for loop, assuming n is correct and you have the same number of columns and rows, your for should go up to n-3 (because you have already skipped 3) — Sulea Cosmin, Feb 13 '14 at 21:45
yes the thing is that it all works fine for one test file but just not for the other. yes i know that i already skipped three. this is an option for when there are 3 columns of description before the matrix starts ( but i commented this) — dieHellste, Feb 13 '14 at 23:13
Looks like you're doing everything right, except that you don't check your assumptions (like a valic `pch` pointer). I bet you'll find that `pch` is zero if you print it. Real programmers use `assert` a lot :-) — Klaas van Gend, Feb 14 '14 at 14:20
yes thanks that was it! i changed it to a while ( pch != NULL) — dieHellste, Feb 15 '14 at 15:33

Zac Howland · Answer 1 · 2014-02-14T17:47:04.883

You allocate str, but you never free it ... and with the code you have, you never can. Thus, every time you call this function, you will leak memory.

You should either add a delete [] str; when you are done with it, or don't use dynamic memory at all and stick with std::string.

Additionally, you can avoid the whole conversion issue and just use std::istream::operator>> (e.g. using std::istringstream) to parse your input.

Something like this (untested) should work better for you:

struct reader : std::ctype<char>
{
    reader() : std::ctype<char>(get_table()) {}

    static std::ctype_base::mask const* get_table()
    {
        static std::vector<std::ctype_base::mask> rc(table_size, std::ctype_base::mask());
        rc['\n'] = std::ctype_base::space;
        rc[','] = std::ctype_base::space;
        rc['-'] = std::ctype_base::space;
        rc[' '] = std::ctype_base::space;
        rc['\t'] = std::ctype_base::space;
        return &rc[0];
    }
};

int fromVectorEigen(const std::vector<std::string>& source, std::vector<std::vector<double>>& destination)
{
    reader rdr;
    std::for_each(source.rbegin(), source.rend(), [&](const std::string& s)
    {
        std::istringstream iss(s);
        iss.imbue(std::locale(), &rdr);
        std::string ignored_columns;
        double value;
        iss >> ignored_columns >> ignored_columns >> ignored_columns;
        std::vector<double> values;
        while (iss >> value)
        {
            values.push_back(value);
        }
        destination.push_front(values);
    });
    return 0;
}

And instead of using raw pointers and `delete`, assigning allocated memory to `unique_ptr`/`auto_ptr` or `shared_ptr`. — Peter, Feb 13 '14 at 22:29
@Quirliom Yes, it would be `delete []`. The better solution is not not use (manual) dynamic memory at all, though. — Zac Howland, Feb 14 '14 at 02:19
'code'the delete[] str didnt work. I still get the same error. — dieHellste, Feb 14 '14 at 08:49

score 0 · Answer 2 · answered Feb 13 '14 at 22:19

If you want to stay with this current design of not using streams, but eliminate the explicit allocation of memory, you could use a std::vector:

  #include <vector>
  //...
  std::vector<char> str(mystring.begin(), mystring,end());
  str.push_back(0);
  char * pch; // pointer to tokens
  pch = strtok (&str[0]," ,-");

memory leak strtok & atof

2 Answers2