Pointer Failure (C++)

Question

Possible Duplicate:
c++ warning: address of local variable

char* strdup(const char* s)
{
    char dup[strlen(s)];
    for (int i = 0; i<strlen(s); i++)
    {
        dup[i]=s[i];
    }
    return dup;
}

This function is supposed to hold the new array that has been read backwards plus another slot. When I compile it I get the error "warning: address of local variable 'dup' returned" and when I run the program it returns the memory address.

A typical design is to get the destination address as an argument. This overcomes the issue mentioned below — julx, Mar 22 '11 at 20:43
Beware. Your code might seem to work correctly, and you might be tempted to ignore the compiler warnings. However, your code has a major defect in it, and it will explode violently. — John Dibling, Mar 22 '11 at 20:45
As an aside, `strdup` is reserved (along with all other names starting with `str`), so even when you fix the memory allocation problem, the code *could* still fail. Since you've tagged this as C++, you should almost certainly just use `std::string` instead. — Jerry Coffin, Mar 22 '11 at 20:49
It's spelled [Johnnie Walker](http://en.wikipedia.org/wiki/Johnnie_Walker). ;) — GManNickG, Mar 22 '11 at 21:05

Oliver Charlesworth · Answer 1 · 2011-03-22T20:46:17.593

char dup[strlen(s)] defines a local stack variable; this goes out of scope when the function ends, so any attempt to access it after that will result in undefined behaviour.

Your options are:

Use a heap variable (obtained using new). You will have to remember to delete it at some point.
Have the function write into an existing buffer, provided by the caller (e.g. void strdup(char *out, const char *in)).
Use C++ constructs like std::string, which do all the hard work for you.

As you have marked your question "C++", I strongly recommend Option #3.

BrMcMullin · Answer 2 · 2011-03-22T20:48:44.613

1

Your definition specifies an char array pointer as its return type but you initialize a char array inside your function and try to return it. Try this:

char* strdup(const char* s)
{
    char *dup = new char[strlen(s)];
    for (int i = 0; i<strlen(s); i++)
    {
        dup[i]=s[i];
    }
    return dup;

}

edited Mar 22 '11 at 20:48

answered Mar 22 '11 at 20:42

BrMcMullin

1,261
2
12
28

Nope, it doesn't work that way. You're defining a pointer to a char array and return that (uninitialized) pointer. – DarkDust Mar 22 '11 at 20:44

DarkDust · Answer 3 · 2011-03-22T20:50:56.300

0

The dup variable is an array of char and is allocated on the stack rather than the heap (via new or malloc). As soon as the stack frame is left (that is: the function is left) this is undefined memory that will be overwritten by other things soon.

You need to turn dup into a char * and use new or malloc to allocate the necessary memory.

edited Mar 22 '11 at 20:50

answered Mar 22 '11 at 20:41

DarkDust

90,870
19
190
224

score 0 · Answer 4 · answered Mar 22 '11 at 20:41

The problem is that you never allocate dup on the heap, so when you exit the stack frame, dup will automatically be removed with the stack frame. This means that it's not possible to have a valid reference to dup, as it ceases to exist once you exit the function.

score 0 · Answer 5 · answered Mar 22 '11 at 20:42

0

This should work:

char* strdup(const char* s)
{
    char* dup = new char[strlen(s)];
    for (int i = 0; i<strlen(s); i++)
    {
        dup[i]=s[i];
    }
    return dup;
}

EDIT: when you are done, don't forget to use 'delete' to free the memory ;)

answered Mar 22 '11 at 20:42

Javi R

2,320
1
17
21

score 0 · Answer 6 · answered Mar 22 '11 at 20:42

you can't return dup[] because, as it is, it's a local variable and won't be valid outside the function (well, the memory it points to won't be valid anymore). You have to call something like malloc(), which allocates memory on the heap (space visible by all your app)

score 0 · Answer 7 · answered Mar 22 '11 at 20:43

char* strdup(const char* s)
{
    char dup[strlen(s)]; // defines *local* variable on *stack*
    for (int i = 0; i<strlen(s); i++)
    {
        dup[i]=s[i];
    }
    return dup; // returning dup[0] = dup address
}

You are returning address of local variable, created on stack. When you return from the function the stack will be rewind and your dup variable gone.

score 0 · Answer 8 · answered Mar 22 '11 at 20:43

The line

char dup[strlen(s)];

will not work in C++. Arrays need a constant size specified at compile time; strlen(s) is a variable.

As far as your actual warning is concerned, it is a bad practice to return a pointer to a local variable to the caller; since the local variable (in this case, the array dup) is allocated on the stack, when the function returns, it is deallocated, and hence, the returned pointer may be invalid. Compilers are designed to catch such errors and flag a warning saying that this could be a potential source of problems.

This is more than bad practice and a "potential source of problems", it is totally *undefined behaviour*! In all likelihood, this will crash and burn if used in a non-trivial program. — Oliver Charlesworth, Mar 22 '11 at 20:50

Pointer Failure (C++)

8 Answers8