0

Here's my problem: I read here on StackOverflow that it is unsafe sometimes to return pointers to local variables from a function. For example:

#include<iostream>

using namespace std;

int *foo(void) {
    int x[] = {1,2,3};
    return x;
}

int main() {
    int *numbers;
    numbers = foo();
    return 0;
}

I'd like to know if this is unsafe, considering that x being a local array, the memory could be unallocated, what's the better way to achieve the same result?

Eitan T
  • 32,660
  • 14
  • 72
  • 109
Roland
  • 21
  • 4

6 Answers6

10

I read here on StackOverflow that it is unsafe sometimes to return pointers to local variables from a function.

It is always unsafe to return pointers to local variables. Indeed it is wrong to do so, and using this pointer will cause undefined behavior. See also this awesome post.

If you want to return data to the calling scope, you could use a std::vector as a copy:

std::vector<int> foo(void){
  std::vector<int> x = {1,2,3}; // using C++11 initializer list
  return x;
}

If it's a fixed length array (always of size 3), you could use std::array instead.


Depending on your requirements you may also use a static variable. That is, the variable will never go out of scope, s.t. you can return it safely by reference (or by pointer). Note that you have only one copy. If you modify it, it will remain modified. (Make it const & if it's read only.)

std::vector<int>& foo(void) {
  // this is only instantiated once when the function is first called
  static std::vector<int> x = {1,2,3}; 
  return x;
}
Community
  • 1
  • 1
moooeeeep
  • 31,622
  • 22
  • 98
  • 187
  • And due to move semantics, there is no performance penalty in returning a copy of the vector. – Naveen Jul 02 '12 at 10:44
  • 1
    It's not undefined behavior. It is wrong, because actually using (dereferencing) that pointer for anything will be undefined behavior. – Ben Voigt Jul 02 '12 at 13:10
0

First of all int x = {1,2,3} is syntax error. It should be int x[] = {1,2,3};

This is undefined behaviour. Because the automatic array has a lifetime inside its block where it is defined, that is inside the foo() function. So whenever you return from foo() the storage for x is no more guaranteed to be reserved for you. Therefore if you access that location through pointers then the behaviour is undefined.

To achieve the same result dynamically allocate memory.

int *foo(void){
int x[] = {1,2,3}, *x_to_return;
x_to_return = new int [sizeof (x)/sizeof(x[0])];
memcpy (x_to_return, x, sizeof (x));
return x_to_return;
}

Basically, what you need to do is to dynamically allocate the storage using new, copy your data to the block of memory (base of which is) allocated by new and return that memory address to the caller.

Don't forget to free the allocate the memory once you have finished using it, else your code would have memory leakage.

Also a point to be noted, if you have a declaration like static int x[] = {1,2,3}; then you can return the address of x, because in this case the lifetime of x is the entire program runtime.

As your question is tagged c++ you should use vector, check moooeeeep's answer.

phoxis
  • 60,131
  • 14
  • 81
  • 117
  • Sorry, using `new` now. and csting for `memcpy` . `vector` is a better way. – phoxis Jul 02 '12 at 10:51
  • 1
    Note that you could also use [std::copy](http://en.cppreference.com/w/cpp/algorithm/copy) instead of `memcpy`. – moooeeeep Jul 02 '12 at 10:54
  • Yep, I am not much used to with c++ specific functions actually, and keep an online manual in hand. I have pointed to your answer, which is more appropriate. – phoxis Jul 02 '12 at 10:56
  • 1
    You are using `new` incorrectly. `new int[n]` allocates `n` ints, so you are allocating `sizeof(x)*sizeof(int)` ints, which is quite a bit more then needed. Furthermore a more c++ way would use `std::copy` instead of `memcpy` – Grizzly Jul 02 '12 at 10:56
  • 1
    @phoxis: Now you're copying memory from outside the boundaries of `x`, which is undefined behaviour. You want `new int [sizeof(x)/sizeof(x[0])]`. – molbdnilo Jul 02 '12 at 12:06
0

It is unsafe (or rather wrong) to return a pointer to that array, because it does not exist any more when the function returns. The memory could not just be deallocated, but it will be.
Note that it might still accidentially work, but honestly that would be worse than if it didn't work (because it's unpredictable and impossible to debug). Never attempt "but it seems to work" things, even if they seem to work.

This is the same for returning a pointer or a reference, with a const reference being an exception. A const reference keeps the referenced object alive for its own lifetime.

Dynamically allocating or making the object static would be options if you want to return a pointer. Or, just return a temp object by value, relying on the compiler to RVO it out.

Damon
  • 67,688
  • 20
  • 135
  • 185
0

You can either:

  1. Declare x as static
  2. Declare x as a pointer
Fred
  • 4,894
  • 1
  • 31
  • 48
George Nechifor
  • 439
  • 3
  • 12
0

It is never safe to to return a pointer to a local variable (it's undefined behaviour in fact). For most implementations those live on the stack, so it can be overwritten when the pointer is used.

You can return an dynamically allocated array:

int* foo() { int* x = new int[3]; ..}

Of course you would need to manually delete the pointer, which makes it hard to write robust, excpetion safe code. Therefore it's typically preferable to use a vector:

std::vector<int> foo() { 
   std::vector<int> x;
   x.push_back(1); 
   x.push_back(2); 
   x.push_back(3);
   return x;
}

If you use c++11, you can use an initialization list to fill the vector, making the code much nicer:

std::vector<int> foo() { std::vector<int> x = {1,2,3};  return x; }

C++11 has move semantics, which means that in this case returning the vector by value costs almost no performance. For C++03 if performance is essential you could give the function a reference/pointer to vector as a parameter and fill that:

void foo(std::vector<int>& x) {x.clear(); x.push_back(1); ...}
Grizzly
  • 19,595
  • 4
  • 60
  • 78
-1

Yes it is unsafe because the array is allocated on the stack, as such it will be deallocated when the function returns.

Instead of allocating the array inside the function, create it outside and pass a pointer to it to the function. This is just an example:

#include<iostream>
using namespace std;

void foo(int numbers[]){
    numbers[0] = 1;
    numbers[1] = 2;
    numbers[2] = 3;
}

int main(int args, char**argv) {
    int numbers[3];
    foo(numbers);
    cout << numbers[0] << numbers[1] << numbers[2];
    return 0;
}

This will print "123".

Trasplazio Garzuglio
  • 3,535
  • 2
  • 25
  • 25
  • -1 for array-to-pointer decay and sending an array in an unsafe way. – Puppy Jul 03 '12 at 08:18
  • 1
    @DeadMG, the point of my answer was to show the original poster that you can create the container outside the function and pass it in as a parameter instead of creating it originally inside the function. I don't think using vectors help him understand the basics. – Trasplazio Garzuglio Jul 03 '12 at 09:09
  • There is nothing "basic" about this stuff. It's vastly more difficult to use and unreliable. It is the opposite of basic. It is "Please destroy my program whenever I make a trivial mistake and don't tell me about it". Nobody should ever do this stuff, and *doubly* so for learners. – Puppy Jul 03 '12 at 10:24