I have a custom FastStack class, implemented as a fixed size array and an index into that array.

In my copy constructor, I allocate the array and then assign each object from the copy's array into the new array. There's some refcounting in the objects on the stack, hence assignment is used rather than a simple copy.

The problem is that when allocating the array, it sometimes overwrites part of the other stack's array. As can be expected, this leads to eventual segmentation faults when that data is dereferenced.

class FastStack {
private:
    int m_size, m_ptr;
    ObjectRef* m_stack;

public:
    FastStack(int size) : m_size(size), m_ptr(-1) {
        m_stack = new ObjectRef[m_size];
    }

    FastStack(const FastStack& copy) : m_size(copy.m_size), m_ptr(copy.m_ptr) {
        long a = (long)copy.m_stack[0];

        m_stack = new ObjectRef[m_size];

        if ((long)copy.m_stack[0] != a)
            fprintf(stderr, "\nWe have a serious problem!\n\n");

        for (int i = 0; i <= m_ptr; i++)
            m_stack[i] = copy.m_stack[i];
    }

    ~FastStack() {
        delete[] m_stack;
    }
};

class ObjectRef {
private:
    DataObj* m_obj;

public:
    ObjectRef() : m_obj(0) { }

    ObjectRef(DataObj* obj) : m_obj(obj) {
        if (m_obj) m_obj->addRef();
    }

    ObjectRef(const ObjectRef& obj) : m_obj(obj.m_obj) {
        if (m_obj) m_obj->addRef();
    }

    ~ObjectRef() {
        if (m_obj) m_obj->delRef();
    }

    ObjectRef& operator=(DataObj* obj) {
        if (obj) obj->addRef();
        if (m_obj) m_obj->delRef();
        m_obj = obj;
        return *this;
    }

    ObjectRef& operator=(const ObjectRef& obj) {
        if (obj.m_obj) obj.m_obj->addRef();
        if (m_obj) m_obj->delRef();
        m_obj = obj.m_obj;
        return *this;
    }
};

I see that "We have a serious problem!" line shortly before a segfault, and stepping through it with gdb I can see that one of the ObjectRefs created by new has the same address as the other stack's array.

My first instinct is 开发者_开发百科to say that new should never be allocating memory that is already in use, but that clearly seems to be the case here and I am at a complete loss as to what can be done.

Added: At the time that I see this happen, m_size = 2 and m_ptr = 0.

The full code for this program is on github at https://github.com/dvpdiner2/pycdc but is quite convoluted and difficult to follow.

Sometimes new is known to give memory that has been already allocated to other objects and when that happens good C++ code must repeat the allocation in the hope of having memory not shared with others...

NO!

new is ok, your program is buggy. Write this one hundred times on the blackboard.

new will never give you memory that has been given to someone else BUT if you did anything bad before (i.e. accessing out of the limits, dereferencing a pointer once the object it was pointing to has been deallocated, used an invalidated iterator and jillions of other possible "UB" violations) then new gains a special permit and can do whatever it likes, including making daemons appearing out of your nosrils.

Your code appears highly suspect but I don't see anything that is for sure an error (too much code is hidden, for example what exactly is an ObjRef and how it is declared). From what I see however I'm almost sure you did something wrong before getting to that new allocation because there many violations of good C++ practices (for example the shown class has a constructor and a copy constructor, but no assignment operator nor a destructor).

The biggest problem with the shown code however is that looks like an half-backed and buggy attempt of mimicking a subset of what a plain std::vector would do. To say exactly what is(are) the problem(s) however requires more context... this is bad code but could be legal depending on how other code has been written. It's also evident that even this small piece of code has been altered and reduced as there are no methods to access anything, no friends and data members are private (so it's basically impossible to do anything with those objects).

What you should do is use a std::vector to manage the memory, and just keep the index (m_ptr) managed explicitly by you. This will solve all these problems.

However, I seriously can't see what the hell is wrong with your code. Consider yourself an optimizer, and take a quick pass, focusing just on a and copy.m_stack[0].

long a = (long)copy.m_stack[0];

m_stack = new ObjectRef[m_size];

if ((long)copy.m_stack[0] != a)



long a = (long)copy.m_stack[0];

if ((long)copy.m_stack[0] != a)


if (copy.m_stack[0] != copy.m_stack[0])

If that condition is true, then you must have corrupted your heap, and possibly but less likely the stack. Heap corruption can bite in apparently completely unrelated code to the code at fault. Either that, or you're screwing with threads and not telling us about it. (and getting it wrong).

Of course, you're missing several other important functions, like operator= and destructor, but hey- none of which you would have to implement yourself if you used a vector.

In a loop you iterate from 0 to m_ptr:

for (int i = 0; i <= m_ptr; i++)
    m_stack[i] = copy.m_stack[i];

But, your array m_stack contains m_size elements (assuming that you initialized them).

EDIT: The only way (I could see) to overlap m_stack and copy.m_stack is if placement new operator is used. But, according to posted source, you didn't do it.

It seems to me like the most likely culprit is actually the input to the copy constructor.

However, I will say that this code doesn't do nearly enough validation and the way it is constructed is going to make it hard to detect a bug or misuse until it crashes at some point down the road. A class written in this manner will produce hard to find bugs, as you're seeing now.

Simple things you should do at minimum: 1. Handle new throwing an exception 2. Check the bounds before you index into an array, even if you're just using m_stack[0] 3. Check that the input to the copy constructor is valid (aka initialized)

Oh, and the easiest way to track down memory errors like this is just to valgrind it. It's surprisingly good at flushing out memory issues quickly, even thought that's not its stated goal.

Also this would be super simple to debug if you included a working chunk of code that isolated the problem in it's simplest reproducible form.

You can replace the entire code posted by this:

class FastStack {
public:

protected:
    std::vector<boost::shared_ptr<DataObj> > m_Stack;
};

(and if you need to add a constructor that makes sure that the vector has preallocated its memory).

When you are doing unnecessary work like you are doing here by implementing classes that someone else has already done better ("someone else" are in this case smart people, smarter than most of us) it is likely that you add more problems than you solve.

std::vector is going to be a far better collection than your allocated array and boost::shared_ptr (or tr1::shared_ptr or std::shared_ptr if you are using a tr1/c++0x-compatible compiler) is doing exactly what your ObjectRef class is doing (but better). From your code it appears as if DataObj contains its own refcount, which you can also scrap if you use shared_ptrs instead.

Use the tools that are available - the less code you have to write, the less chance you have of writing bugs... If you replace your code with the above and are still seeing problems, your error is somewhere else.

May it be that your code works with multiple threads?

Since new never ever returns memory that is already allocated the only way that you get the check message is that another threads is doing something to the copy instance.

This suspicion is also raised by the fact that it only happens from time to time. This is typically for multi-threading bugs (as they need a special timing to happen that only occur from time to time).