Corrupt pointer on function return

[aatwo]

Hey guys. I just got told off by my boss for a snippet of code and in the interest of self improvement I decided to seek further clarification on what is wrong with it =)

I basically have a list of objects as follows:

QList<MyObject> mMyObjects;

I then have some code in some function that iterates through the list and returns a pointer to one of the objects if a certain condition is met. For example:

MyObject* GetMyObject()
{
    for( auto it = mMyObjects.begin(); it != mMyObjects.end(); ++it )
    {
        if( someCondition )
            return &(*it);
    }
    return nullptr;
}

I've been told that this snippet of code is bad because "some compilers will corrupt the stack on pointer return" and that "live pointers being returned from inside loops can corrupt the stack", but I don't know why this would affect this code.

Any thoughts?

[Duthomhas]

Your boss doesn't know what he's taking about. No extant compiler will produce code that corrupts the stack when returning a pointer value. Any compiler that did/does is nonconforming and worthless. (The flaw would be almost instantly manifest.)

[coder777]

As Duoas said: the stack is not the problem, but as always with raw pointer you might later point to an object that does not exists and when accessing the heap may be corrupted.

[Duthomhas]

After having posted a response, I occurred to me that this does not necessarily help you with your boss.

Can you return, say, an iterator to the object?
Or, even though it may be less efficient, an index into the list of objects?

(In other words, the code need not be perfect, it just needs to work.)

[dhayden]

When returning a pointer you need to be clear about who owns the pointed-at object. In other words, who is responsible for deleting it? If not the caller, then how long can the caller assume that the pointer will remain valid? If you define these things carefully and document them, then the function is fine.

In this case, I assume that *it returns a reference to an object in the mMyObjects, and thus &(*it) is a pointer to the object. In that case the pointer is valid until the object is moved or deleted. A safe policy is to say that it's valid until the first non-const method call to mMyObjects. I believe that is how std::string::c_str() is defined.

[aatwo]

Thanks for the replies guys.

To be honest I don't know precisely why this code was flagged. It might have spotted it as part of a code review and this is just something he looks out for. Alternatively there may have been a bug in that part of the code that this snippet of code got the blame for; potentially pointer ownership related. But I like to think I would not get the blame for someones accidental misuse of pointers!

However to respond directly to Duoas, I have been told to change anywhere I am returning from within a loop to instead store anything to be returned on the stack just outside of the loop, break from the loop then return. For example the snippet provided in my original post would become:

MyObject* GetMyObject()
{
    MyObject* ret = nullptr;
    for( auto it = mMyObjects.begin(); it != mMyObjects.end(); ++it )
    {
        if( someCondition )
        {
            ret = &(*it);
            break;
        }
    }
    return ret;
}

I mean I don't want to cause any friction here with my boss, I simply want to learn as much as possible. I'm happy to take it as criticism and adjust how I write loops, but I'll just keep it to myself as I want to avoid promoting the spread of misinformation!

[cire]

To be honest I don't know precisely why this code was flagged. It might have spotted it as part of a code review and this is just something he looks out for.

Superficially, it may appear to be returning the address of a local object. The use of return &object; is always something one has to take a second look at.

[dhayden]

I have been told to change anywhere I am returning from within a loop to instead store anything to be returned on the stack just outside of the loop, break from the loop then return.

That's pretty silly advice, at least in my opinion, especially since this:

    MyObject* ret = nullptr;
    for( auto it = mMyObjects.begin(); it != mMyObjects.end(); ++it ) {
        if( someCondition ) {
            ret = &(*it);
            break;
        }
    }
    return ret;

... and this:

    for( auto it = mMyObjects.begin(); it != mMyObjects.end(); ++it ) {
        if( someCondition ) {
            return &(*it);
        }
    }

... are not equivalent code. The first example always returns while the second one only returns if someCondition is true at some point. Using your boss's rule, if you wanted to return conditionally, you'd have to change the last statement to

if (ret) {
    return ret;
}

Now the code is getting much longer and more complicated, all in the name of following some rule.

[cire]

Using your boss's rule, if you wanted to return conditionally, you'd have to change the last statement to

I really doubt you'll find coding guidelines anywhere that allow you to write the latter and court undefined behavior. If your function is supposed to return a value, return a value.

[helios]

Those two snippets are not like what OP posted.

MyObject* GetMyObject()
{
    for( auto it = mMyObjects.begin(); it != mMyObjects.end(); ++it )
    {
        if( someCondition )
            return &(*it);
    }
    return nullptr;
}

MyObject* GetMyObject()
{
    MyObject* ret = nullptr;
    for( auto it = mMyObjects.begin(); it != mMyObjects.end(); ++it )
    {
        if( someCondition )
        {
            ret = &(*it);
            break;
        }
    }
    return ret;
}

These two functions are equivalent, and neither has undefined behavior.