12

I'm investigating a memory leak and from what I see, the problem looks like this:

int main(){
    char *cp = 0;
    func(cp);
    //code
    delete[] cp;
}

void func(char *cp){
    cp = new char[100];
}

At the //code comment, I expected cp to point to the allocated memory, but it still is a null pointer meaning I never delete the memory. What am I doing wroing?

  • 1
    I assume `cbuf` was suppose to be `cp`? – GManNickG Dec 18 '09 at 23:18
  • how about positing real code. Unless the code is cut and paste there is the posability of you adding errors. This just makes thigs harder and we end up solving cut and paster errors like cbuf -> cp – Martin York Dec 18 '09 at 23:23
  • Sorry about that, I'll keep that in mind for next time. Yes cbuf was supposed to be cp –  Dec 19 '09 at 00:01

7 Answers7

20

You are assigning cp the value of the allocated memory. However, that's a variable on the stack: a copy of the cp in main! cp is local to the function you're in.

What you want is a reference:

void func(char *& cp)

This will alias cp to be the parameter passed in.

GManNickG
  • 494,350
  • 52
  • 494
  • 543
13
void func(char *cp){
    cp = new char[100];
}

In this function, char *cp is a "pointer being passed by copy" what means that they are pointing to the same memory address but they are not the same pointer. When you change the pointer inside, making it to point to somewhere else, the original pointer that has been passed will keep pointing to 0.

Andre Pena
  • 56,650
  • 48
  • 196
  • 243
6

The parameter cp is a local variable of the function - changing it does not alter anything outside the function. A better way to write the function is:

char * func(){
    return new char[100];
}

And not to do directly with your question, but you should probably be using std::string and std::vector rather than dynamically allocated arrays.

  • 1
    Hmm, two downvotes. Not that I care particularly, but others here might benefit from knowing what is wrong with this answer. –  Dec 18 '09 at 23:33
  • Nothing that I can see. If you're going to point out something like the C++ features you mentioned, I think it's probably good to also add a part about not hard-coding the array size. Additionally, if you're going to use those particular features, you probably want to pass a pointer to one in by reference to create it, or else pass an already-created one in by reference to avoid the copy constructor penalty... but there's really nothing WRONG with this answer. – San Jacinto Dec 18 '09 at 23:50
  • Pointers don't have a copy constructor. –  Dec 19 '09 at 10:54
  • I was referring to std::String and std::vector when speaking of "the c++ features you mentioned." I did mangle my reply, so sorry for the confusion. For what you have, I would add "don't hardcode...". the rest is referring to the "c++ features." – San Jacinto Dec 21 '09 at 02:48
1

You're passing in cbuf, not cp.

Skilldrick
  • 69,215
  • 34
  • 177
  • 229
1

The function is only changing a COPY of cp. Use a reference instead.

Jesse Emond
  • 7,180
  • 7
  • 32
  • 37
1

Although references are wonderful in offering an intuitive abstraction, enhanced further by C++11 rvalue references to allow function chaining (and other esoteric coding), it is arguable that they provide any safety (viz why is a reference considered safer than a pointer) There are instances where it is better to resolve the above with a pointer to pointer function argument. Specifically when there is the need to maintain a similar codebase in ansi c and c++.

#include <iostream>

using namespace std;

void func(char ** cp) {
    *cp = new char[100];
    //do something useful
    (*cp)[0] = 'A';
}

void func(char *& cp) {
    cp = new char[100];
    //do something useful
    cp[0] = 'B';
}

int main(int argc, char** argv) {
    char * cp;
    //pointer to pointer
    func(&cp);
    cout << "Index 0 : " << cp[0] << '\n' << flush;
    delete[] cp; //remember to delete!!
    //pointer to ref
    func(cp);
    cout << "Index 0: " << cp[0] << '\n' << flush;
    delete[] cp;
    return 0;
}

Of course the disposal of memory resources out of the scope of the instatiating function disobeys RAII.

Community
  • 1
  • 1
claydonkey
  • 149
  • 2
  • 6
0

As GMan and Neil mentioned, in order to work you will have to change func to:

char* func();

or void func(char*& p);

which will solve your immediate problem.

There is, however, a maintenance problem. In either case, func returns a pointer. What is not clear to the user of func is that returned pointer will have to be deleted. For this reason, generally avoid this construct unless 100% necessary. Rather:

  1. Help the user allocate the correct amount of memory which can then be passed to func
  2. Use an object to store the allocated memory. The object can then delete the character array when it is destructed.

So for C++ code,I would recommend:


class CBuf
{
public
    CBuf() 
    {
        iBuf = new char[100];
    }
    ~CBuf
    {
        delete[] iBuf;
    }
    char* func()
    {
        //do stuff;
        return iBuf;
    }
private:
    char* iBuf;
};

int main()
    {
    CBuf cb;
    char* mychar = cb.func();
    //do stuff with character array

    //destructor gets called here because cb goes out of scope
    }

However, in C programming especially, it might be 100% necessary to have some sort function to create the array. Therefore in C programming you can replace the destructor with a CreateCBuf and DestroyCBuf function. In this way the user of your library will know that the buffer returned needs to be destroyed.

doron
  • 27,972
  • 12
  • 65
  • 103
  • std:vector is fine. It handles all the memory management for you. The problem in OPs example is that ownership is very unclear. – doron Dec 20 '09 at 17:28