Why am I getting heap corrpution when I'm not using new or delete?

Question

TL;DR: always remember that std::vector needs to move your data around when it grows, which invalidates any pointers you still have floating around.

I've googled around for this problem a bit, and it seems every case I came across was a question of calling delete on the same pointer twice. I'm writing a small program and I'm getting heap corruption, but the only thing doing heap allocation is the c++ standard library. I have a hunch I'm leaking a reference to a local variable or done something wrong with polymorphism, but I can't figure it out.

#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <vector>

using namespace std;

struct Project;
struct Solution;

struct Line {
    string command;
    vector<string> params;

    void print(ostream &os) {
        os << command << ": ";
        for (string s : params)
            os << s << ' ';
        os << endl;
    }
};

struct Properties {
    vector<string> includes;
    vector<string> libPaths;
    vector<string> libs;
    vector<string> sources;
    vector<string> headers;
    vector<Project *> depends;
    string folder;
    string name;
    string type;
};

struct Project : Properties {
    Project() { built = false; }

    bool built;

    void build() {
        if (built)
            return;
        built = true;

        for (Project *p : depends)
            p->build();

        cout << "Building project: " << name << endl;
    }
};

struct Solution : Properties {
public:
    Project *getProject(const string &name) {
        for (Project &p : projects) {
            if (p.name == name)
                return &p;
        }

        // No project with such a name -- create it
        Project p;
        cout << &p << endl;
        p.name = name;
        projects.push_back(p);
        cout << "Created project: " << name << endl;
        return getProject(name);
    }

private:
    vector<Project> projects;
};

Line parseLine(const string &strline) {
    istringstream stream(strline);
    Line line;

    stream >> line.command;
    while (stream.good()) {
        string tok;
        stream >> tok;
        if (tok.length() > 0)
            line.params.push_back(tok);
    }

    return line;
}

template <typename T>
vector<T> concat(const vector<T> &a, const vector<T> &b) {
    vector<T> vec;
    for (T obj : a)
        vec.push_back(obj);
    for (T obj : b)
        vec.push_back(obj);
    return vec;
}

template <typename T>
void printVector(ostream os, vector<T> v) {
    for (T obj : v)
        os << obj;
    os << endl;
}

int main(int argc, char *argv[]) {
    Solution solution;
    Properties *properties = &solution;
    ifstream stream("testproj.txt");

    Project p[100]; // No error here....

    string linestr;
    for (int lineNum = 1; getline(stream, linestr); lineNum++) {
        Line line = parseLine(linestr);

        if (line.command == "solution") {
            // Make future commands affect the solution
            properties = &solution;
        } else if (line.command == "exe" || line.command == "lib") {
            if (line.params.size() != 1) {
                cerr << "Error at line " << lineNum << endl;
                return 1;
            }

            // Make future commands affect this project
            properties = solution.getProject(line.params[0]);
            properties->type = line.command;
            properties->name = line.params[0];
        } else if (line.command == "includes") {
            properties->includes = concat(properties->includes, line.params);
        } else if (line.command == "libpath") {
            properties->libPaths = concat(properties->libPaths, line.params);
        } else if (line.command == "libs") {
            properties->libs = concat(properties->libs, line.params);
        } else if (line.command == "folder") {
            if (line.params.size() != 1) {
                cerr << "Error at line " << lineNum << endl;
                return 1;
            }

            properties->folder = line.params[0];
        } else if (line.command == "source") {
            properties->sources = concat(properties->sources, line.params);
        } else if (line.command == "header") {
            properties->headers = concat(properties->headers, line.params);
        } else if (line.command == "depends") {
            Project *proj;
            for (string projName : line.params) {
                proj = solution.getProject(projName);
                properties->depends.push_back(proj);
            }
        }
    }
}

The error:

HEAP: Free Heap block 00395B68 modified at 00395BAC after it was freed

Here is my stack trace (sorry no line numbers in the source above):

crashes in malloc & ntdll somewhere up here
libstdc++ ---- incomprehensible name mangling
main.cpp, line 24 (inside Properties::Properties()): (compiler-generated constructor)
main.cpp, line 37 (inside Project::Project()): Project() { built = false; }
main.cpp, line 62 (inside Solution::getProject()): Project p;
main.cpp, line 150 (inside main()): proj = solution.getProject(projName);

