Returning ref to data from function leads to dangling ref issue

Question

Please check the two code snippets below. While in sample 2, there clearly resides dangling reference issue as ref of local variable is passed, do you think the same problem exists in sample 1? I myself think sample 1 is correct. While data was pushed in data structure (stl::queue), the ref was taken (function header of enqueue is void enqueue(const int &data)). So there should not be problem while returning data through &data here. Or is there?

Sample 1:

int const& dequeue()
{
    _mutex.lock();
    int &data = _queue.back();
    _queue.pop();
    _mutex.unlock();
    return data;
}

Sample 2:

int const& dequeue()
{
    _mutex.lock();
    int data = _queue.back();
    _queue.pop();
    _mutex.unlock();
    return data;
}

Sample 2 is definitely broken, you return a temporary by reference. For Sample 1 please clarify, does `pop()` remove the item you got via `back()`? In that case `data` would also become an invalid reference. — Kerrek SB, Jun 22 '11 at 01:20
@Kerrek, I don't think that's the point of the question, but some clarification would still be nice - if the question was asked because the code doesn't work, that may be why. — Dominic Gurto, Jun 22 '11 at 01:27
I checked [the documentation](http://www.cplusplus.com/reference/stl/queue/pop/): `pop()` removes the _oldest_ element which is accessed with `front()`. So if your queue has only one element, then you end up with an invalid reference. Otherwise you end up with a reference of undetermined fate; other threads may be popping your queue until it becomes invalid. — Kerrek SB, Jun 22 '11 at 02:07
Kerrek's point is that you're assigning `back` element, while popping `front` element. E.g. `q={1,2}; i=q.back(), q.pop()` then `q=={2}, i==2` — Alexander Malakhov, Jun 23 '11 at 01:48
duplicate of [Iterator invalidation rules](https://stackoverflow.com/questions/6438086/iterator-invalidation-rules) - these apply just as much to references or pointers as to class-type iterators — underscore_d, Aug 03 '17 at 12:24

Dominic Gurto · Answer 1 · 2011-06-22T04:21:59.377

1

Sample 1 is incorrect.

Your reference will become invalid once you call pop().

The reference the function will return must be to an object that is still valid when the function terminates.

This also means that Sample 2 is incorrect, but not for the reason you think. Yes, returning that reference is invalid (it shouldn't even compile), but data is invalid before the end of the function - again, right after the pop() call, any references you have to objects in that container become invalid.

Is there any reason you are returning by reference? You obviously don't expect the value to change at all, and your reference is const, so why not just return by value?

int dequeue()
{
    _mutex.lock();
    int data = _queue.back();
    _queue.pop();
    _mutex.unlock();

    return data;
}

In answer to your more general question, about returning a locally-declared reference, that's fine, as long as the object to which it refers will still be valid once the function terminates. For example:

int glob;

int& f(){
  int x;
  int& ref = glob;
  return x;
}

int main(){
  foo()=10;  //this is fine
}

edited Jun 22 '11 at 04:21

answered Jun 22 '11 at 01:22

Dominic Gurto

3,545
2
14
16

Your example is fine, but don't references to members of containers become invalid if you modify the container appropriately? E.g. `set v{2,7,5,12}; int & a = *v.begin(); v.clear();` -- now writing to `a` is bad, non? – Kerrek SB Jun 22 '11 at 01:37
Well of course, but that would invalidate any iterator - there's nothing special about the fact that `a` is a reference. `int& a = *v.begin(); vector::iterator b = v.begin(); v.clear();` - writing to `a` is bad, but writing to `b` is equally bad. – Dominic Gurto Jun 22 '11 at 01:45
1

@Dominic: But what has `a` got to do with any iterators? I've dereferenced `begin()`, so I get a reference to some element in the container. In a vector I could have written `int & a = v[3];` same idea (just that in a vector the memory would probably still have been valid after `clear()`). `a` is just an int-reference. Isn't that the same situation with the queue? (Assuming `back()` returns a reference.) – Kerrek SB Jun 22 '11 at 01:51
1

Oh, I see what you mean. Yes - I'd say `pop()` invalidates any iterators and references. And `back()` does return a reference. – Dominic Gurto Jun 22 '11 at 01:57
He does modify the container in a way that almost certainly makes the first example invalid: he pops the top element. The design of that function is safe; but that implementation is not. – Dennis Zickefoose Jun 22 '11 at 01:59
1

@Dennis: Almost. He pops the "bottom" element, if that's the way you picture your queue. But it's still reckless because he's not the only one accessing the queue and eventually that reference may become invalid. – Kerrek SB Jun 22 '11 at 02:09
The problem was that I completely misunderstood the question - I took it as a more general question of whether a locally-declared reference could be returned from a function, just from comparing the two samples. I'm still not sure that wasn't the asker's intention. – Dominic Gurto Jun 22 '11 at 02:14
@Kerrek: oh, duh, queue, not stack. I agree though; all the mutexes in the world won't protect a loose reference like that. – Dennis Zickefoose Jun 22 '11 at 02:32
@Dennis: No worries, personally I had never taken conscious note of `std::queue` till now, I was always content with `deque` and I had to check myself (hence the question in my first comment to the OP). @Dominic: The OP is not saying very explicitly what his concern is, unfortunately. – Kerrek SB Jun 22 '11 at 02:41
As a side note, all the thread-safe containers I've seen so far (whether locking or lock-free) always return elements by value, not reference, and they have atomic pop+returns -- I'm sure there's a reason for that! I don't see just now how one could possibly use references like that in the presence of concurrency... any thoughts? – Kerrek SB Jun 22 '11 at 02:44
That's true, and especially since OP is returning a const reference, there would be no reason not to just return by value. I get the impression that that's what he wants to do anyway. – Dominic Gurto Jun 22 '11 at 03:57
Thanks. All the answers and comments are quite impressive. Somehow I just don't feel good to return by value to avoid any copy. What do you suggest for enqueue() and dequeue() functions? Call by pointer and return by pointer ? – Faisal Jun 22 '11 at 05:55
1

@Faisal: Pointers will give exaxtly the same problem as references. Any direct reference to an element (including a pointer) could become invalid due to the modification. Basically, if your function modifies the container, and you want to return an element of the container, returning by value is the only safe option. – Dominic Gurto Jun 22 '11 at 15:35
@Dominic: I think Faisal means `queue q; int* ip1=new int(5); q.push(p); int* ip2 = q.pop()`. This way he'll be copying pointers, and not assigning ref to pointers. This should be ok – Alexander Malakhov Jun 23 '11 at 01:36

Returning ref to data from function leads to dangling ref issue

1 Answers1