It seems to be crashing in the default constructor for Properties? Perhaps while constructing a vector?

Edit: The input file, if it would help:

solution
    includes deps/include deps/include/SDL2
    libpath deps/lib
    libs opengl32 glu32 SDL2main SDL2 libpng16 glew

exe game
    folder game
    source main.cpp
    depends render common

lib render
    folder render
    source Shader.cpp
    header TODO
    depends common

lib common
    folder common
    source util.cpp
    header TODO

Any particular reason you are working with structs instead of classes? — o_weisman, Jun 19 '14 at 07:04
@o_weisman: Any particular reason you think there is a difference? — Ben Voigt, Jun 19 '14 at 07:07
@o_weisman less typing, when base classes and most members are public. — juanchopanza, Jun 19 '14 at 07:08
You _are_ using the heap through STL use. See for yourself with valgrind. — user1095108, Jun 19 '14 at 07:10
FWIW, I did not see the error you are reporting using g++ 4.8.2. — R Sahu, Jun 19 '14 at 07:13
@BenVoigt I think it was already covered in a discussion you participated in here: http://stackoverflow.com/questions/92859/what-are-the-differences-between-struct-and-class-in-c . One example would be sbi's comment to the accepted answer for instance. But honestly I just feel structs should be deprecated by now. — o_weisman, Jun 19 '14 at 09:01
@o_weisman: Sorry, that's a difference between the `struct` and `class` keywords, which both create structs. He's working with classes which ARE structs. The real difference is that "classes" includes unions and union-like types with variant members. Which are not at issue here. Returning to the keywords `struct` and `class`, `struct` is the more appropriate one here anyway. — Ben Voigt, Jun 19 '14 at 15:20

juanchopanza · Accepted Answer · 2014-06-19T07:21:27.670

3

This is a lot of code, but one strong possibility is that you are de-referencing one of the pointers returned by getProject, but this has been invalidated because the vector projects, that holds the objects pointed to, has performed a re-allocation. This invalidates all pointers, references and iterators.

When you do this:

projects.push_back(p);

projects may need to grow, which results in a re-allocation and the invalidation of pointers mentioned above.

Without looking into the code in any depth, it looks like you can implement Solution quite trivially by using an std::map:

struct Solution : Properties
{
public:
    // Check for project with name "name"
    // Add one if it doesn't exist
    // return it
    Project& getProject(const std::string& name) 
    {
      if (!projects.count(name))
      {
        projects[name].name = name;
      }
      return projects[name];
    }

    // Return project with name "name" or raise exception
    // if it doesn't exist
    const Project& getProject(const string &name) const
    {
      return projects.at(name);
    }
private:
    std::map<std::string, Project> projects;
};

edited Jun 19 '14 at 07:21

answered Jun 19 '14 at 07:01

juanchopanza

223,364
34
402
480

Ah, I didn't think of that. Is list guaranteed to keep the pointer in question valid? – Jun 19 '14 at 07:05
One possible solution is to store an index into the vector instead of a pointer. This won't work if items are removed from the middle of the projects vector, but that was already broken. `std::list` pointers and iterators also aren't invalidated by insertions and deletions elsewhere... but list is a fair amount slower than vector. – Ben Voigt Jun 19 '14 at 07:05
@Relish `std::list` has stronger guarantees in this respect because it just adds nodes so no re-allocations are needed. In this case, it should work. It is arguably one of the few cases when it makes sense to use a list. – juanchopanza Jun 19 '14 at 07:06
@juanchopanza: well you solved that problem awfully quick! What do you think would be a good way to keep track of my dependency graph? Should I use pointers from the list, or would it be a good idea to just use strings and resolve the pointers on the fly? – Jun 19 '14 at 07:09
At a glance, it looks like you could use an `std::map`. `std::map` is also a node-based container, following similar invalidation rules to `std::list`, and gives you logarithmic look-up. – juanchopanza Jun 19 '14 at 07:11
@Relish I added an example using `std::map`. I changed it to return references, but you can return pointers too. – juanchopanza Jun 19 '14 at 07:16
@juanchopanza my hero – Jun 19 '14 at 07:19

Why am I getting heap corrpution when I'm not using new or delete?

1 Answers1